Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
meta-cla
Bot
added
the
CLA Signed
…eta#8566) and meta#8554 A dependency-only merge. The base (ad271a7) is the sheet's Gboard ride (meta#8566, on meta#8559 39d37ef, meta#8484 and meta#8076 62ecda1) merged with meta#8554 21ea4dc, which carries cixzhang's meta#8446 66c651f, meta#8549, meta#8548 and meta#8045 at their current heads, so no harness file is resolved here. ChatLayout's own change (this branch's commits since 58f88dc) applies on it with meta#8559's names (`subscribeKeyboardHeight`, `getKeyboardHeight`, `keyboardHeight.ts`).
meta#8176 now stacks on the keyboard module (meta#8559), the sheet's Gboard ride (meta#8566), meta#8549 and meta#8554, and its ride attaches only under a coarse pointer. One conflict, in `useChatKeyboard.ts`: the touch presentation is read once, before both effects (this branch's touch listeners and meta#8176's gated guide subscription), and the transcript's effect keeps its touch listeners beside the gated ride.
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested. The BottomSheet keyboard ride itself follows spec:AST-099 FR8, and in Chromium a Tall sheet's field stays above the keyboard through the hide, but this branch's diff against main is 25,517 authored lines, about 22,700 of them from the open PRs it stacks on (#8559, #8076, #8484 and their own dependencies, including the gesture layer, the interaction harness and an astryxStylex change). Please land those first, or rebase so the diff against main is only this PR's own commit (about 2,800 lines), and re-measure BottomSheet's bundle size against main at that point.
[Automated review]
…ss renders; the base moves to the foundation heads The base (3f66f76) is the sheet's Gboard ride (meta#8566, on meta#8559 6be79f4, meta#8484 and meta#8076 62ecda1) merged with meta#8554 e4f3c8d, on cixzhang's meta#8446 87e516b, meta#8549, meta#8548 and meta#8045, with no conflict. meta#8559 moved `useKeyboardHeight` here, to its first consumer: ChatLayout lays its dock and transcript padding out from it. Its subscribe and snapshot functions are now stable for the component's life (per window), so React keeps one subscription across renders; before, every render unsubscribed and resubscribed, and a lone reader detached the module's listeners and measured again on each keyboard change (INV11). Red first: "reads the layout once per keyboard change for a lone component" expected no listener re-added and one read, and got two `addEventListener` calls. The hook's tests (one re-render per change, a keyboard already up, StrictMode's replay leaving no listener) and the README's layout example come with it.
…aims an axis only in its edge zone; the promotion's style read in its own task; the planner's DEC-11 line) into the presented start
The isEnabled sync and a replaced node's detach silenced every listener for the arena pass they start (react-component-runtime INV2), so a single tap waiting on a double tap they stopped recognized in that pass and was never reported. Delaying the listener call alone would hand it a recognizer the arena has already reset to possible. GestureArena.deferResolution(body) runs code outside input that stops recognizers without resolving what the stops unblock (the recognitions waiting on them, held touches, resets); a pass a microtask later, at the last event's time, resolves it, or the next input does first. Both hook syncs run inside it, reaching the arena through recognizerInternals: the stopped recognizer's own cancel stays silent, and a recognizer that required it to fail recognizes outside the Effect, where its listener reads the recognized state. The README and the isEnabled option say so.
…rred unmount runs It read will-change right after attach(null), before the unmount's microtask, so it passed while that microtask could still promote the element and leave timers behind. It now awaits the microtask, then checks will-change and that no timer is left. It fails on meta#8521's earlier head (97436b2), before the unmount cleared what the landing rebuilt.
…pseudo-element support
…fore the first open
Selector and MultiSelector present their bottom sheet through a lazy
import (SelectorBottomSheet). The first sheet test in each file waited
for that module load inside its open's `findByRole`, whose budget is
Testing Library's 1,000 ms. The keyboard ride adds the animator, the
guide and the ride to BottomSheet's module graph, so under a test run's
module transform the first open grew 160-300 ms, and on a loaded runner
the load alone crossed the budget; because it kept running, the next
tests in the file failed too until it landed.
Loading the module is not part of an open: each file now waits for it
once in `beforeAll`, and each open waits only for the open. No timeout
is raised. Per open, nothing of the ride runs in these tests, and once
the module is loaded the first open takes the same time with and
without the ride.
Red first: with the sheet's module load held for 1.2 s (a vi.mock that
delays the original module), the first-open tests failed ("Unable to
find role dialog" in Selector and MultiSelector, the 'sheet-open'
scenario in the combobox binding); with the preload they pass. Two full
core runs under load leave none of these files failing.
…the flat tree isFingerDownIn and the pointer-events finger match compare with containsAlongFlatTree, as the scroll watch already does: a finger on content inside an open shadow root is down in the scroller around that root, and its touches, retargeted to the shadow host, match its pointer finger, so a claimed pan cancels its moves there (spec:AST-094 FR19, FR20).
…achments asks, and only then The host let the last attachment decide whether an element is an action group (useGestureRecognizer always passes the option), and never recomputed it at a detach: a plain attachment after the group's turned the group off, and a group's attachment released left a recognizer that never asked taking presses under a fine pointer. Fails until the host counts the attachments that ask.
…ents asks The host counts the attachments that pass actionGroup: true and keeps the element a group while one remains, recomputed at each detach. An attachment that passes false, as useGestureRecognizer does by default, no longer turns a group off, and a group's last asking attachment taking its leave ends the group.
… that content The presented hit test takes the document's elementFromPoint, which answers a shadow host for anything inside its open root, and compares with Node.contains, which stops at the boundary: a recognizer on a control inside a moving sheet's shadow root gets no finger, and the tap's click goes to the host. Fails until the hit test descends into open roots and compares along the flat tree.
Where a moving element is drawn away from its layout box, the host hit-tests what is drawn under the finger with the document's elementFromPoint, which answers a shadow host for anything inside its open root. The hit test now descends through each open root's own elementFromPoint and compares with the movers along the flat tree, so a recognizer on a control inside a moving sheet's shadow root gets its finger and the tap clicks that control.
…nt lets go The host's touch-action rules sit in an adopted sheet or a style element per document or shadow root, and were never removed, so a page whose last recognizer detached kept them, against spec:AST-094 FR24 (nothing touch-related remains after the last instance unmounts). Fails until a root's rules go with its last marked element.
…th its last marked element The touch-action writer records which elements it marked in each document or shadow root, and when the last one is unmarked (its last recognizer detached) it removes that root's adopted sheet or style element, so nothing touch-related remains after the last instance unmounts (spec:AST-094 FR24). A later mark writes the rules again; an element marked in another root first lets go of the old one.
… the inline style it reads - The motion registry, the host's engine gate and the README name the presented hit test as an engine workaround (spec:AST-013 FR6) with its review condition (re-measure on each iOS release; remove it once iOS WebKit hit-tests a compositor-moved element where it is drawn) and its regression fixture. - While a curve runs the animator holds the value it starts from inline, so presentation.ts and the README no longer say the inline style holds the end.
…core/internal/touch Core ships the internal entry @astryxdesign/core/internal/touch, which re-exports the gesture primitives for Astryx's own packages, and the Lab-import guard fails a Lab import of core outside core's exports (architecture:react-component-runtime INV13). The README and the barrel's header named internal workspace imports and said nothing here is exported; they now name that entry.
…ill needs the element, and the listeners that cancel The release note, the stream's header, the host's comment and the README said the stream is one passive listener per event type and that a document-capture stopPropagation no longer starves the recognizers. Such a stop never hid a listener on the same node, and some subscriptions cancel. They now say what changed: an app's window-capture stopPropagation, or a document-capture stopImmediatePropagation registered before core, no longer hides a touch; a recognizer that must claim still needs the touch to reach its element; the gesture host's touchend, click and contextmenu cancel while a claimer is attached and the page hold's touchmove while it holds; and the stream stays ahead of an app's window-capture listener only until a type's listener is re-added. The README's list of blocking touchmove listeners gains the claimed touch's target and the document while a claiming element moves.
…default it moves
The {stylexOptions} form of astryxStylex now passes StyleX
treeshakeCompensation: true unless the caller sets it. spec:AST-017 FR3
asks the release note to name the old and new default and the option
that restores the old one; it now does.
…er and for the two stops the input stream fixes - A still finger with sub-pixel jitter over a scroll view keeps its tap and scrolls nothing (spec:AST-094 FR18–FR22). - A tap is still recognized under an app's window-capture stopPropagation, and under a document-capture stopImmediatePropagation registered before core's: the recognizers hear touches on the window's capture phase. Chromium and WebKit pass.
…s once Where nothing that claims is attached, the host's touchend listener is passive and cannot cancel the engine's own click after a retargeted tap clicks what is drawn. The shared click guard swallows that late click whatever is attached; this case pins it, as asked on meta#8293.
…esture host's subscriptions The gesture host subscribes no click: the one cancelling click subscriber is the click guard's, added at a window's first arm and kept. The release note, the stream's header and the gestures README listed click with the host's touchend and contextmenu; they now name the guard's own.
BottomSheet.test.tsx's imports from both sides are kept; the search goldens are regenerated after the remaining merges.
… guide The keyboard layout guide (this pull request) reads where focus moves to tell a keyboard that comes up from one that stays: meta#8559 removed the store's focus reports because nothing there read them, so they return here with their first reader. One pair of capture listeners on the document, attached while anything subscribes, reports each focusin and focusout with the current height; BottomSheet's reveal still acts on viewport reports only.
Conflicts were in this stack's own keyboard files: the browser-support sentence keeps ChatLayout's composer and takes meta#8559's page wording, BottomSheet's test imports follow the sheetKeyboard split, and the keyboard notes keep both the sheet's and ChatLayout's Chrome 124 findings. The search goldens are regenerated after the merge.
There was a problem hiding this comment.
Changes requested: the BottomSheet keyboard ride follows spec:AST-099 FR8, but this branch's diff against main is now 33,704 authored lines, 30,825 of them from the open PRs it carries (#8360 with #8076, #8559 and #8484). Please land those first, or rebase so the diff against main is only this PR's own change; at 3,013 lines that change still needs a case for its size or a split, for example by moving the Safari half nothing here rides yet to #8176. Please also fix two issues in the own change and, once it stands alone, report BottomSheet's size for it with a justification for the keyboard guide's touch-only share, as architecture:component-style-authoring INV6 asks:
- Moving focus from a field to a
<select>in a Tall sheet still drops the scroll room at Chrome's hide report, because the guide counts a select as bringing up a keyboard (keyboardLayoutGuide.ts:129-130). - The keyboard store's new
focusinandfocusoutlisteners ondocumentattach for every Tall sheet, fine pointers included, againstspec:AST-094FR24 (keyboardHeight.ts:165-178).
[Automated review]
| case 'SELECT': | ||
| return !(element as HTMLSelectElement).disabled; |
There was a problem hiding this comment.
In Chrome on Android a <select> opens Chrome's own picker and Gboard slides down, but counting it here means a blur from a field to a select predicts no hide (:289-294) and Chrome's hide report is held as a focus handoff (:253-268). The sheet, holding no hide, then releases its room at that report (keyboardHeight.ts:886-901), the one-step drop spec:AST-099 FR8 rules out. Counting a select as keyless in Chrome's profile would let the blur predict the hide the sheet rides.
There was a problem hiding this comment.
Fixed in 0fbae99: under Chrome's profile a select no longer counts as keeping an input view, so moving focus from a field to a select predicts the hide and the sheet rides its room down on Gboard's slide. Safari's picker still keeps it. A case for a field-to-select move fails before the change.
| doc.addEventListener('focusin', onFocus, true); | ||
| doc.addEventListener('focusout', onFocus, true); |
There was a problem hiding this comment.
These attach for every subscriber, and a Tall sheet subscribes on every pointer (:1140), so a fine-pointer sheet now keeps two document listeners that only the guide reads, although the guide never subscribes there (:669-673). spec:AST-094 FR24 lets a fine-pointer component add document listeners only during a press, as :704-707 does for the blur listener; attaching this pair only while a focus-reading subscriber exists would keep that.
There was a problem hiding this comment.
Fixed in 658638d: the store's document focus listeners now exist only while a subscriber asks for focus moves ({focus: true}). Only the keyboard layout guide does, so a Tall sheet adds none under any pointer. A case for a viewport-only subscriber fails before the change.
…asks for them
The store's document focus listeners attached for every subscriber, so
every Tall BottomSheet added them, under a fine pointer too (spec:AST-094
FR24). A subscriber now asks for focus moves with `{focus: true}`; only the
keyboard layout guide does, and the document's focusin and focusout
listeners exist only while such a subscriber does. Viewport reports reach
every subscriber as before.
Red first: a viewport-only subscriber added the two document listeners.
…'s hide The guide counted a select as bringing up a keyboard, as its picker does on iOS, so in Chrome it held the hide Chrome reports when focus moved from a field to a select, waiting for a keyboard that never comes back (Chrome on Android opens the select's list over the page), and a Tall sheet dropped its scroll room at the report. Under Chrome's profile a select no longer keeps an input view: moving focus to one predicts the hide and the sheet rides its room down on Gboard's slide. Safari's picker is unchanged. Red first: the guide published no hide at Chrome's report.
There was a problem hiding this comment.
Changes requested: the BottomSheet keyboard ride follows spec:AST-099 FR8, and this head fixes the select and fine-pointer listener issues, but the branch's diff against main is still 33,787 authored lines, 30,825 of them the content of open PRs #8360 (with #8076), #8559 and #8484.
- Land those PRs first, or rebase so the diff against
mainis this PR's own change (3,116 lines), then make that change's case for going over 2,000 lines or split it, for example by moving the Safari half to #8176. - In Chrome's profile, let a blur from a field to a native
date,time,datetime-local,monthorweekinput predict the hide as a blur to a select now does (packages/core/src/keyboard/keyboardLayoutGuide.ts:244-249), and add one of them to the guide's and the sheet's Chrome tests. - Report BottomSheet's size change for the own change and justify the keyboard guide's touch-only bytes under
architecture:component-style-authoringINV6.
[Automated review]
| const keepsInputView = (target: EventTarget | null): boolean => | ||
| bringsUpKeyboard(target) && | ||
| !( | ||
| profile === 'chromium' && | ||
| (target as Element | null)?.tagName === 'SELECT' | ||
| ); |
There was a problem hiding this comment.
In Chrome on Android a native date, time, datetime-local, month or week input also opens its picker over the page while Gboard slides down, but this exclusion names only SELECT, and bringsUpKeyboard counts those inputs as keyboard fields. A blur from a field to one therefore predicts no hide, Chrome's report is held as a focus handoff, and the sheet drops its room at that report, the FR8 jump the select case just fixed. Letting keepsInputView also refuse those input types in Chrome's profile, with one of them in the guide's and the sheet's Chrome tests, would close it.
There was a problem hiding this comment.
Fixed in 74a6c65: under Chrome's profile a date, time, datetime-local, month or week field now counts as opening a picker over the page, like a select, so moving focus to one predicts the hide and the sheet rides its room down; the guide's Chrome tests cover a date and a time field and the sheet's cover a select and a date field, and the date and time cases fail before the change. On the Android emulator, Chromium 158 and Chrome 124 opened each picker as a dialog over the page with the keyboard hidden: stills.
…the keyboard's hide Chrome on Android edits a date, time, datetime-local, month or week field in a picker over the page and Gboard slides down, as it does for a select, but the guide still counted those fields as keeping an input view, so it held Chrome's hide and a Tall sheet dropped its scroll room at the report. Under Chrome's profile they now count as opening a picker over the page, like a select: moving focus to one predicts the hide and the sheet rides its room down. Safari's pickers, which take the keyboard's place, are unchanged. Measured on the Android 15 emulator with Gboard, a text field focused and then a date field, a select and a time field: in Chromium 158 and Chrome 124 each opened its picker as a dialog over the page and the keyboard went away (the IME reported hidden; the visual viewport returned to the layout viewport's height 76 to 447 ms after the focus moved). Red first: the guide's date and time cases and the sheet's date case failed before the change; the select cases already passed.
There was a problem hiding this comment.
Changes requested: the BottomSheet keyboard ride follows spec:AST-099 FR8 and this head closes the date and time input gap, but the branch's diff against main is still 33,871 authored lines, 30,825 of them the content of open PRs #8360 (with #8076), #8559 and #8484.
- Land those PRs first, or rebase so the diff against
mainis this PR's own change (3,200 lines), then make that change's case for going over 2,000 lines or split it, for example by moving the Safari half to #8176. - Report BottomSheet's size change for the own change and justify, under
architecture:component-style-authoringINV6, the keyboard guide's touch-only bytes outside the shared touch modules (about 2.5 KB gzip).
[Automated review]
Review only this PR's change: vjeux/astryx@ae028f4...55e954f, plus vjeux/astryx@23db8c8...74a6c65 (the keyboard store's focus reports return with their first reader, the keyboard layout guide, and are heard only while it asks; in Chrome, moving focus to a select or a date or time field rides the keyboard's hide; the merges between them add nothing of their own).
Stacked on #8559 (the keyboard module), #8360 (the animator, carrying #8076) and #8484 (the keyboard's timing); #8176 stacks on it.
Why
In Chrome on Android, a tall BottomSheet's last field trails Gboard up by about half a second, and when the keyboard goes down the sheet's keyboard room drops 295 px in one frame while Gboard is still sliding.
What led to it
Part of bringing Astryx's touch behavior to parity with iOS's UIKit, text entry first. #8559 makes room for the keyboard by following Chrome's reports, which arrive just after Gboard starts its show and in the middle of its hide. The engineering owner decided Android should get the same smooth keyboard motion as iPhone (
spec:AST-099DEC-2, FR8).Options and tradeoffs
Why this choice
The keyboard layout guide dates each of Chrome's reports back to the start of Gboard's slide and publishes Gboard's measured curve, and the sheet's content rides that curve on the compositor. During the hide the room stays until the keyboard is down, then goes with no jump. With Reduce Motion the content fades instead of sliding. Under a mouse, and in Safari and Firefox, none of this attaches.
How to check it
Evidence
Unit and browser tests (Chromium, WebKit) check the show and hide against Gboard's curves, Reduce Motion, and that no keyboard tracking starts under a mouse or on Safari's schedule. Known failures from main, fixed elsewhere: CreateRow's announcement (#8640) and the non-modal sheet's Escape test (#8647).
Size (INV6, gzip, main → head): BottomSheet JS 34,771 → 50,240 B, CSS 8,079 → 8,081 B.