Repository navigation
Conversation
…oard from it spec:AST-099 makes core measure the on-screen keyboard in one place, from the visual viewport, in every browser that shrinks it for the keyboard. BottomSheet's own hook measured it by user agent (Mobile Safari only) and put its keyboard room in a CSS custom property of its own. It becomes `keyboard/`, core's one keyboard module, and BottomSheet reads the keyboard from it: - The height (FR2): the layout viewport's height (`utils/layoutViewport.ts`, in layout px under a pinch zoom in every engine, less a classic horizontal scrollbar whose thickness is measured once per document and zoom) minus the visual viewport's height times its scale. A page pan or a pinch zoom never reads as a keyboard; Chrome on Android is measured from its visual viewport like every browser (DEC-4); inside an iframe, without a visual viewport, or under `interactive-widget=resizes-content` it reads 0 (FR6, FR7, FR8). The decision is a capability, never the user agent. - One set of listeners per window, however many subscribers, with one layout read per keyboard change (architecture:react-component-runtime INV11); `useKeyboardHeight` gives a component the height. - Focus (FR4): `refuseFocusReveal` delivers a claimed focus move with `preventScroll` before the browser's own reveal; `watchRevealPan` puts back a browser's own pan to a focused field after the keyboard's report (measured in Firefox for Android, and in Chrome 124 on Android). - BottomSheet: a fully expanded Tall sheet lays its keyboard room out through a functional style on its content box (FR3), reveals the focused field in the keyboard report's own event (FR4), puts back a document scroll while the page is locked and a browser's own pan, and dismisses the keyboard when the sheet starts to move (FR5). The sheet does not ride the keyboard's slide here: that motion comes from the touch module an app installs (TouchSupport), in its own pull request. Core gains the interaction harness as a dev dependency (the line meta#8076 adds too) for its first interaction spec on main, `BottomSheetKeyboardInset.interaction.browser.spec.ts`. Docs: the keyboard module's README, the browser-support guide's section on the on-screen keyboard (BottomSheet), BottomSheet's doc, and two changesets (core: BottomSheet; cli: the guide).
|
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
…8549 and meta#8554 into ChatLayout ChatLayout's keyboard ride now stacks on its dependencies, merged as one base (58f88dc): the keyboard module (meta#8559, on main), the sheet's ride on Gboard's slides (on meta#8559, meta#8076 and meta#8484), the split frame evidence (meta#8548 under meta#8549 and meta#8554), and the rendering trace's invalidations with the compositor-update classifier (meta#8554). Every file those carry takes their version; ChatLayout's parts are put back on top: - `useMobileKeyboard.ts`: the sheet ride's version (its coarse-pointer gate, main's Reduce Motion query), plus `claimTapFocus` and the ChatLayout lines of its header; its tests and `index.ts` likewise. - The keyboard README and the guide's, model test's, rideKeyboard's and layoutViewport's headers: theirs, plus ChatLayout's lines. - `keyboardLayoutGuide.ts` takes the frame-watch fix, and `timing.ts` meta#8484's constants with meta#8076's change. - The harness is meta#8554's, beside meta#8549's and meta#8446's content. - The browser-support guide names ChatLayout's composer beside the sheet; the modules' changesets stay theirs, and ChatLayout's own changeset names the guide (and the CLI).
|
Posted by Cindy's agent on her behalf. A naming change to match |
|
|
…9 does spec:AST-099 describes the value as the keyboard height, and the module measures the on-screen keyboard wherever one appears, tablets too, so "mobile" was narrower than what it does: - `subscribeMobileKeyboard` becomes `subscribeKeyboardHeight`, and its event type `MobileKeyboardEvent` becomes `KeyboardHeightEvent`; - `getMobileKeyboardHeight` becomes `getKeyboardHeight`; - BottomSheet's hook `useMobileKeyboard`, which lays out a Tall sheet's keyboard room and routes its focus, becomes `useSheetKeyboard` (`useKeyboardHeight` is already the hook that gives a component the height), and its options type `UseSheetKeyboardOptions`; - the file `useMobileKeyboard.ts` becomes `keyboardHeight.ts`, with its test. No behavior changes.
…e sheet's Gboard ride meta#8559 renames the keyboard module's names (`subscribeKeyboardHeight`, `getKeyboardHeight`, BottomSheet's `useSheetKeyboard`, the file `keyboardHeight.ts`); this pull request's own files take the new names (the guide, rideKeyboard, the README's ride sections and the tests). meta#8076 adds cixzhang's meta#8056 fold on b4e0adb, with no conflict.
…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.
meta#8176 now sits on the foundation heads (the sheet's ride on meta#8559 39d37ef and meta#8076 62ecda1, and meta#8554 21ea4dc with cixzhang's meta#8446, meta#8549, meta#8548 and meta#8045). A base move: the dismissal's own change applies on it with meta#8559's names (getKeyboardHeight, subscribeKeyboardHeight).
![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 keyboard module follows spec:AST-099 and fixes the Firefox and pinch-zoom cases, but three things need attention before approval.
useKeyboardHeightpassesuseSyncExternalStorea new subscribe function on every render, so a lone consumer re-attaches the module's listeners and measures three times per keyboard change, against INV11's one-layout-read rule; a stable subscribe function, or leaving the hook for its first consumer, fixes it.- At 3,229 authored lines, please split the separable parts (the reveal-pan put-back, the Firefox content-box room,
useKeyboardHeight, the docs section) from the module move, or explain in the description why they cannot land apart. - AST-099's merge gate needs iOS Simulator evidence for this head, and the current recording comes from #8176's earlier head.
[Automated review]
| export function useKeyboardHeight(win?: Window | null): number { | ||
| const target = win ?? (typeof window === 'undefined' ? null : window); | ||
| return useSyncExternalStore( | ||
| onChange => { |
There was a problem hiding this comment.
This subscribe function is recreated on every render, so React unsubscribes and resubscribes useSyncExternalStore each time. With a single consumer the unsubscribe detaches the shared store (lines 203-208) and the new subscribe re-attaches its listeners and measures again (line 150): one keyboard change measures three times, against INV11's "at most one layout read per keyboard change". The INV11 test at keyboardHeight.test.ts:359 passes only because it keeps a second subscriber attached. A useCallback keyed on target (or a module-level subscribe per window) keeps one subscription.
There was a problem hiding this comment.
One subscription matters because a lone reader otherwise detaches the module's listeners and measures again on every keyboard change. Nothing in this pull request renders from useKeyboardHeight, so it moved to #8176, its first consumer, where its subscribe and snapshot functions now stay the same for the component's life (red first: a lone component's keyboard change re-added the listeners; now it reads the layout once). The INV11 tests here read through subscribers only.
BottomSheet reads the keyboard through `subscribeKeyboardHeight` and `getKeyboardHeight`; nothing in this pull request renders from `useKeyboardHeight`. The hook moves to ChatLayout's keyboard ride, its first consumer, where it keeps one subscription across renders. The INV11 tests here read through subscribers only: three readers, each reading the height again, share one layout read per keyboard change; and a keyboard already up is read when the first subscriber attaches. The README's layout-from-the-height section moves with the hook.
meta#8559 moves `useKeyboardHeight` to ChatLayout's ride, its first consumer, and reads the height through subscribers in its INV11 tests. This pull request keeps the keyboard layout guide in those tests (its rider reads the height again; it reads a keyboard already up), and its README's ride section keeps the layout formula the ride example uses.
There was a problem hiding this comment.
Changes requested: the keyboard module follows spec:AST-099, the focus claim now attaches only under a coarse pointer on one shared listener, and Firefox for Android and desktop pinch zoom behave as described, but the pull request's scope still blocks approval.
- At 3,438 authored lines, please land the content-box room and the report-frame reveal with
watchRevealPanas their own pull requests on top of the module move, or show a state where the move alone does worse than the base. - Please leave the store's
focusin/focusoutreports (keyboardHeight.ts:154-173) to their first consumer, since nothing in this pull request reads them.
[Automated review]
| const onFocus = (event: FocusEvent) => { | ||
| emit({ | ||
| type: event.type === 'focusin' ? 'focusin' : 'focusout', | ||
| height: store.height, | ||
| timeStamp: event.timeStamp, | ||
| target: event.target, | ||
| relatedTarget: event.relatedTarget, | ||
| }); | ||
| }; | ||
| viewport?.addEventListener('resize', onViewport); | ||
| viewport?.addEventListener('scroll', onViewport); | ||
| win.addEventListener('resize', onViewport); | ||
| doc.addEventListener('focusin', onFocus, true); | ||
| doc.addEventListener('focusout', onFocus, true); |
There was a problem hiding this comment.
Nothing in this pull request reads these focus reports: useSheetKeyboard returns on every report that is not viewport (line 978) and no test reads one, yet the store adds two capture listeners on document that run on every focus change while a Tall sheet is presented, beside the sheet's own focusin and focusout listeners on its body (lines 1019-1020). Like useKeyboardHeight, which moved to #8176, they can move to the pull request whose code first reads them.
…focus listeners The keyboard store also reported every `focusin` and `focusout`, through two capture listeners on `document` that ran on every focus change while anything subscribed. Nothing in this pull request reads those reports: BottomSheet's hook returned on every report that is not `viewport`, and no test read one. The store now listens to the visual viewport and the window only; the focus reports join the pull request whose code first reads them. Red first: subscribing added two document focus listeners (now none).
|
Without the content-box room, Firefox for Android's last field sat at 649 px under a keyboard top at 377, and without the put-back one show in four stayed panned 214 px. Both are what the module's documented behavior promises against, so the pieces stay together. |
There was a problem hiding this comment.
Changes requested: the keyboard module follows spec:AST-099, the store's unused focus reports are gone, and Firefox for Android and desktop pinch zoom behave as described, but at 3,425 authored lines the pull request still bundles parts that can land on their own. Please land the content-box room (with its Firefox interaction case) and the report-frame reveal with watchRevealPan and its tests as follow-up pull requests on top of the module move, moving the docs' Firefox sentence with the last of them, or show a state where the module move alone does worse than main.
[Automated review]
The only conflict is the CLI's icon-set search golden, which counts matching docs: main's meta#8753 and meta#8759 add two icon docs (367 → 369), and this pull request's on-screen keyboard section adds one. Regenerated at the merged tree with the golden test's --update, as its header says to: the count is 370 and the twenty results shown are main's.
There was a problem hiding this comment.
Changes requested: the keyboard module follows spec:AST-099, and in simulation Firefox's last field now scrolls above the keyboard and a desktop pinch zoom no longer adds keyboard room, but two things block approval.
- At 3,425 authored lines, please land the content-box room (with its Firefox interaction case) and the report-frame reveal with
watchRevealPanand its tests as follow-up pull requests on top of the module move, or show a state where the module move alone does worse thanmain. spec:AST-099's merge gate needs a Firefox for Android emulator or device run, in a top-level document, for the Firefox for Android behavior the browser-support docs and the changeset now promise; the evidence table shows iOS, Chromium 158 and Chrome 124 only.
[Automated review]
| typeof MutationObserver === 'undefined' | ||
| ? null | ||
| : new MutationObserver(records => { | ||
| // Ignore the inset the sheet lays out on the body. Consumer DOM, |
There was a problem hiding this comment.
Non-blocking: the room is now written on the content box (its style attribute and class), not on the body, so those records no longer match this filter, and the sheet's own room write schedules another reveal pass in the next frame (it writes the same value, so it settles). Either update the comment or also skip the content box's own attribute records; its size changes still reach the ResizeObserver.
…page The browser-support guide said focusing a field in a Tall sheet never pans the page. On the Android emulator (Firefox 158, Gboard, a top-level document), 8 of 8 shows rest the focused field 48 px above the keyboard with the page in place, but in 2 of them Firefox panned first and the sheet put the page back 9 and 35 ms later. The guide now says the sheet leaves the page where it was and puts back a pan the browser starts within a couple of frames. The keyboard module's notes said Chromium 158 does not pan; it pans by the keyboard's whole height when the field stays covered (as with the sheet's reveal on the next frame), and never needs to with the reveal in the report's event.
|
Both points, measured on the Android emulator with Gboard in a top-level document (Storybook's iframe.html opened directly):
|
There was a problem hiding this comment.
Changes requested for one mechanical fix: merged with current main, the CLI's icon set search golden is one match short, because main already moved it from 369 to 370 (#8756) and the new On-Screen Keyboard section adds another, so test-node (1/4) fails both experimental-isolation.test.mjs snapshots (expected 370, received 371). Please merge main again and regenerate the no-icon-set goldens with the CLI's update command. Your Firefox for Android emulator run settles the earlier points: it shows the module move alone leaving the field under the keyboard where main kept it visible, and it meets spec:AST-099's emulator gate for the Firefox claims. The description's evidence table still lacks the Firefox for Android row your comment mentions, so please add it there too.
[Automated review]
| "data": { | ||
| "query": "icon set", | ||
| "matchCount": 369, | ||
| "matchCount": 370, |
There was a problem hiding this comment.
Merged with current main this has to read 371: main already moved it to 370 in #8756, and the On-Screen Keyboard section adds one more match, so CI's test-node (1/4) fails this snapshot on the merge, and the .txt twin carries the same count. Merging main and rerunning the CLI's golden update command fixes both.
There was a problem hiding this comment.
Recommended for approval: the keyboard module follows spec:AST-099, the icon-set goldens now read 371 after the merge with main, and CI passes on that merge. Your Firefox for Android emulator run and the desktop before-and-after frames back the behavior the docs describe, and they show why the content-box room and the pan put-back belong with the module move. One optional cleanup: the MutationObserver filter at keyboardHeight.ts:899 still describes the room as written on the body, but it now lives on the content box, so each room change runs one extra reveal pass.
[Automated review]
The no-icon-set search goldens conflicted after main's meta#7732 moved the "icon set" count; with this pull request's keyboard section the merged docs return 370. Regenerated with the test's --update.
There was a problem hiding this comment.
Recommended for approval: the keyboard module follows spec:AST-099, and after the merge with main the icon-set goldens read 370 with every CI check passing. Your Firefox for Android emulator run and the desktop before-and-after frames back the behavior the docs describe, and they show why the content-box room and the pan put-back belong with the module move. One optional cleanup remains: the MutationObserver filter at keyboardHeight.ts:899 still describes the room as written on the body, but it now lives on the content box, so each room change runs one extra reveal pass.
[Automated review]
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.
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.
Review only this PR's change: vjeux/astryx@6162917...c7cf5d2
On main; #8566 and #8176 stack on it.
Why
On a phone, a Tall BottomSheet's last fields sit under the on-screen keyboard. On Android, main leaves them to the browser, which pans the whole page up to the field, by as much as the keyboard's height (312 px in Chrome), so the sheet's top leaves the screen. On desktop, main reads a pinch zoom as a keyboard and adds 448 px of empty space.
What led to it
Part of bringing Astryx's touch behavior to parity with iOS's UIKit, text entry first.
spec:AST-099(current) asks for one keyboard source every component can read, detected from browser capabilities rather than the user agent.Options and tradeoffs
Why this choice
The module measures the keyboard from the viewport sizes. The sheet adds that height as room at the end of its content, where Firefox can scroll it, reveals the field in the keyboard report's own event, and when a browser pans anyway, puts the page back. Together they keep the field above the keyboard with the page in place in Safari, Chrome and Firefox.
Owner decision (FR6): no capability tells a desktop browser from one with an on-screen keyboard; on desktop the module measures 0 and the focus handling attaches only under touch.
How to check it
Evidence
Firefox 158 at this head, 8 shows: the field rests 48 px above the keyboard every time; Firefox started a pan twice and the sheet put it back after 9 and 35 ms (numbers). Known failures from main, fixed elsewhere: slow first loads (#8353, #8471, #8642), CreateRow's announcement (#8640), the non-modal sheet's Escape test (#8647).
Size (INV6, gzip, main → head): BottomSheet JS 34,747 → 37,534 B, CSS 8,079 → 8,081 B.