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
github-actions
Bot
added
the
needs:spec-owner-review
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
![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 engineering owner's decision on the on-screen keyboard spec (spec:AST-099) for this head, and on the fixes below.
Spec decision needed. This head adds DEC-5 and an FR8 exception to AST-099, and the docked ride and the page-content reveal rely on FR9 and DEC-5 from #9750, which is not on main (its DEC-5 is also a different decision from this head's). An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner; otherwise this pull request waits for #8559 and #9750 to land, with one DEC-5 renumbered.
Blocking findings:
followKeyboard(target, {move: 'reveal'})claims focus moves withpreventScrollbut reveals only on a viewport report, so Next or Tab with the keyboard up leaves the next field under the keyboard (keyboardTarget.ts:249-275, against AST-099 FR4).keepFieldAboveKeyboardrefuses the browser's reveal and has no fallback when its scroller cannot scroll far enough, so the field stays covered or the browser pans the page (keyboardTarget.ts:180-202,:386-397).- A
'ride'target lifts on every keyboard report even when focus is outside it, so a docked bar covers the page field placed 17 px above the keyboard (keyboardTarget.ts:157-176). caretRangeAtandplaceTextControlCaret(keyboardHeight.ts:695-735) copycaretAtinutils/selectionLock.ts:322-355, anddrawnOffsetOfcopiesrideOffset(useMobileKeyboard.ts:251-258); share one of each.- The description and
keyboard/README.md:67cite AST-099 FR9, which the record does not contain at this head, and the title promises "any field" while no field adopts the module yet; reword both. - The target attachment's key omits the touch query (
useMobileKeyboard.ts:209), so docking a mouse never drops the tap route and focus claim. - AST-099's merge gate, simulator and emulator evidence (DEC-3), is not met for the BottomSheet ride this head carries.
- At 7,824 authored lines, 6,275 of them from #8559, please land #8559 first so this pull request is its own 1,557 lines.
[Automated review]
| const stopMoving: (isFinal?: boolean) => void = | ||
| move === 'ride' | ||
| ? rideWithKeyboard(target, () => focusedFieldIn(target), offset, win) | ||
| : subscribeKeyboardHeight(win, event => { |
There was a problem hiding this comment.
This reveals only on a viewport report, but the claim above already delivered the focus move with preventScroll. With the keyboard up, Next or Tab into a covered field brings no report, so the field stays under the keyboard (AST-099 FR4). Reveal the claimed field when focus arrives, as keepFieldAboveKeyboard does at line 417.
| return; | ||
| } | ||
| const doc = field.ownerDocument; | ||
| if (scroller === (doc.scrollingElement ?? doc.documentElement)) { |
There was a problem hiding this comment.
Nothing checks how far the scroller actually moved. When it cannot scroll the whole distance, the browser's own reveal has already been refused, so the field stays covered (Safari) or the browser pans the page by the keyboard's height (Chrome, Firefox). The sheet keeps the browser's reveal for a field it cannot bring up; this path needs the same fallback, or room.
| offset: number, | ||
| win: Window, | ||
| ): number { | ||
| const targetCovered = coveredBy( |
There was a problem hiding this comment.
The target's own bottom edge always counts, so a 'ride' target lifts on every keyboard report even when the focused field is outside it, and the risen bar covers a page field revealed 17 px above the keyboard. Lift only while focusedFieldIn(target) is set.
| }; | ||
| } | ||
|
|
||
| function caretRangeAt(doc: Document, x: number, y: number): Range | null { |
There was a problem hiding this comment.
utils/selectionLock.ts already resolves a caret from a point (caretAt, lines 322-355), including shadow roots. Export it or move it to a shared utility and call it here, so one fix reaches both callers.
| ? keepFieldAboveKeyboard(target, {offset}) | ||
| : followKeyboard(target, {move, offset}); | ||
| }, | ||
| `${isEnabled}|${move}|${reservesInset}|${offset}`, |
There was a problem hiding this comment.
This key leaves out the pointer, unlike the sheet path's key at line 883, and followsKeyboard reads the coarse pointer only once at attach. A tablet that docks a mouse keeps the tap route and the focus claim. Add isTouch to the key.
| a surface watches. BottomSheet's hook watches while its sheet is presented | ||
| under a coarse primary pointer; under a fine one it registers no touch | ||
| listener. | ||
| - **Keyboard targets (FR9, `keyboardTarget.ts`):** a target keeps the |
There was a problem hiding this comment.
AST-099 has no FR9 at this head; it is proposed in #9750. Cite it as proposed until that record change lands.
vjeux
changed the title
|
Thanks. Each point, at 3ca29c4:
|
There was a problem hiding this comment.
Changes requested. You chose to land #8559 and #9750 first and restack; these items block this head or remain for the restacked one:
- This head carries all of #8559 (8,123 authored lines in all, 6,303 of them #8559's commit), so it needs that restack before review can approve it.
- #8559's
spec:AST-099DEC-5 and its FR8 exception have no engineering-owner approval, and the docked ride and the page-content reveal rest on FR9, which exists only in #9750. - AST-099 makes emulator evidence with Gboard the merge gate, and this head carries #8559's Chrome ride and Android focus takeover without it.
- When focus moves between two fields in one docked bar, detaching the first field's path runs its final stop and sets the bar back to 0 while the second field is focused and the keyboard is up (
keyboardTarget.ts:480-491). - A
field.focus()with nothing focused raises no blur and no touch, so the field path never claims it and the browser's reveal runs (keyboardTarget.ts:446-449), against "the page never pans". .changeset/keyboard-targets-docked-ride.mdsays the module keeps a field above the keyboard "wherever it sits", but a field no scroller can bring up stays covered.
The reveal, ride, pointer, citation and duplicated-helper issues raised earlier are fixed at this head, and the keyboard and BottomSheet suites pass.
[Automated review]
| field.removeEventListener('focus', onFocus); | ||
| field.removeEventListener('blur', onBlur); | ||
| stopSession?.(true); | ||
| stoppedSession?.(true); |
There was a problem hiding this comment.
If focus already moved to another field in the same docked bar, that field's ride took the bar over (waitingRides, line 353), but this final stop still runs animator.set({y: 0}) (line 410). A probe with two inputs in one fixed bar: the bar sits at -307 with the second field focused, then drops to 0 when the first field's path detaches, under the keyboard. Reset the bar only while this ride still owns it, for example by dropping stoppedSession when another ride takes the target.
| if (!followsKeyboard(win)) { | ||
| return () => {}; | ||
| } | ||
| const stopClaim = refuseFocusReveal(doc, { |
There was a problem hiding this comment.
This claim runs on a blur, and the tap route covers a first tap, but a field.focus() while nothing has focus raises neither, so the browser's own focus runs with its reveal (a probe sees the call arrive with no preventScroll). In Safari a tapped button takes no focus, so a Reply button that focuses a composer is this case. Park focus that leaves for nothing, as the sheet's claim does, or put the pan back with watchRevealPan; otherwise narrow "The page itself never pans" (line 429).
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [chore] Internal: Core's keyboard module keeps a field above the on-screen keyboard wherever it sits: a field in scrolling content is scrolled into place by its nearest scroller, a field in a container fixed at the viewport's bottom rides that container on the keyboard's slide, and a first tap on a field is focused without the browser's reveal. Core's fields adopt it next; apps import nothing new. |
There was a problem hiding this comment.
"wherever it sits" is broader than the code: when no scroller can bring a field up, as on a page no taller than the viewport, the field stays covered (reveal, keyboardTarget.ts:221-224). Suggest "keeps a field above the on-screen keyboard where its scrollers can bring it up".
| // even though the touchend was cancelable and the route cancelled it. A | ||
| // quick tap gets none, and a non-passive listener from the touch's start | ||
| // changes nothing. The events go to whatever is under the finger by then. | ||
| // Once the composer starts down with the keyboard, that is the transcript, |
There was a problem hiding this comment.
Non-blocking (INV29): this module has no composer or transcript. Keep only the reason, for example: "The events go to whatever is under the finger by then, which can take the focus the tap delivered."
|
/approve-spec 1260250 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Changes requested: one blocking issue remains in the new field path. The spec side is settled: FR9 and DEC-5 are on main, and this head's AST-099 edits carry an engineering owner's /approve-spec.
- Newly live check:
nearestScrollerfalls back to the document even when it has nothing to scroll (packages/core/src/keyboard/keyboardTarget.ts:95), soplacementOfclaims every page field's focus and first tap withpreventScroll(:806-808,:846-855) and the field stays under the keyboard whilewatchPansputs back the browser's own pan, against AST-099 Gap 11 ("an element that has nothing to scroll does not count as a scroller (the field keeps the browser's own reveal)"); count the document only when it has scroll range, letplacementOfreturn null otherwise, and add a test for a page without range.
The earlier issues (the shared docked bar dropping on detach, the unclaimed script focus, the changeset wording) are fixed at this head, the size is covered by the behavior-module declaration, and the keyboard and BottomSheet suites pass.
[Automated review]
| return node; | ||
| } | ||
| } | ||
| return doc.scrollingElement ?? doc.documentElement; |
There was a problem hiding this comment.
This falls back to the document even when it has nothing to scroll, so placementOf claims the field's focus and first tap and the reveal cannot move it: the field stays under the keyboard, and watchPans puts back the browser's own pan. AST-099 Gap 11 says an element with nothing to scroll does not count as a scroller and the field keeps the browser's reveal. Count the document only when its scrollHeight exceeds its clientHeight, as the ancestor check above does, and let placementOf return null otherwise.
There was a problem hiding this comment.
Fixed in 73ca152: placementOf counts the document only when it has scroll range (scrollHeight > clientHeight), so a page with nothing to scroll keeps the browser's own reveal, as Gap 11 says. A new case covers a page without range, and it fails on the previous head.
|
/approve-spec 73ca152 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet.
The keyboard targets now leave a page with nothing to scroll to the browser's own reveal, which resolves the last blocking issue, and the spec:AST-099 edits carry an engineering owner's approval for this head.
Next:
- Mark the pull request ready for review; its branch is in a fork, so a maintainer adds the
ci:runlabel so CI runs. - This branch carries #8559's commit, so it lands together with or after #8559.
Non-blocking:
// Nothing to move.inkeyboardTarget.tsnarrates an empty branch; invert the condition instead.nearestScrollerandscrollerWithinrepeat Outline's private scroller walk inuseScrollSpy.ts; one shared helper could serve all three.
[Automated review]
| // target the ride never moved gets no write at all. | ||
| const y = lift > 0 ? -lift : 0; | ||
| if (y === 0 && animator.get('y') === 0) { | ||
| // Nothing to move. |
There was a problem hiding this comment.
Non-blocking (INV29): this comment narrates an empty branch. Invert the condition so the branch is not needed, for example if (y !== 0 || animator.get('y') !== 0) { ... }.
…and the tap route `useMobileKeyboard` gains the targets that hold no inset, and a field's own path, from `keyboard/keyboardTarget.ts`: - `move: 'ride'` without `inset` moves a Core host's own element with the keyboard: on the guide's slide where the browser publishes one (`rideKeyboard`), and at each report elsewhere, clearing the keyboard with its focused field `offset` above it. - `move: 'reveal'` scrolls the focused field into place in the target, a scroller, in the keyboard report's own event, and at once when focus moves to another field with the keyboard up. - Without `move`, the target is a field. When it gains focus it decides where it is from its nearest fixed-position ancestor (`fixedContainerOf`). Docked at the viewport's bottom edge by its layout, the container moves up only by the part of it the keyboard still covers; one taller than the room above the keyboard never rides its whole overlap: its scroller there reveals the field, and with none it rides only as far as the field needs. In a fixed container that fills the viewport, the field's scroller there reveals it. In scrolling content its nearest scroller, the document included, brings it into view on both edges, 10 px above the keyboard while one shows, and nothing else moves. Inside a keyboard target, a container that fills the viewport with no scroller around the field, one that floats above the bottom edge, or a docked bar that sets its own `translate` (the ride would replace it), it does nothing, and the browser's own reveal stays there. - A ride places its target on the frame after each report, slide or focus move, once every listener of that event has run, and measures what the keyboard still covers after any move the app makes itself: a bar the app already moves with the keyboard is not moved, one it lifts part of the way moves the rest, and one it is moving itself at that moment (a running CSS transition or animation on its position) is left to it. The ride writes the CSS `translate`, which adds to an app's `transform`; it writes nothing on an element it never moved, and on an app's element only while it stays docked. A target lifts only while a field inside it has focus, and one ride moves a target at a time. - `distanceIntoBand` (`keyboard/revealBand.ts`) is the band geometry the field's reveal and the Tall sheet's `scrollControlIntoSafeArea` share. - Where the path places a field, focus moves into it (`refuseFocusReveal`) and its first tap (`claimTapFocus`, which moves here from ChatLayout's work with `focusAtPoint`) arrive without the browser's reveal. A claimed tap raises the tap's click on the field afterwards, a synthetic touch `PointerEvent` with the tap's coordinates (a `MouseEvent` where none exists), so a caller's `onClick` still runs once. A pan the browser makes on its own after a report, as after a script's `focus()` no claim saw, is put back (`watchRevealPan`). The attachment follows the primary pointer, so a docked mouse drops all of it. - Every target registers, and BottomSheet's inset path registers its panel, so a field inside one moves nothing twice. - In Chrome on Android, a Tall sheet holds its keyboard layout past Gboard's hide until Chrome reports the keyboard gone. During a hide it predicted from a blur, the keyboard layout guide lets a late report of the keyboard partway down, or still up, start nothing. So a busy page no longer moves the content back up before it settles. Focus that comes back still rides it up. Focus in a frame, an open shadow host or a custom element leaves the viewport to decide. - `focusAtPoint` reads the caret through `caretAt` (utils/selectionLock), the sheet's ride offset through `drawnOffsetOf`, and the tap route hands Safari's late mouse events to the shared click guard (`armClickGuard`). The measure on focus, the preventScroll claim and the ride come from the Lab SearchField's bottom placement, which this replaces.
github-actions
Bot
added
the
needs:spec-owner-review
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision for this head, and one item must be fixed either way.
This head narrows spec:AST-099 FR3's Tall-sheet hide-ride sentence and its verification row to Safari on iOS, and adds Gap 12 for Firefox for Android, after the last spec approval, which named the previous head; the code still runs that ride in Firefox for Android. An engineering owner settles it by commenting /approve-spec 30cd9afb239c4ffddff77b75481238936052a40e (the author may do so as an engineering owner), or by asking for the ride to be gated out of Firefox for Android.
- Attach Android emulator runs at the final head, as AST-099's platform support requires before merge: Chrome on Android with Gboard (animations at normal scale and removed, plus a slow-device profile) for the new late-report hold in
keyboardLayoutGuide.ts, and Firefox for Android for the hide path it now takes, whose only run is from an earlier head.
The pull request's size is not a blocker here, and CI has not run on this draft head.
[Automated review]
| }, settled - time); | ||
| return null; | ||
| } | ||
| if (height > 0.5 && awaitsHide()) { |
There was a problem hiding this comment.
Blocking (evidence): this hold changes how every fully expanded Tall sheet in Chrome on Android leaves the keyboard on a slow page, and AST-099's platform support makes Gboard emulator evidence at the exact head (animations at normal scale and removed, plus a slow-device profile) a merge gate. Only unit tests with modelled report times cover it. Please attach that run at the final head, together with a Firefox for Android run of a blur in a Tall sheet with the keyboard up, since useMobileKeyboard.ts still runs the unguided hide there.
There was a problem hiding this comment.
Both runs are booked at 30cd9af: Chrome on Android with Gboard (animations at normal scale and removed, plus the slow-device profile) and Firefox for Android (a blur in a Tall sheet with the keyboard up). Their results will be attached here.
| return ride == null || ride.get('y') === 0; | ||
| } | ||
|
|
||
| // The properties that move a bar on the page. |
There was a problem hiding this comment.
Non-blocking (comments): this restates POSITION_PROPERTIES. Drop it, or keep only a reason, for example: "An inset change moves a fixed bar like a transform does."
There was a problem hiding this comment.
Agreed; this comment change rides with the next push of this pull request, if one becomes necessary, so it does not move the head that the platform evidence and the spec approval are bound to.
| return dock === 'docked' ? {docked: node} : dock; | ||
| } | ||
|
|
||
| // The nearest fixed-position ancestor of `element`, or null. |
There was a problem hiding this comment.
Non-blocking (comments): this restates nearestFixedAncestor, and line 258 restates isDockedContainer; delete both. At line 342 keep only the reason: "A smooth scroll would leave the field covered when Chrome checks it."
There was a problem hiding this comment.
Agreed; this comment change rides with the next push of this pull request, if one becomes necessary, so it does not move the head that the platform evidence and the spec approval are bound to.
|
/approve-spec 30cd9af |
github-actions
Bot
removed
the
needs:spec-owner-review
Comment-only: 18 sentences over 25 words in the keyboard module's new comments are split into shorter ones with the same facts. No code line changes.
|
/approve-spec f8843e6 |
github-actions
Bot
added
needs:spec-owner-review
There was a problem hiding this comment.
Changes requested: four items block merge. The spec:AST-099 edits (FR3's Tall-sheet hide-ride sentence, its verification row and Gap 12) are owner-approved for this head.
- Miss: in Safari on iOS (and Firefox for Android), moving focus from a Tall sheet's field into a web component's input or an iframe starts the hide ride (
packages/core/src/keyboard/useMobileKeyboard.ts:534-540) while that inner field keeps the keyboard up, so the content drops under the keyboard and stays there until it closes; share the guide's frame and shadow-host check (hidesFieldinkeyboardLayoutGuide.ts) with this gate and add a shadow-host and a frame case. - Attach the Android emulator runs that AST-099's platform support requires at the final head (Chrome and Firefox for Android with Gboard, each at normal and removed animation scale and on a slow-device profile), because the runs in the description predate this head's merge of
mainand Firefox has no removed-animation or slow-device take. - Regression:
packages/core/src/utils/scrollingAncestor.tsredeclares the scrollable-overflow set thatutils/scrollView.tsexports asSCROLLABLE_OVERFLOW, so import it, and share its overflow test, to keep one definition of what scrolls. - Newly live check: in
keyboardTarget.ts,place()writes the container's translate and then reads the field's and the scroller's boxes in its too-tall branch (:682-688), whicharchitecture:react-component-runtimeINV18 rules out in a keyboard-report path; compute the reveal before the write, offset by the container's move.
The size is not a blocker under the behavior-module exemption, and CI has not run on this draft head.
[Automated review]
| * @position Shared by the keyboard module's reveals and Outline's scroll spy. | ||
| */ | ||
|
|
||
| const SCROLLING_OVERFLOW = new Set(['auto', 'scroll', 'overlay']); |
There was a problem hiding this comment.
Regression (one shared path): utils/scrollView.ts, merged from main, exports SCROLLABLE_OVERFLOW as the one definition of the overflow values that scroll, shared by press feedback and the gesture probe. Import it here instead of redeclaring the set. The two helpers already differ on overflow: scrollView.ts counts a box only past 1 px (overflows), and line 33 counts any overflow.
There was a problem hiding this comment.
Done in 8b9fa9a: scrollingAncestor imports SCROLLABLE_OVERFLOW from utils/scrollView.ts (the same three values), so one definition says which overflow values scroll. The 1 px threshold stays as it is: scrollingAncestor keeps what Outline's getScrollableAncestor did on main (scrollHeight > clientHeight), so a threshold would be a behavior change for its own pull request.
| // How the field path places `field` when it gains focus: a ride of its docked | ||
| // container, or a reveal by its nearest scroller. A fixed container the app | ||
| // positions fills the viewport, floats above the bottom edge, or sets its own | ||
| // `translate`. In one, that is a scroller between the field and the container. | ||
| // Null where the browser's own reveal stays. That is inside a keyboard target, | ||
| // which places the field itself, or in such a container with no scroller around | ||
| // the field. |
There was a problem hiding this comment.
Non-blocking (comments): the split leaves vague references ("In one, that is a scroller…", "That is inside a keyboard target…"). Suggest: "The placement of field when it gains focus. A docked container rides. Page content, or a fixed container the app positions, reveals the field through a scroller. Null keeps the browser's reveal: inside a keyboard target, or with no such scroller."
scrollingAncestor imports utils/scrollView's SCROLLABLE_OVERFLOW instead of its own copy of the same three values, so one definition says which overflow values scroll. placementOf's comment is reworded. No behavior change.
|
/approve-spec 8b9fa9a |
There was a problem hiding this comment.
Changes requested: three items block merge. The spec:AST-099 edits (FR3's Tall-sheet hide-ride sentence, its verification row and Gap 12) are owner-approved for this head, and the field targets match FR9, DEC-5 and INV11.
- Miss: in Safari on iOS (and Firefox for Android, which takes the same path), moving focus from a Tall sheet's field into a web component's input or a frame starts the hide ride (
packages/core/src/keyboard/useMobileKeyboard.ts:534-540) while the inner field keeps the keyboard up, so the content drops 248 px under the keyboard and stays there; share the guide'shidesFieldcheck (frames, open shadow hosts, custom elements) with this gate and withonReveal, and add a shadow-host and a frame case. - Attach the Android emulator runs that AST-099's platform support requires at the final head (Chrome and Firefox for Android, each with animations at normal scale and removed and on a slow-device profile), because the runs in the description come from an earlier head.
- Newly live check:
place()inpackages/core/src/keyboard/keyboardTarget.tswrites the container's translate and then reads the field's and scroller's boxes (:678-688), whicharchitecture:react-component-runtimeINV18 rules out in a keyboard-report path; compute the reveal before the write, offset by the container's move.
The size is not a blocker under the behavior-module exemption, and CI has not run on this draft head.
[Automated review]
| if ( | ||
| isExpandedRef.current && | ||
| hasKeyboardLayoutRef.current && | ||
| !bringsUpKeyboard(document.activeElement) |
There was a problem hiding this comment.
Blocking (Miss): focus moving into a web component's input (an open shadow host) or an iframe leaves document.activeElement on the host or frame, which bringsUpKeyboard rejects, so this starts the hide ride while the inner field keeps the keyboard up. No report comes, onReveal never releases the hold, and the content stays drawn 248 px lower under the keyboard until it closes. Share the guide's hidesField check (keyboardLayoutGuide.ts:101-110) with this gate, as awaitsHide does, and add a shadow-host and a frame case beside the select case.
There was a problem hiding this comment.
Fixed in 2a3b205: the gate that starts the ride and onReveal's turnaround both use the guide's hidesField beside bringsUpKeyboard, so focus moving into a frame, an open shadow host or a custom element starts no ride, and mid-ride it rides the content back up. New shadow-host and frame cases sit beside the select case and the refocus case; they fail on 8b9fa9a.
| animator.set({y}); | ||
| } else { | ||
| rideKeyboard(animator, {y}, transition); | ||
| } | ||
| } | ||
| if (inside != null) { | ||
| reveal(inside.field, inside.scroller, height, offset, win); |
There was a problem hiding this comment.
Blocking (Newly live check, INV18): when a report leaves a ridden container too tall with a scroller around its field, this writes the container's translate (:682 or :684) and reveal then reads the field's and the scroller's boxes (:688), so a read follows a write in a keyboard-resize path. Compute the reveal distance before the write and offset it by the container's move (y - animator.get('y')).
There was a problem hiding this comment.
Fixed in 2a3b205: place() reads every box first, measuring the reveal with the field and its scroller shifted by the container's coming move (y - animator.get('y')), then writes the scroll, then the translate. A new case checks that no box or scroll read follows the move and that the field lands FIELD_OFFSET above the keyboard; it fails on 8b9fa9a.
In iOS Safari and Firefox for Android, focus moving from a Tall sheet's field into a frame, an open shadow host or a custom element no longer starts the hide ride. A field inside one can keep the keyboard up where the page cannot see it, so the content stayed drawn under the keyboard until it closed. Focus moving into one mid-ride rides the content back up, as a field does. The gate shares the guide's hidesField. A docked container that turns too tall reads every box before it moves. The scroller's reveal measures the field where the move leaves it, and the scroll is written before the translate.
|
/approve-spec 2a3b205 |
There was a problem hiding this comment.
Changes requested: two items block merge.
The spec:AST-099 edits carry an engineering owner's approval for this head, and the frame, shadow-host and read-order fixes hold up, each with a test that fails before the fix. CI has not run on this draft head yet.
- Miss:
revealWithKeyboardinpackages/core/src/keyboard/keyboardTarget.ts(lines 553-558) reveals the focused field at every visual-viewport report, scroll events included, so once fields adopt it each pan of the visual viewport re-measures the field and scrolls a field the person moved out of view straight back; reveal only when the keyboard's height changes (and on focus), as AST-099 FR9 asks. - Attach Chrome on Android (Gboard) and Firefox for Android emulator runs of a blur in a fully expanded Tall sheet with the keyboard up, at the final head, with animations at normal scale and removed plus a slow-device profile, as AST-099's platform support requires before merge; the takes in the description come from an earlier head.
[Automated review]
| // Whether focus on `target` may keep the keyboard up. A control that brings | ||
| // one up does, and so can a frame, an open shadow host or a custom element, | ||
| // through its own field. | ||
| function mayKeepKeyboard(target: Element | null): boolean { |
There was a problem hiding this comment.
Non-blocking: hidesField counts every open shadow host, so focus moving to a plain button inside one (a web-component button, say) starts no ride either. On iOS the content then holds until the keyboard's report and glides down only after it, not from the blur as FR3 says. An open root's shadowRoot.activeElement names what holds focus; resolving it before bringsUpKeyboard would keep the conservative rule only for frames, closed roots and custom elements with no open root.
It read the content's drawn place at the test's real clock, and its glide started whenever a slow run handled the report. The clock now holds still through the report, and both samples read fixed times after it.
|
/approve-spec 9058a60 |
There was a problem hiding this comment.
Changes requested: two items still block merge.
The spec:AST-099 edits carry an engineering owner's approval for this head, and the new commit's held clock makes the iOS glide test sample the same curve however slowly it runs. CI has not run on this draft head yet.
- Miss:
revealWithKeyboardinpackages/core/src/keyboard/keyboardTarget.ts(lines 553-558) still reveals the focused field at every visual-viewport report, scroll events included, so once fields adopt it each pan of the visual viewport re-measures the field and scrolls a field the person moved out of view straight back; reveal only when the keyboard's height changes (and on focus), as AST-099 FR9 asks. - Attach Chrome on Android (Gboard) and Firefox for Android emulator runs of a blur in a fully expanded Tall sheet with the keyboard up, at the final head, with animations at normal scale and removed plus a slow-device profile, as AST-099's platform support requires before merge; the takes in the description come from an earlier head.
[Automated review]
| const stopReports = subscribeKeyboardHeight(win, event => { | ||
| if (event.type === 'viewport') { | ||
| revealNow(event.height); | ||
| pans.report(event.height); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Miss (INV18, FR9): this subscriber reveals on every 'viewport' report, and the module raises one for each visual-viewport resize and scroll and each window resize (keyboardHeight.ts:119-138). Each call reads the field's and its scroller's boxes in revealDistance and scrolls the field back into its band, so a pan or zoom of the visual viewport with a page field focused lays the page out and pulls the field back. AST-099 FR9 asks for the reveal in the frame the keyboard's size is reported. Reveal only when event.height differs from the last height (as watchPans already tracks), and keep the focusin reveal.
There was a problem hiding this comment.
Fixed in 4c2f20b: the reveal now comes only when the keyboard's height changes by more than a pixel (and still on focus), so a pan, a scroll or a zoom's rounding leaves the field where the person put it; the put-back of the browser's own pan is unchanged. A new case checks a same-height scroll and a sub-pixel change; it fails on 9058a60.
The reveal by a field's scroller came at every visual-viewport report, scrolls, pans and zooms included, so a field the person scrolled away came straight back. It now comes when the keyboard's height changes by more than a pixel, and still when focus moves; the put-back of a browser's own pan is kept.
|
/approve-spec 4c2f20b |
There was a problem hiding this comment.
Changes requested: one item still blocks merge. The spec:AST-099 edits (FR3's Safari on iOS hide ride, its verification row and Gap 12) carry an engineering owner's approval of this head, and the code matches the record.
- Attach Chrome on Android (Gboard) and Firefox for Android emulator runs at this exact head of a blur in a fully expanded Tall sheet with the keyboard up, with animations at normal scale and removed plus a slow-device profile, as AST-099's platform support requires before merge; the runs in the description were taken on an earlier head, before the Firefox ride gate changed and
mainwas merged in.
Non-blocking: rideWithKeyboard re-places its container at every visual-viewport report, a pan included (keyboardTarget.ts:743-748), so if a keyboard report means a change of the keyboard's size, gate it on the height as the field reveal now does. CI has not run on this draft head yet.
[Automated review]
Review this PR's change: vjeux/astryx@0081c11...4c2f20b
Behavior module: keyboard
User need
On a phone, a focused field must stay above the on-screen keyboard, in the page or a bottom bar; today only a fully expanded Tall BottomSheet does.
Problem and solution fit
Safari pans the page to reveal a field, a bottom bar's field stays covered, and iOS Safari's Tall sheet jumps after the keyboard closes. The module gains:
translate), a scroller around the field reveals it; with none, the browser's reveal stays.onClickonce.Current specification
spec:AST-099FR9, DEC-5, FR3, FR4 and FR8;architecture:react-component-runtimeINV11. Updates AST-099's FR3 and Gaps 1, 11 and 12.Public delta and caller burden
Internal, except a fix for a Tall BottomSheet's keyboard close (changeset); fields adopt the rest in #9777.
Design and alternatives
distanceIntoBand.Evidence
Unit tests drive a stand-in visual viewport; iOS Simulator and Firefox for Android logs are under Testing.
Known limits
Scope
Testing
keyboardTarget.test.ts(44 cases) drives the targets through a stand-in visual viewport: the reveal on both edges by the nearest scroller only, in the report's own event, and only when the keyboard's height changes by more than a pixel, so a pan, a scroll or a zoom leaves a field the person scrolled away, with nothing else moved when it cannot scroll far enough; a page with nothing to scroll left to the browser's own reveal; a field in a full-viewport container revealed by its scroller there, and left to the browser with none; a docked container taller than the room above the keyboard kept in place while its scroller reveals the field, every box read before a container that turns too tall moves back, and the field revealed from where that move leaves it, and with no scroller lifted only as far as its field needs, with the guards for an app's move and an outside field kept; the next field revealed on a focus move with no new report; an app's docked bar moved by the part the keyboard covers, and back once it hides; a bar the app already moves with the keyboard, from a listener that runs before or after the module's, left alone; a bar the app lifts part of the way with its own transform moved by the rest, its transform kept; a bar with the app's own CSS transition or animation running on its position left alone, while the ride's own animation, the transition a bar'stransition: allruns on the ride's translate, an app's script animation, and an app fade do not count; only that bar ever carries a translate, and only while its layout stays docked; a second field in the same bar keeps the bar up; a fixed bar floating above the bottom edge, and a bar the app centres with its owntranslate, left exactly as they are, while a field in either with a scroller around it is revealed by that scroller; a Core host's own sticky element rides; the stand-down inside a target; the synthetic touch click on the field and an ancestor; a pan the browser makes after an unclaimedfocus()put back; nothing under a mouse.BottomSheet.test.tsx, a Tall sheet in iOS Safari (no keyboard geometry API, a coarse pointer): after a blur the content's drawn top, sampled every 16 ms over 0.5 s, moves down in steps under 40 px to where the earlier scroll draws it while the room and scroll stay, and the keyboard's report then moves nothing drawn and clears the translate; under Reduce Motion it dips out and back in at its resting place, never between; under a fine pointer nothing rides; focus moving from the field to a select, whose picker iOS puts where the keyboard was, starts no ride, and neither does focus moving into an open shadow host or a frame, whose own field can keep the keyboard up; a field focused mid-slide rides it back up, and so does focus moving into a shadow host or a frame then; a report while the field keeps focus glides the content from where it was drawn.Chrome on Android with a busy page (
BottomSheet.test.tsx,keyboardChromium.test.ts): Chrome's reports of a blur's hide reach the page 603 and 1119 ms after it, long after Gboard's 167 ms slide. The content stays drawn where the slide leaves it; the report of the keyboard partway down moves nothing; the report of the keyboard gone drops room, scroll and translate in one commit, with nothing drawn moving. In the guide, late reports and the check after the slide turn nothing back, and neither does a hand-off to a select, whose picker Chrome opens over the page. Guards: a field focused again mid-hide rides the content back up; focus moved into a frame or an open shadow host still lets the viewport decide (the content rides back up after the check); after Back, the keyboard brought back by the still-focused field rides up. Red-first arm c2dc0f3 (these tests over 4d1daec's source, not for landing): 4 of them fail there, and they pass here.keyboardTarget.guide.test.ts: a ride stopped by a blur lands on the hide's slide, with no set that cuts it.useMobileKeyboard.target.test.tsx: the tap route drops under a mouse and returns for a finger.keyboardHeight.test.ts: the tap route hands Safari's late mouse events to the shared click guard, which a new press, its click or its 400 ms window ends.revealBand.test.ts: 5 cases.scrollingAncestor.test.ts: the shared walk thatnearestScroller, the fixed-container reveal and Outline's scroll spy now call: the nearest block-axis scroller, and the bound.Red-first: each of these cases failed before its fix: the covered-part lift, the bar the app already moves from a listener that runs after the module's, the bar the app lifts part of the way, the floating bar, the bar with its own
translate, the bar the app is moving itself, the bar undertransition: all, the script-animated bar, the full-viewport container's scroller, the full-height drawer with and without a scroller, its guards, the page with nothing to scroll, the nearest-scroller-only reveal, the iOS sheet's blur ride, Reduce Motion dip and focus-kept glide (before its fix the content stays 248 px off, then jumps), the field-to-select case (before its fix the content rode down under the picker), the scroller reveal in a floating bar and in a bar with its owntranslate(before, the browser's reveal stayed), and the iOS shadow-host and frame cases and the too-tall read order (arm 83b55c6 runs these tests over 8b9fa9a's source, not for landing: all 5 fail there), and the same-height report case (arm 27b2791, over 9058a60's source). The listener-before case, the own-ride-animation case, the no-scroller full-viewport case, the iOS mid-slide refocus and the fine-pointer case are regression guards; they passed before too.vitest run packages/core/src/keyboard packages/core/src/BottomSheet: 21 files, 627 tests pass (the keyboard module's 121).iOS Simulator (iPhone 17 Pro, iOS 26.2, Safari), the Tall sheet story, a field focused, then the keyboard dismissed; a per-frame page log of where the content is drawn (its translate less the body's scroll). On main 42ccd7c, feat(core): one keyboard module; a Tall BottomSheet reads it and rides Gboard #8559 and an earlier head of this pull request, the content holds still while the keyboard hides, then jumps 170 px in one frame at iOS's report, 472–527 ms after the blur. With this pull request's ride (later heads add only the select gate): Done −170 → 0 px over 19 frames from 57 to 441 ms, every step down, and 0.23 px in the report's frame; after scrolling 260 px first, −474 → −260 (the earlier scroll), 0.47 px at the report; a drag on the sheet with the keyboard up, −170 → 0 over 20 frames, 0.14 px at the report.
iOS Simulator (iPhone 17 Pro, iOS 26.2, Safari), taps on the Tall sheet right after a drag of its handle, with the story built from 30cd9af's source (this head adds only comments and main's merge), from 28dfc7b and from main 9e559e9. Each take drags the sheet 44 pt by its handle, taps Title 100 ms after the lift (inside the click guard's 400 ms), taps again 250 ms later (on the same spot, or 141 pt to its left), drags again (the keyboard goes down) and taps Title 100 ms after that lift. Real touches come from one HID stream, and the page logs every pointer, touch, mouse, click and focus event in its capture phase. Three takes per build and variant:
No drag makes a click. On every build alike, a second tap that starts while iOS brings up the keyboard for the field just focused never reaches the page: no pointerdown or touchstart. Frames from one take on 30cd9af, with the finger as a dot and Safari's address text masked: the drag, the tap right after it, the keyboard up with no dot for the second tap, the field focused, the second drag and the tap after it.
Android 16 emulator (Pixel 1080×2400, Gboard), a blur in the Tall sheet with the keyboard up, measuring how far the field rises again once it has started down. At 30cd9af, Chrome 133: 0 px in every take, at normal speed (2), with animations removed (2) and at 4× CPU slowdown (4). In 4 of those takes (2 at 4× CPU, 2 with animations removed), Chrome reported the keyboard still up after the blur; at 4d1daec that late report sent the field back up 47–62 px at 4× CPU. Firefox for Android 159.0.1 (Mozilla's release build, a coarse primary pointer): 0 px (2 takes); it reports the hide once, at its end, so the content moves its 23 px in that frame (Gap 12), as at 4d1daec, with no page pan there. Chrome re-takes at this head, the slow-page Firefox takes and a mid-hide re-focus are being recorded.
Reachability of this head's last product change, the reveal's height check:
revealWithKeyboardis called only atkeyboardTarget.ts:521(infollowKeyboard,:490) and:874(inkeepFieldAboveKeyboard,:839). Those two are called only atuseMobileKeyboard.ts:233–234, afterif (!isEnabled || reservesInset || !isTouch) returnat:229. The one caller ofuseMobileKeyboard,BottomSheetPanel.tsx:767, passesinset: true(:769), so the Tall sheet never reaches it.Full local gates with main 0081c11 merged in (b2a51e4), node 24.22.1 from
.nvmrc, afterpnpm install --frozen-lockfile: the keyboard, BottomSheet, Outline, utils, hooks and gestures units (173 files, 3,663 tests); strict ESLint over the whole repo (CI=true pnpm lint, 0 errors); the full build, then typecheck (0 errors); knowledge records, export sync, changesets and their coverage, the i18n catalog and runtime checks, CLI API types, token docs and Lab readiness pass;pnpm api:reportshows only main's own lines; the UI tests for the changed set (428 files, 13,089 tests), the node tests (1 file, 4 tests) and the CLI golden reads (13) pass; Storybook builds; in Chromium the BottomSheet keyboard, scroll-end and drag a11y specs (33), the keyboard-inset interaction spec (9) and Outline's spec (1) pass. On top of it: comment splits, the shared overflow values' import, the frame and read-order fixes, the reveal's height check, and a test-only commit: the iOS glide case holds its clock through the report, so a slow run reads the same curve (with 6 busy loops pinned to the test's 2 cores, the old sampling fails 8 of 8 runs and this one passes 10 of 10). At this head, Prettier, strict ESLint,CI=true pnpm lint(0 errors), the keyboard, BottomSheet, utils and Outline units (82 files, 1,822 tests), the core typecheck andpnpm review:pre(no findings) pass.Writing rubric results
R1 narration: none; FR3's sentence names Safari on iOS, and Gaps 1, 11 and 12 state what the keyboard module does and what is still missing, in the present tense.
R2 research: none in the record.
R3 evidence: Gaps 1, 11 and 12 cite the module's source path, not a pull request; FR3's case is a row in its verification table.
R4 rejected alternatives: none added.
R5 change references: none.
R6 open questions: none.