Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
9bcc941 to
79f61b0
Compare
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval waits on an owner decision: no current spec covers ChatLayout's new keyboardDismissMode prop or its default-on 'interactive' dismissal. An engineering owner needs to provide or identify the governing spec, meaning whether ChatLayout ships the prop and which default it uses, and record it by commenting /approve-spec <full head SHA> on this pull request; the author can do this if they are an engineering owner. Independently of that decision, these changes are needed:
- The diff includes four open predecessor pull requests (#8045, #8067, #8074, #8176), about 27,000 authored lines, so please land them through their own reviews and rebase this branch down to its ChatLayout commit.
'onDrag'blurs the composer even when no software keyboard is shown, unlike'interactive', so please gate it on the keyboard being up and add a test.- The new draft
spec:AST-094needs its own owner ruling in the pull request that introduces it, and its FR7 citesspec:AST-090, which is not in the repository.
[Automated review]
| }; | ||
|
|
||
| // The browser took the touch to scroll: a drag began. | ||
| const onPointerCancel = (event: PointerEvent) => { |
There was a problem hiding this comment.
'onDrag' dismisses here without checking that a software keyboard is up, while 'interactive' returns early when guide.inset <= 0 (line 127). On a touch screen used with a hardware keyboard, a ChatLayout set to onDrag loses the composer's focus on every touch scroll of the transcript. Consider requiring guide.inset > 0 for onDrag too (at the touch start or here), and adding the "no keyboard" case to the onDrag tests.
| name: 'ChatLayout', | ||
| displayName: 'Chat Layout', | ||
| description: '完整聊天界面的布局外壳。消息在页面中自然流动，编写器固定在底部，带有毛玻璃效果。通过 density 属性控制密度。', | ||
| description: '完整聊天界面的布局外壳。消息在页面中自然流动，编写器固定在底部，带有毛玻璃效果。通过 density 属性控制密度。在手机上，编写器随软键盘同步上移并停在键盘上方，停留在最新消息的对话保持在最新消息；轻点编写器会聚焦它而不滚动页面。', |
There was a problem hiding this comment.
docsZh was not updated for this change: its description omits the drag-to-dismiss behavior that is now the default, and propDescriptions has no keyboardDismissMode entry, so Chinese readers see the English text for the new prop.
79f61b0 to
187dadb
Compare
There was a problem hiding this comment.
Approval waits on an owner decision: no current spec covers ChatLayout's new keyboardDismissMode prop or its default-on 'interactive' dismissal. An engineering owner needs to provide or identify the governing spec, meaning whether ChatLayout ships the prop and which default it uses, and record it by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. Independently of that decision, these changes are needed:
- The diff includes four open predecessor pull requests (#8045, #8067, #8074, #8176), about 27,000 authored lines, so please land them through their own reviews and rebase this branch down to its ChatLayout commit.
'onDrag'blurs the composer even when no software keyboard is shown, unlike'interactive', so please gate it on the keyboard being up and add a test for that case.- The new draft
spec:AST-094needs its own owner ruling in the pull request that introduces it, and its FR7 citesspec:AST-090, which is not in the repository. docsZhinChatLayout.doc.mjshas nokeyboardDismissModeentry and does not mention the new default.
[Automated review]
187dadb to
edc7843
Compare
There was a problem hiding this comment.
Approval waits on an owner decision: no current spec covers ChatLayout's new keyboardDismissMode prop or its default-on 'interactive' dismissal. An engineering owner needs to provide or identify the governing spec, meaning whether ChatLayout ships the prop and which default it uses, and record it by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. Independently of that decision, these changes are needed:
- The diff includes six open predecessor pull requests (#8045, #8067, #8074, #8176, #8258, #8287), about 29,800 authored lines, so please land them through their own reviews and rebase this branch down to its ChatLayout commit.
'onDrag'blurs the composer even when no software keyboard is shown, unlike'interactive', so please gate it on the keyboard being up and add a test for that case.- The draft
spec:AST-094needs its own owner ruling in the pull request that introduces it: its FR7 citesspec:AST-090, which is not in the repository, and its FR4 rules out the theme-tunable touch motion that #8258 adds. docsZhinChatLayout.doc.mjshas nokeyboardDismissModeentry and does not mention the new default.
[Automated review]
|
Posted by Cindy's agent on her behalf. Parts of this PR re-implement behavior that already exists, or is being specified, as a shared part of core. Converging keeps one implementation per behavior:
This is about where the behavior lives, not about the PR's intent. |
|
Agreed: one implementation.
|
edc7843 to
cd49b1b
Compare
github-actions
Bot
added
the
needs:design-review
There was a problem hiding this comment.
Approval waits on an engineering-owner decision: component:ChatLayout FR10 and DEC-1 add drag-to-dismiss as built-in behavior with no caller control, and the new spec:AST-094 FR43 entry keeps it off under a mouse, but nothing approves them at this head.
To settle it, an engineering owner approves FR10, DEC-1 and the FR43 entry as written by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or names the pointer scope they want, since FR10 follows a finger and a pen while DEC-1 records the decision as touch behavior.
Blocking, independent of that decision:
- Miss:
focusedEditableandnodeOfinpackages/core/src/Chat/transcriptDismiss.ts(lines 33–48) copydeepActiveElementfromutils/flatTree.tsandisNodeTargetfromkeyboard/keyboardHeight.ts, so call those instead and a fix to either reaches this drag. - Miss: the changeset and
ChatLayout.doc.mjssay the drag dismisses "with a software keyboard up", but the tests prove it does nothing in a frame or underinteractive-widget=resizes-content, so say "in a top-level page (not underresizes-content)" as FR10 does.
The earlier FR10 wording fix is in, and making both fixes before the approval avoids approving twice.
[Automated review]
| let active = doc.activeElement as HTMLElement | null; | ||
| // Focus inside a shadow root shows as its host. | ||
| while (active?.shadowRoot?.activeElement != null) { | ||
| active = active.shadowRoot.activeElement as HTMLElement; | ||
| } |
There was a problem hiding this comment.
Miss (One shared path): this walk repeats deepActiveElement in utils/flatTree.ts, which the input hold and the selection lock call. Suggested: const active = deepActiveElement(doc); return bringsUpKeyboard(active) ? (active as HTMLElement) : null;
| function nodeOf(target: unknown): Node | null { | ||
| return target != null && | ||
| typeof target === 'object' && | ||
| typeof (target as Node).nodeType === 'number' | ||
| ? (target as Node) | ||
| : null; | ||
| } |
There was a problem hiding this comment.
Miss (One shared path): isNodeTarget in keyboard/keyboardHeight.ts, the module this file already imports getKeyboardHeight from, makes the same check. Suggested: return isNodeTarget(target as EventTarget | null) ? (target as Node) : null;, or call it where the touch's target is read.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] ChatLayout: with a software keyboard up, dragging the conversation down to the composer with a finger or a pen and lifting there now dismisses the keyboard, as in Messages; a release moving back up keeps it. A mouse drag still selects text and moves focus as before. A web page cannot move the system keyboard, so the keyboard leaves on its own at the lift instead of following the finger. Drags that start in the composer never dismiss it, and dismissing moves focus nowhere. Drag-to-dismiss has no caller control. Typing with a keyboard is unchanged. |
There was a problem hiding this comment.
Miss (claims match scope): the tests prove nothing happens in a frame or under interactive-widget=resizes-content, as FR10 states. Suggested: "with a software keyboard up in a top-level page (not under interactive-widget=resizes-content)", and the same in ChatLayout.doc.mjs.
…pans to a focused field
While Safari on iOS 26.2 pans the page to a focused field near the bottom,
it reports innerHeight as the visual viewport's height (377 of 714 with
the keyboard up). layoutViewportHeight read innerHeight, so the keyboard
module measured 0, as if no keyboard were up, against AST-099 FR2 ("a page
pan is never a smaller keyboard"). The root's client height keeps the
layout viewport's height. At a visual-viewport scale of 1, where
innerHeight equals the visual viewport's height and the root is as wide
as the visual viewport, layoutViewportHeight now takes the larger of the
two. Everywhere else it is unchanged: a zoomed-out page's scaled
innerHeight and the root's unscaled height are in different units, a
layout wider than the screen gives a root taller than the screen, and the
scroll clamp reads as before.
The README's ride formula now takes the keyboard's top from
layoutViewportHeight(window), not innerHeight.
6d366f5 to
97e4115
Compare
github-actions
Bot
added
the
needs:design-review
|
/approve-spec 97e4115 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Changes requested. An engineering owner has approved component:ChatLayout FR10 and DEC-1 and the new spec:AST-094 FR43 entry at this head, so the drag-to-dismiss and the composer's follow are settled direction; three fixes remain.
- Miss:
focusedEditableandnodeOfinpackages/core/src/Chat/transcriptDismiss.ts(lines 33–50) copydeepActiveElementfromutils/flatTree.tsandisNodeTargetfromkeyboard/keyboardHeight.ts, so call those instead and a fix to either reaches this drag. - Miss: the changeset and
ChatLayout.doc.mjssay the drag dismisses "with a software keyboard up", but the tests prove it does nothing in a frame or underinteractive-widget=resizes-content, so say "in a top-level page (not underresizes-content)" as FR10 does. - The diff is 2,050 authored lines, 175 of them the keyboard-height commit that is open on its own as #9983, so add the identical line
Stack: #9983 → #8217to both pull request bodies or land #9983 first and rebase.
Non-blocking: FR10 says "the keyboard's edge" follows the finger where the code moves the frosted layer, which is worth fixing in the same push. A new commit needs a fresh /approve-spec for the new head, which the author may give as an engineering owner.
[Automated review]
| function focusedEditable(doc: Document): HTMLElement | null { | ||
| let active = doc.activeElement as HTMLElement | null; | ||
| // Focus inside a shadow root shows as its host. | ||
| while (active?.shadowRoot?.activeElement != null) { |
There was a problem hiding this comment.
Miss (One shared path): this walk repeats deepActiveElement in utils/flatTree.ts, which the input hold and the selection lock call. Suggested: const active = deepActiveElement(doc); return bringsUpKeyboard(active) ? (active as HTMLElement) : null;
There was a problem hiding this comment.
Fixed in 1fd9e0e: focusedEditable now calls deepActiveElement.
|
|
||
| /** The node that a contact began on, from any window. */ | ||
| function nodeOf(target: unknown): Node | null { | ||
| return target != null && |
There was a problem hiding this comment.
Miss (One shared path): isNodeTarget in keyboard/keyboardHeight.ts, the module this file already imports getKeyboardHeight from, makes the same check. Suggested: return isNodeTarget(target as EventTarget | null) ? (target as Node) : null;
There was a problem hiding this comment.
Fixed in 1fd9e0e: nodeOf now calls isNodeTarget.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] ChatLayout: with a software keyboard up, dragging the conversation down to the composer with a finger or a pen and lifting there now dismisses the keyboard, as in Messages; a release moving back up keeps it. Once the finger is over the composer, the composer and the frosted layer under it follow the finger down, and they spring back to rest after the lift; under Reduce Motion they stay still and the pressed overlay fades in over the composer instead. A mouse drag still selects text and moves focus as before. A web page cannot move the system keyboard, so the keyboard leaves on its own at the lift instead of following the finger. Drags that start in the composer never dismiss it, and dismissing moves focus nowhere. Drag-to-dismiss has no caller control. Typing with a keyboard is unchanged. |
There was a problem hiding this comment.
Miss (claims match scope): the tests prove nothing happens in a frame or under interactive-widget=resizes-content, as FR10 states. Suggested: "with a software keyboard up in a top-level page (not under interactive-widget=resizes-content)", and the same in ChatLayout.doc.mjs.
There was a problem hiding this comment.
Fixed in 1fd9e0e, in the changeset and in both ChatLayout.doc.mjs strings.
| | FR7 | `density` MUST reflect on the root as the `chat-layout` target's `data-density`, and MUST select the dock, message-area, dock-inner, and blur-layer geometry together. | Current source, tests, and docs | | ||
| | FR8 | The layout MUST publish its scroll container and message content refs through `ChatLayoutContext` so the message list can register the observed content box. | Current source | | ||
| | FR9 | Accepted `ref`, DOM/data/ARIA props, `className`, `style`, and `xstyle` MUST reach and compose on the layout root. | `architecture:public-component-api/INV5–INV6, INV8` | | ||
| | FR10 | While the keyboard module measures a software keyboard (`spec:AST-099` FR2; none where a page sets `interactive-widget=resizes-content`, FR8), a drag down the transcript MUST dismiss the keyboard, as in Messages, when it lifts at or past the composer's top as the page draws it, with no projection (`spec:AST-094` FR36; UIKit's interactive dismissal, measured on the iOS 26.2 simulator, in `Chat/transcriptDismiss.ts`), and a release moving back up MUST keep it (FR37). The drag attaches for every pointer (`spec:AST-094` FR43) and dismisses the keyboard for a touch or pen pointer while a software keyboard is showing; a mouse drag is the browser's: it selects text, and the press moves focus. Dismissing only blurs the focused field; focus moves nowhere else. While a touch or pen drag can dismiss the keyboard, the composer and the keyboard's edge follow the finger on a rubber band (`spec:AST-094` FR3), show an armed state once the finger is over the composer, and on lift either dismiss or spring back to rest from the release speed (`spec:AST-094` FR38); under Reduce Motion nothing moves and the armed state is a non-moving cue; a mouse drag keeps the browser's behavior. Drag-to-dismiss is part of ChatLayout's behavior, with no caller control (DEC-1). The drag is a pan on the layout's root that never claims a touch or delays a scroll (`spec:AST-094` FR15, FR24). | `spec:AST-094` DEC-1, FR3, FR15, FR24, FR36–FR38, FR43; `spec:AST-099`; DEC-1 | |
There was a problem hiding this comment.
Non-blocking: "the composer and the keyboard's edge follow the finger" names a part the code does not move; edgeRef is the frosted blur layer, and the changeset says a page cannot move the keyboard. Suggested: "the composer and the frosted layer under it follow the finger".
There was a problem hiding this comment.
Kept as written. This FR10 line is the spec owners' approved wording, and "the keyboard's edge" names the frosted layer that sits at that edge. Any rewording is theirs.
| if (isReduced()) { | ||
| return; | ||
| } | ||
| // An armed composer moves at the finger's speed times the band's slope. |
There was a problem hiding this comment.
Non-blocking (INV29): this comment restates the expression below it; consider dropping it.
…ses the keyboard With a software keyboard up, a touch or pen drag down ChatLayout's transcript that lifts at or past the composer's top dismisses the keyboard, as in Messages. A release moving back up keeps it, and a drag that starts in the composer's text never dismisses. The drag decides once, at the lift, and the dismissal only blurs the field. A mouse never reaches it, so a mouse drag keeps the browser's text selection and focus. Once the finger is over the composer, the drag is armed: the composer and the frosted layer under it settle down by the composer's bottom padding, then follow the finger on UIKit's rubber band. Any lift springs them back to rest from the release speed, on the keyboard's spring. Under Reduce Motion nothing moves, and an armed drag fades the pressed overlay in over the composer. Chat/transcriptDismiss.ts makes a pan from the gesture system that never claims a touch, so the transcript still scrolls natively. It compares the lift with the composer's top as the page draws it, so a browser's own pan to the focused field moves the line with the page. Chat/composerFollow.ts moves the composer and the frosted layer with the shared motion animator. ChatLayout attaches the pan to its root with useGestureRecognizer, and the follow to its composer and frosted layer. component:ChatLayout gains FR10 and DEC-1, and spec:AST-094 FR43 lists the dismissal under a mouse among what stays off.
97e4115 to
1fd9e0e
Compare
|
/approve-spec 1fd9e0e |
There was a problem hiding this comment.
Both pull requests in this stack are approved, pending CI.
#9983: approve. The layout-viewport floor restores the keyboard height while Safari pans to a focused field (spec:AST-099 FR2), and the Android emulator readings at this head cover Chrome with Gboard and Firefox.
#8217: approve. FR10, DEC-1 and the spec:AST-094 FR43 entry carry an engineering owner's approval for this head, the earlier fixes are in (shared helpers, release-note scope, stack line), and the drag, the composer's follow and the Reduce Motion cue behave as FR10 says in a Chromium check.
Whole stack: pass. Merged onto main, it typechecks and its unit tests pass.
A pull request lands only when it and every pull request below it are approved. Approved pending CI: CI has not run on these heads yet; once #8217 is ready for review, a maintainer adds the ci:run label to both so CI runs.
Non-blocking: a test asserting that the composer's spring-back starts from the release speed would pin spec:AST-094 FR38.
[Automated review]
|
Noted: the test that pins the spring-back to the release speed ( |
github-actions
Bot
added
needs:spec-owner-review
|
/approve-spec 39a9502 |
There was a problem hiding this comment.
Changes requested on #8217; #9983 is approved.
#9983: approve. The keyboard height now reads the whole keyboard while Safari pans to a focused field, restoring spec:AST-099 FR2, and nothing has changed since its approval.
#8217: request changes. The rewritten FR10, DEC-1 and the spec:AST-094 FR36 and FR43 edits carry an engineering owner's approval for this head, but the composer's scroll hold needs these fixes:
- A two-finger pinch that starts on the composer has every move cancelled while the keyboard is up, so the page cannot be zoomed from there, which
spec:AST-094FR10 requires to stay the browser's; cancel only while exactly one finger is down (transcriptDismiss.ts:278-285). - The hold cancels the moves of touches no recognizer owns, from the first move, through a blocking listener beside a pan that never claims, which
spec:AST-094FR19 does not allow; add the case to FR19's exceptions for approval, or let the composer drag claim its touch (transcriptDismiss.ts:291-296). holdComposerScrollandisInComposerScrollercopy the gesture system's page hold (gestures/pageHold.ts) and scroll test (gestures/nativeScroll.ts); extendholdPageScrollto take the composer's decision and delete the copy (transcriptDismiss.ts:168-304).- The crossing dismissal keeps the keyboard down when the finger moves back up before the lift, which
spec:AST-094FR37 still forbids because the new FR36 exception names FR36 alone; extend the exception to FR37.
Whole stack: fails on #8217's fixes, and a pull request lands only when it and every pull request below it are approved; CI has not run on either draft head yet.
[Automated review]
| const onMove = (event: TouchEvent) => { | ||
| if ( | ||
| event.cancelable && | ||
| Array.from(event.changedTouches).some(touch => held.has(touch.identifier)) | ||
| ) { | ||
| event.preventDefault(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
This cancels the move whenever any changed touch started on the composer, however many fingers are down, so a two-finger pinch on the composer is cancelled too (real Chromium: 12 of 12 two-finger moves). spec:AST-094 FR10 keeps page pinch-zoom the browser's when no pinch recognizer claims the fingers. Cancelling only while exactly one finger is down, as gestures/pageHold.ts:145-153 does, plus a two-finger case, would fix it.
| // Not passive: a passive listener cannot keep a touch's scroll from | ||
| // starting. | ||
| composer.addEventListener('touchstart', onStart, {passive: true}); | ||
| composer.addEventListener('touchmove', onMove, {passive: false}); |
There was a problem hiding this comment.
The composer's only recognizer is a pan that never claims (claimsTouches: false, line 214), yet this blocking listener cancels its touches from the first move, under the pan's 10 px start, in any direction. spec:AST-094 FR19 says a touch no recognizer owns MUST NOT be cancelled outside five exceptions, none of which covers the composer, and that only an element with a recognizer that must claim mid-gesture registers a blocking listener. It also sits on every ChatLayout, so a native scroll starting on the composer waits for the main thread (FR19's known limit). Adding this case to FR19's exceptions for the owners to approve, or letting the composer drag claim its touch, would reconcile them.
There was a problem hiding this comment.
Took the exception route in d19cf69. FR19 lists the composer's page hold as a fifth exception, in the spec owners' wording. The hold exists only while a software keyboard is up, so no ChatLayout carries a blocking listener otherwise. Claiming would give the composer a host touch-action that blocks pinch-zoom.
| export function holdComposerScroll(composer: HTMLElement): () => void { | ||
| // The fingers held still, by identifier. | ||
| const held = new Set<number>(); |
There was a problem hiding this comment.
One shared path: this and isInComposerScroller (line 168) repeat what the gesture system already owns. holdPageScroll in gestures/pageHold.ts holds a page still under one finger with a per-touch decision, leaves a second finger alone, skips touches a recognizer owns and listens only while the pointer is coarse; canElementScroll in gestures/nativeScroll.ts:176 tests overflow and room per axis. Extending holdPageScroll to take the composer's decision while a keyboard is up, and deleting this copy, keeps one implementation.
There was a problem hiding this comment.
Done in d19cf69: holdComposerScroll and isInComposerScroller are gone. The composer holds holdPageScroll with its own decision, and both the hold and the composer pan test scrollers with canElementScroll.
| NavigationStack, and dismissing the keyboard (except ChatLayout's | ||
| transcript-drag dismissal, which follows `component:ChatLayout` FR10/DEC-1: | ||
| it dismisses when the drag reaches the composer); each use's commit point |
There was a problem hiding this comment.
The exception names FR36 only, but FR37 just below still says "A release whose velocity points back toward the start MUST cancel, whatever the travel so far", while the crossing dismissal keeps the keyboard down when the finger moves back up (ChatLayoutTranscriptDismiss.test.tsx:252-261). Extending the exception to FR37 would keep the record consistent.
There was a problem hiding this comment.
Done in d19cf69: FR37 carries the same scoped exception as FR36.
| | FR7 | `density` MUST reflect on the root as the `chat-layout` target's `data-density`, and MUST select the dock, message-area, dock-inner, and blur-layer geometry together. | Current source, tests, and docs | | ||
| | FR8 | The layout MUST publish its scroll container and message content refs through `ChatLayoutContext` so the message list can register the observed content box. | Current source | | ||
| | FR9 | Accepted `ref`, DOM/data/ARIA props, `className`, `style`, and `xstyle` MUST reach and compose on the layout root. | `architecture:public-component-api/INV5–INV6, INV8` | | ||
| | FR10 | While the keyboard module measures a software keyboard (`spec:AST-099` FR2; none where a page sets `interactive-widget=resizes-content`, FR8), a touch or pen drag down the transcript MUST dismiss the keyboard at the moment it reaches the composer's top as the page draws it, or lifts there, with no projection: a flick that lifts above the line keeps the keyboard (DEC-1). The composer MUST then give one small spring bounce on the keyboard's spring (`spec:AST-094` FR3), started at the finger's speed and no deeper than the composer's bottom padding, so it never moves faster than the finger; the bounce moves the dock container, so the composer, the frosted layer and the scroll-to-bottom button move together, and under Reduce Motion nothing moves. A touch or pen drag that starts on the composer MUST dismiss the keyboard once it moves down, more down than across, past the pan's hysteresis, unless it held still for a long press first (iOS's text loupe, whose drag moves the caret). While a software keyboard is up, no drag that starts on the composer MAY scroll content: its moves are cancelled, except in a part of the composer that scrolls on its own on either axis, such as a long draft or a strip of attachments (DEC-1). The drags attach for every pointer (`spec:AST-094` FR43); a mouse drag is the browser's: it selects text, and the press moves focus. Dismissing only blurs the focused field; focus moves nowhere else. Drag-to-dismiss is part of ChatLayout's behavior, with no caller control (DEC-1). Both drags are pans that never claim a touch (`spec:AST-094` FR15, FR24); the composer's scroll hold is one touchmove listener on the composer alone. | `spec:AST-094` DEC-1, FR3, FR15, FR24, FR43; `spec:AST-099`; DEC-1 | |
There was a problem hiding this comment.
Non-blocking: "the composer's scroll hold is one touchmove listener on the composer alone" — holdComposerScroll registers four (touchstart, touchmove, touchend, touchcancel), as ChatLayoutTranscriptDismiss.test.tsx:385-391 asserts.
There was a problem hiding this comment.
Fixed in d19cf69: FR10 now says the page hold listens only while a software keyboard is up.
| // Not passive: a passive listener cannot keep a touch's scroll from | ||
| // starting. |
There was a problem hiding this comment.
Non-blocking (INV29): this comment sits over a touchstart registered passive: true. Moving it to the touchmove line keeps the reason next to the listener it explains.
There was a problem hiding this comment.
That listener is gone in d19cf69; the shared page hold owns the one blocking listener.
|
/approve-spec d19cf69 |
There was a problem hiding this comment.
Changes requested on #8217; #9983 is approved.
#9983: approve. The keyboard height reads the whole keyboard while Safari pans to a focused field, restoring spec:AST-099 FR2.
#8217: request changes. The rewritten FR10, DEC-1 and spec:AST-094 edits carry an engineering owner's approval for this head, and straight drags behave as FR10 says, but two fixes remain:
- A drag that starts on a sideways-scrolling strip in the composer and drifts sideways at all is not held, because
canElementScrollcounts any nonzero x travel, so the conversation scrolls under the keyboard (40 px for a 120 px drag up with 8 px of drift in Chromium) against FR10's "MUST NOT scroll content"; count an axis only when its travel is at least half the other's, as the gesture system's scroll probe does, and add a drifting-strip case (transcriptDismiss.ts:188-204). - On
main,spec:AST-094FR19 already reads "five exceptions" after a toast's swipe was added, so this head's composer exception leaves six under "five" once merged; rebase and say "six" in FR19 and in the FR18–FR22 verification row (spec.md:411,:955).
Whole stack: fails on #8217's two fixes; it merges cleanly onto main, where the changed suites and the core typecheck pass. A pull request lands only when it and every pull request below it are approved, and CI has not run on either draft head yet.
[Automated review]
| at != null && at !== composer; | ||
| at = parentAcrossShadow(at) | ||
| ) { | ||
| if (at instanceof Element && canElementScroll(at, dx, dy)) { |
There was a problem hiding this comment.
canElementScroll(at, dx, dy) tests every axis whose delta is nonzero, so a drag up on a strip that scrolls sideways counts as the strip's scroll as soon as it drifts left at all. The page hold then cancels nothing and the conversation scrolls under the keyboard: in Chromium a 120 px drag up with a 20 px drift moved it 129 px. Counting an axis only when its travel is at least half the other's, as createNativeScrollProbe does (gestures/nativeScroll.ts:217-220), and adding a drifting strip case would close it.
| `touchmove` through a non-passive listener on its element; a watcher, | ||
| which never claims, is exempt (DEC-7). A `touchmove` of a touch no recognizer owns MUST NOT be cancelled, with | ||
| four exceptions. While a scroll lock under a modal presentation holds the | ||
| five exceptions. While a scroll lock under a modal presentation holds the |
There was a problem hiding this comment.
On main this line already reads "five exceptions" (a toast's swipe, component:Toast FR4). Merged, FR19 lists six exceptions under "five", and the FR18–FR22 row's "FR19's five exceptions" goes stale too. Rebasing and saying "six" in both places would keep the record consistent.
There was a problem hiding this comment.
Done in 9d66f5f, on a merge of main: FR19 and the FR18–FR22 row now say "six".
|
/approve-spec 9d66f5f |
There was a problem hiding this comment.
Changes requested on #8217; #9983 is approved.
#9983: approve. The keyboard height reads the whole keyboard while Safari pans to a focused field, restoring spec:AST-099 FR2.
#8217: request changes. The record edits carry an engineering owner's approval for this head, and the drifting-strip hold and the FR19 count from the last review are fixed. Two shared-path fixes remain:
- Miss:
bounceComposerskips the bounce only on the system Reduce Motion setting, while Core's Reduced Motion policy (reducedMotionFade, and the animator) also honours a theme's reduced-motion mark, so once a theme can reduce motion for everyone the chat dock would keep bouncing; decide it withreducedMotionFade(dock, {touchRelease: true}), or one shared predicate if that import is too heavy (composerBounce.ts:41-46). scrollsInComposerrestatescreateNativeScrollProbe's half-travel axis rule instead of calling it; export that filter fromgestures/nativeScroll.tsand use it in both places (transcriptDismiss.ts:194-198).
Whole stack: fails on #8217's two fixes; the changed suites pass on this head. A pull request lands only when it and every pull request below it are approved, and CI has not run on either draft head yet.
[Automated review]
| if ( | ||
| win == null || | ||
| typeof dock.animate !== 'function' || | ||
| win.matchMedia?.('(prefers-reduced-motion: reduce)').matches |
There was a problem hiding this comment.
Miss (One shared path): this reads only the system setting. Core's Reduced Motion policy, reducedMotionFade (motion/reducedMotion.ts:88), and the animator (motion/animator.ts:973) also count a theme's reduced-motion mark (design:reduced-motion DR2a), so once a theme can set 'always' the dock keeps bouncing while every other touch animation stops. Suggested: if (reducedMotionFade(dock, {touchRelease: true}) != null) return;, or one exported predicate for the two-part check if that import is too heavy for the size report.
| const x = Math.abs(dx) >= Math.abs(dy) / 2 ? dx : 0; | ||
| const y = Math.abs(dy) >= Math.abs(dx) / 2 ? dy : 0; |
There was a problem hiding this comment.
One shared path: this restates createNativeScrollProbe's half-travel rule (gestures/nativeScroll.ts:219-220). Exporting that axis filter from nativeScroll.ts (for example majorAxes(dx, dy)) and calling it here and in the probe's canScroll keeps one rule.
|
/approve-spec 955564d |
There was a problem hiding this comment.
Changes requested on #8217; #9983 is approved.
#9983: approve. It restores spec:AST-099 FR2: the keyboard height now reads the whole keyboard while Safari pans the page to a focused field, and its tests also pass on today's main.
#8217: request changes. The record changes carry an engineering owner's approval for this head, and one blocking finding remains:
- Regression:
transcriptDismiss.ts:29importsisNodeTarget, whichmainremoved in #10224, so once merged withmainCore's typecheck fails (TS2305) and every touch drag throwsisNodeTarget is not a function; mergemainand useisNodefromutils/nodeType(andisElement/isNodeat lines 202 and 293).
Whole stack: blocked by that fix; a pull request in this stack lands only when it and every pull request below it are approved, and CI has not run on this draft head yet.
[Automated review]
| import {bringsUpKeyboard} from '../keyboard/keyboardLayoutGuide'; | ||
| import { | ||
| getKeyboardHeight, | ||
| isNodeTarget, |
There was a problem hiding this comment.
Regression: main no longer exports isNodeTarget from keyboard/keyboardHeight (#10224 moved the check to isNode in utils/nodeType.ts). Merged with main, this import fails Core's typecheck (TS2305), and nodeOf throws isNodeTarget is not a function on every drag, so nothing dismisses. Suggested, after merging main:
import {isElement, isNode} from '../utils/nodeType';
// nodeOf
return isNode(target) ? target : null;Use isElement(at) at line 202 and isNode(origin) at line 293 as well, so the composer's checks accept another window's nodes, as nodeOf already does.
A drag down the conversation now blurs the field the moment it reaches the composer. The dock container then gives one small bounce on the keyboard's spring, started at the finger's speed, so the composer, the frosted layer and the scroll-to-bottom button move as one. A drag down that starts on the composer dismisses the keyboard too, and while the keyboard is up no drag on the composer scrolls the page. The follow code and its armed cue are gone.
|
/approve-spec 4c00682 |
There was a problem hiding this comment.
Changes requested on #8217; #9983 is approved.
#9983: approve. It restores spec:AST-099 FR2, and its tests also pass on today's main.
#8217: request changes. The record changes carry an engineering owner's approval for this head and the build against today's main is fixed; one blocking finding remains:
- Miss:
transcriptDismiss.ts:105-110(isFinished) and:264restate the gesture system'sisTerminalandisActive(gestures/GestureRecognizer.ts:159-165); call those instead, dropisFinished, and add the import to the gesture-import list the unit test pins.
Whole stack: blocked by that fix; a pull request in this stack lands only when it and every pull request below it are approved, and CI has not run on this draft head yet.
[Automated review]
| /** Whether the pan's drag is over: released, cancelled or failed. */ | ||
| function isFinished(pan: PanGestureRecognizer): boolean { | ||
| return ( | ||
| pan.state === 'ended' || pan.state === 'cancelled' || pan.state === 'failed' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Miss (One shared path): this is isTerminal(state) from gestures/GestureRecognizer.ts:159-161, which Core's internal touch entry re-exports; line 264 is likewise !isActive(pan.state) (:163-165, already imported by useActionGroupTracking.ts). Suggested:
import {isActive, isTerminal} from '../gestures/GestureRecognizer';
// :169 and :255
if (isTerminal(pan.state) || reachesLine) {
// :264
if (!isActive(pan.state)) {Then drop isFinished and add '../gestures/GestureRecognizer' to the import list ChatLayoutTranscriptDismiss.test.tsx pins.
Stack: #9983 → #8217
Merge group: #9983, #8217
Review only this PR's change: vjeux/astryx@86c74e8...1fd9e0e and vjeux/astryx@6fd4c28...4c00682 (a pure merge of main between).
User need
In iOS Messages, dragging the conversation to the field puts the keyboard away, as does a swipe down on the input bar. ChatLayout kept it up.
Problem and solution fit
A watcher pan on the root lets the transcript scroll. With a software keyboard up, the field blurs when a touch or pen drag reaches the composer's top. The dock container then bounces once on
springs.keyboard, from the finger's speed, at most the composer's bottom padding, unless motion is reduced. The composer, frosted layer and scroll-to-bottom button share that transform.A drag down from the composer blurs it too, unless a long press began it (caret loupe). While the keyboard is up, the gesture system's page hold cancels one-finger drags that start on the composer, so nothing scrolls under it. Its own scrollers, a pinch and a zoomed page stay the browser's.
Current specification
component:ChatLayoutFR10 and DEC-1, rewritten.spec:AST-094FR36 and FR37 keep the keyboard with a scoped exception; FR19 lists the composer's hold.Public delta and caller burden
No new prop or type. The follow code goes (
composerFollow.ts, its armed cue, the animator): 9.8 kB gzip less (INV6).Design and alternatives
touch-actionblocks pinch-zoom.Evidence
iOS 26.2 Simulator, "Full AI Chat", HID drags:
Scope
Writing rubric results
component:ChatLayout: R4, DEC-1's two one-line rejections; others none.spec:AST-094FR19, FR36, FR37 and FR43 lines andspec:AST-099paths: R1–R6 none.Testing
Behavior module: ChatLayout transcript dismissal (transcriptDismiss.ts)
ChatLayout's import, bytes (core build, esbuild, minified, React and StyleX external), both on today's main:
The animator, the rubber band and the follow module leave.
transcriptDismiss.test.ts32/32,ChatLayoutTranscriptDismiss.test.tsx23/23,composerBounce.test.ts5/5,dragAxes2/2. The theme-mark case fails on 9d66f5f's source, and two strip cases on d19cf69's. Two cases cover a node made in another window; each fails withinstanceofin place ofisNodeorisElement.isNodeTargetis gone) and its Storybook build. This head passes both.pnpm review:preand the repo checks (sync:exports:check,verify-exports,check:repo,check:token-docs,check:motion-roles,check:changesets,check:knowledge) pass.