Repository navigation
fix(core): an anchored layer that would run under the software keyboard ends above it - #8208
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
github-actions
Bot
added
the
needs:spec-owner-review
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| lab/Schedule | 🔴 68.9 kB (+5.1 kB) · 3.9 kB CSS (0) | — | — · — · — |
| Selector | 🔴 87.8 kB (+4.7 kB) · 16.3 kB CSS (+101 B) | — | — · — · — |
| MoreMenu | 🔴 82.8 kB (+4.6 kB) · 16.5 kB CSS (+91 B) | — | — · — · — |
120 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| DropdownMenu | 🔴 82.5 kB (+4.6 kB) · 16.5 kB CSS (+91 B) | — | — · — · — |
| TableHeaderCell | 🔴 88.5 kB (+4.7 kB) · 16.5 kB CSS (+74 B) | — | — · — · — |
| Pagination | 🔴 97.8 kB (+4.6 kB) · 16.7 kB CSS (+104 B) | — | — · — · — |
| TableCell | 🔴 88.9 kB (+4.6 kB) · 16.8 kB CSS (+75 B) | — | — · — · — |
| ContextMenu · Storybook | 🔴 87.7 kB (+4.6 kB) · 16.4 kB CSS (+76 B) | 699 (+4) | — · — · — |
| Table | 🔴 97.1 kB (+4.6 kB) · 17.0 kB CSS (+75 B) | — | — · — · — |
| richtext/RichTextEditorToolbar | 🔴 238.7 kB (+4.7 kB) · 454 B CSS (0) | — | — · — · — |
| lab/CommentComposer | 🔴 113.6 kB (+4.6 kB) · 1.9 kB CSS (0) | — | — · — · — |
| lab/TransferListSelector | 🔴 91.7 kB (+4.6 kB) · 1.9 kB CSS (0) | — | — · — · — |
| Carousel | 🔴 57.3 kB (+4.5 kB) · 9.5 kB CSS (+76 B) | — | — · — · — |
| lab/TourStep | 🔴 65.9 kB (+4.6 kB) · 590 B CSS (0) | — | — · — · — |
| Markdown | 🔴 158.2 kB (+4.5 kB) · 19.3 kB CSS (+91 B) | — | — · — · — |
| MultiSelector · Storybook | 🔴 84.8 kB (+4.5 kB) · 15.7 kB CSS (+101 B) | 2,705 (+13) | — · — · — |
| lab/TransferList | 🔴 84.6 kB (+4.5 kB) · 1.7 kB CSS (0) | — | — · — · — |
| Popover · Storybook | 🔴 59.8 kB (+4.4 kB) · 12.9 kB CSS (+95 B) | 1,039 (+1) | — · — · — |
| lab/Comments | 🔴 278.2 kB (+4.5 kB) · 2.5 kB CSS (0) | — | — · — · — |
| DateRangeInput | 🔴 80.3 kB (+4.4 kB) · 11.3 kB CSS (+78 B) | — | — · — · — |
| lab/ListInput | 🔴 90.0 kB (+4.5 kB) · 1.6 kB CSS (0) | — | — · — · — |
| lab/CommentThread | 🔴 273.0 kB (+4.5 kB) · 2.4 kB CSS (0) | — | — · — · — |
| Tokenizer | 🔴 79.2 kB (+4.4 kB) · 17.2 kB CSS (+101 B) | — | — · — · — |
| lab/ChatEmojiPicker | 🔴 73.5 kB (+4.4 kB) · 773 B CSS (0) | — | — · — · — |
| lab/ChatReactionBar | 🔴 74.0 kB (+4.4 kB) · 1.1 kB CSS (0) | — | — · — · — |
| TabList | 🔴 40.1 kB (+4.2 kB) · 6.4 kB CSS (+71 B) | — | — · — · — |
| Toolbar | 🔴 39.4 kB (+4.2 kB) · 7.9 kB CSS (+63 B) | — | — · — · — |
| HoverCard | 🔴 23.7 kB (+4.2 kB) · 4.0 kB CSS (+66 B) | — | — · — · — |
| Avatar | 🔴 39.2 kB (+4.2 kB) · 5.5 kB CSS (+68 B) | — | — · — · — |
| Tooltip | 🔴 25.0 kB (+4.2 kB) · 4.0 kB CSS (+64 B) | — | — · — · — |
| AvatarGroup | 🔴 41.9 kB (+4.2 kB) · 5.6 kB CSS (+69 B) | — | — · — · — |
| StatusDot | 🔴 27.2 kB (+4.2 kB) · 5.3 kB CSS (+67 B) | — | — · — · — |
| FieldLabel | 🔴 42.2 kB (+4.2 kB) · 4.9 kB CSS (+67 B) | — | — · — · — |
| InfoTip | 🔴 42.4 kB (+4.1 kB) · 5.2 kB CSS (+68 B) | — | — · — · — |
| Field | 🔴 46.0 kB (+4.1 kB) · 6.4 kB CSS (+66 B) | — | — · — · — |
| TextArea | 🔴 53.3 kB (+4.1 kB) · 8.9 kB CSS (+68 B) | — | — · — · — |
| InputGroup | 🔴 48.2 kB (+4.1 kB) · 7.3 kB CSS (+68 B) | — | — · — · — |
| CheckboxList | 🔴 49.7 kB (+4.1 kB) · 7.5 kB CSS (+67 B) | — | — · — · — |
| RadioList | 🔴 47.4 kB (+4.0 kB) · 6.4 kB CSS (+65 B) | — | — · — · — |
| AlertDialog | 🔴 94.0 kB (+3.8 kB) · 13.2 kB CSS (+81 B) | — | — · — · — |
| DropdownMenuSubMenu | 🔴 63.5 kB (+3.8 kB) · 9.4 kB CSS (+69 B) | — | — · — · — |
| ListItem | 🔴 62.5 kB (+3.8 kB) · 10.8 kB CSS (+87 B) | — | — · — · — |
| TextInput | 🔴 58.3 kB (+3.8 kB) · 10.0 kB CSS (+79 B) | — | — · — · — |
| FileInput | 🔴 59.2 kB (+3.8 kB) · 9.7 kB CSS (+81 B) | — | — · — · — |
| BaseTypeahead | 🔴 60.9 kB (+3.7 kB) · 9.7 kB CSS (+91 B) | — | — · — · — |
| AvatarGroupOverflow | 🔴 44.1 kB (+3.8 kB) · 5.5 kB CSS (+69 B) | — | — · — · — |
| TopNavMenu | 🔴 64.8 kB (+3.7 kB) · 10.0 kB CSS (+77 B) | — | — · — · — |
| ComplexSelector | 🔴 61.4 kB (+3.7 kB) · 10.5 kB CSS (+78 B) | — | — · — · — |
| Toast | 🔴 64.5 kB (+3.8 kB) · 9.1 kB CSS (+63 B) | — | — · — · — |
| TopNav | 🔴 62.1 kB (+3.7 kB) · 9.7 kB CSS (+78 B) | — | — · — · — |
| ChatComposer | 🔴 67.1 kB (+3.7 kB) · 10.6 kB CSS (+79 B) | — | — · — · — |
| Typeahead | 🔴 68.8 kB (+3.7 kB) · 11.3 kB CSS (+95 B) | — | — · — · — |
| Link | 🔴 48.1 kB (+3.7 kB) · 6.9 kB CSS (+67 B) | — | — · — · — |
| SideNavHeading | 🔴 65.4 kB (+3.7 kB) · 10.0 kB CSS (+75 B) | — | — · — · — |
| TableSelectionToolbar | 🔴 60.1 kB (+3.7 kB) · 11.9 kB CSS (+77 B) | — | — · — · — |
| CheckboxListItem | 🔴 63.4 kB (+3.7 kB) · 10.8 kB CSS (+87 B) | — | — · — · — |
| ChatComposerInput | 🔴 65.0 kB (+3.7 kB) · 10.1 kB CSS (+78 B) | — | — · — · — |
| ChatComposerTokenElement | 🔴 65.0 kB (+3.7 kB) · 10.1 kB CSS (+78 B) | — | — · — · — |
| TopNavMegaMenu | 🔴 59.9 kB (+3.7 kB) · 10.6 kB CSS (+75 B) | — | — · — · — |
| LayerProvider | 🔴 68.3 kB (+3.7 kB) · 9.5 kB CSS (+78 B) | — | — · — · — |
| ChatLayout | 🔴 53.8 kB (+3.7 kB) · 9.3 kB CSS (+78 B) | — | — · — · — |
| DialogHeader | 🔴 53.4 kB (+3.7 kB) · 10.3 kB CSS (+78 B) | — | — · — · — |
| TopNavHeading | 🔴 64.8 kB (+3.7 kB) · 9.9 kB CSS (+76 B) | — | — · — · — |
| RadioListItem | 🔴 60.5 kB (+3.7 kB) · 9.6 kB CSS (+67 B) | — | — · — · — |
| Thumbnail | 🔴 54.8 kB (+3.7 kB) · 10.3 kB CSS (+84 B) | — | — · — · — |
| NumberInput | 🔴 61.7 kB (+3.7 kB) · 10.2 kB CSS (+79 B) | — | — · — · — |
| ChatLayoutScrollButton | 🔴 51.1 kB (+3.7 kB) · 8.9 kB CSS (+66 B) | — | — · — · — |
| SideNavCollapseButton | 🔴 51.4 kB (+3.7 kB) · 8.8 kB CSS (+65 B) | — | — · — · — |
| IconButton | 🔴 50.6 kB (+3.7 kB) · 8.7 kB CSS (+65 B) | — | — · — · — |
| MobileNav | 🔴 60.4 kB (+3.7 kB) · 9.4 kB CSS (+78 B) | — | — · — · — |
| DrawerHeader | 🔴 53.6 kB (+3.7 kB) · 11.7 kB CSS (+79 B) | — | — · — · — |
| lab/CommentPin | 🔴 52.2 kB (+3.8 kB) · 1.2 kB CSS (0) | — | — · — · — |
| Button | 🔴 50.5 kB (+3.7 kB) · 8.7 kB CSS (+65 B) | — | — · — · — |
| Lightbox | 🔴 58.7 kB (+3.7 kB) · 9.2 kB CSS (+67 B) | — | — · — · — |
| CodeBlock | 🔴 64.5 kB (+3.7 kB) · 10.0 kB CSS (+78 B) | — | — · — · — |
| MobileNavToggle | 🔴 50.9 kB (+3.7 kB) · 8.7 kB CSS (+65 B) | — | — · — · — |
| Calendar | 🔴 61.4 kB (+3.7 kB) · 9.3 kB CSS (+75 B) | — | — · — · — |
| ChatDictationButton | 🔴 51.1 kB (+3.7 kB) · 8.8 kB CSS (+65 B) | — | — · — · — |
| SideNavItem | 🔴 60.4 kB (+3.7 kB) · 9.9 kB CSS (+74 B) | — | — · — · — |
| Banner | 🔴 52.5 kB (+3.7 kB) · 9.3 kB CSS (+75 B) | — | — · — · — |
| ChatSendButton | 🔴 51.0 kB (+3.7 kB) · 8.7 kB CSS (+64 B) | — | — · — · — |
| TabMenu | 🔴 61.9 kB (+3.7 kB) · 9.9 kB CSS (+78 B) | — | — · — · — |
| ToggleButton | 🔴 51.4 kB (+3.7 kB) · 8.8 kB CSS (+64 B) | — | — · — · — |
| Stepper | 🔴 52.2 kB (+3.7 kB) · 8.9 kB CSS (+66 B) | — | — · — · — |
| richtext/RichTextView | 🔴 267.7 kB (+3.7 kB) · 2.3 kB CSS (0) | — | — · — · — |
| CheckboxInput | 🔴 57.3 kB (+3.7 kB) · 9.0 kB CSS (+69 B) | — | — · — · — |
| lab/EmojiPicker | 🔴 76.2 kB (+3.7 kB) · 722 B CSS (0) | — | — · — · — |
| richtext/RichTextEditor | 🔴 287.4 kB (+3.7 kB) · 2.6 kB CSS (0) | — | — · — · — |
| AppShell | 🔴 80.0 kB (+3.7 kB) · 12.3 kB CSS (+68 B) | — | — · — · — |
| lab/DiffViewer | 🔴 76.9 kB (+3.7 kB) · 2.1 kB CSS (0) | — | — · — · — |
| BreadcrumbItem | 🔴 75.1 kB (+3.6 kB) · 11.4 kB CSS (+68 B) | — | — · — · — |
| SideNav | 🔴 79.9 kB (+3.6 kB) · 10.2 kB CSS (+80 B) | — | — · — · — |
| lab/SelectableCardGroup | 🔴 55.7 kB (+3.7 kB) · 162 B CSS (0) | — | — · — · — |
| Slider | 🔴 95.9 kB (+3.4 kB) · 8.3 kB CSS (+67 B) | — | — · — · — |
| SegmentedControl | 🔴 87.3 kB (+3.3 kB) · 6.8 kB CSS (+67 B) | — | — · — · — |
| Switch | 🔴 93.4 kB (+3.3 kB) · 9.5 kB CSS (+80 B) | — | — · — · — |
| charts/ChartTooltip | 147.7 kB (+1.8 kB) · 423 B CSS (0) | — | — · — · — |
| PowerSearch | 261.8 kB (+1.6 kB) · 22.7 kB CSS (+113 B) | — | — · — · — |
| TimeInput | 142.3 kB (+1.5 kB) · 14.6 kB CSS (+85 B) | — | — · — · — |
| charts/Chart | 170.3 kB (+1.5 kB) · 494 B CSS (0) | — | — · — · — |
| DateInput | 165.1 kB (+1.4 kB) · 15.6 kB CSS (+82 B) | — | — · — · — |
| DateTimeInput | 186.8 kB (+1.3 kB) · 17.3 kB CSS (+78 B) | — | — · — · — |
| ProgressBar | 11.0 kB (+40 B) · 6.2 kB CSS (+62 B) | — | — · — · — |
| CommandPalette | 58.7 kB (+1 B) · 12.4 kB CSS (+80 B) | — | — · — · — |
| CommandPaletteInput | 27.0 kB (0) · 7.5 kB CSS (+71 B) | — | — · — · — |
| ChatMessageList | 23.0 kB (0) · 6.9 kB CSS (+69 B) | — | — · — · — |
| Spinner | 18.6 kB (−1 B) · 5.8 kB CSS (+68 B) | — | — · — · — |
| Text | 7.4 kB (0) · 5.0 kB CSS (+67 B) | — | — · — · — |
| Timer | 9.6 kB (+1 B) · 5.9 kB CSS (+66 B) | — | — · — · — |
| ChatToolCalls | 26.0 kB (0) · 8.3 kB CSS (+66 B) | — | — · — · — |
| Heading | 7.9 kB (−1 B) · 5.0 kB CSS (+67 B) | — | — · — · — |
| Timestamp | 22.9 kB (0) · 8.9 kB CSS (+63 B) | — | — · — · — |
| charts/ChartLegend | 35.6 kB (+1 B) · 224 B CSS (0) | — | — · — · — |
| Breadcrumbs | 16.1 kB (0) · 3.4 kB CSS (0) | — | — · — · — |
| DropdownMenuCheckboxItem | 21.0 kB (0) · 6.4 kB CSS (0) | — | — · — · — |
| DropdownMenuDivider | 5.2 kB (0) · 548 B CSS (0) | — | — · — · — |
| DropdownMenuGroup | 6.3 kB (0) · 3.4 kB CSS (0) | — | — · — · — |
| DropdownMenuItem | 20.3 kB (0) · 6.0 kB CSS (0) | — | — · — · — |
| DropdownMenuRadioGroup | 6.3 kB (0) · 3.3 kB CSS (0) | — | — · — · — |
| DropdownMenuRadioItem | 21.1 kB (0) · 6.4 kB CSS (0) | — | — · — · — |
| SelectorOption | 19.2 kB (0) · 5.8 kB CSS (0) | — | — · — · — |
| TypeaheadItem | 6.4 kB (0) · 3.6 kB CSS (0) | — | — · — · — |
| ContextMenuItem | 25.9 kB (−1 B) · 6.1 kB CSS (0) | — | — · — · — |
Package size: @astryxdesign/core 6.58 MB packed (+12.8 kB); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 9 rows, core 4 rows; run pnpm api:report.
Wide change (shared styles: packages/core/src/Layer/layerViewportInset.stylex.ts, packages/core/src/theme/derivedVarRegistry.test.ts): add ci:run-visual, ci:run-a11y or ci:run-rtl to run them.
Generated by PR Enrichment workflow | View full report
1663f60 to
a9d43aa
Compare
a9d43aa to
3c17f4f
Compare
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: no current spec states that a Popover should shrink above the software keyboard (AST-059 FR3 allows a component's lower cap, but AST-059 DEC-1 and the layer runtime's INV11 give the viewport inset to the layer), and the pull request also adds unapproved requirements in the draft AST-094, in useMenuPress (FR9, DEC-8 and the touch open row) and in DropdownMenu FR8. An engineering owner can provide or identify the governing spec for the keyboard rule (component Popover, AST-094 or the layer runtime) and record the decision by commenting /approve-spec 3c17f4ffe30c4d670e599e52ca23eb255dd7b206 on this pull request; the author may do so as an engineering owner. Whatever the ruling, these need fixing:
usePopoverKeyboardAvoidanceremoves a caller's inlinemax-block-sizeandoverflow-yfrom the surface on every close, so a Popover givenstyle={{maxBlockSize: 160}}reopens uncapped.- With the keyboard up, tapping the scrolled-in Save button lands no click in Chromium touch, because the blur restores the full height between
mousedownandmouseup. - A popover placed above its trigger is re-positioned rather than shortened from its top, and still ends under the keyboard.
- The useMenuPress record lists a 500 ms long-press delay while the code defaults to 150 ms.
- The compare holds 33,282 authored lines from the dependency stack; land those first so this pull request carries only its 702-line change.
[Automated review]
| const restore = () => { | ||
| style.removeProperty('max-block-size'); | ||
| style.removeProperty('overflow-y'); | ||
| }; |
There was a problem hiding this comment.
restore() removes max-block-size and overflow-y from the surface whether or not this hook set them, and it runs on every close (cleanup) and before every re-measure. The surface also carries the caller's style prop, which React re-applies only when it changes, so a <Popover style={{maxBlockSize: 160, overflowY: 'auto'}}> reopens uncapped after its first close (Chromium: 160 px on the base, 381 px here). Remember what the hook wrote and remove only that, or restore the previous inline values.
There was a problem hiding this comment.
Fixed in cd5c2f1: the hook records the caller's inline max-block-size and overflow-y when the popover opens and puts them back when its cap comes off, so it removes only what it wrote. Unit cases cover a style cap that needs no keyboard cap and one under it; both failed before.
| // Falling (it uncovers the full height as it goes) or at once. | ||
| avoid(transition.to); |
There was a problem hiding this comment.
A tap on a button in the capped popover blurs the field, the guide publishes a predicted hide from focusout, and this restores the full height between mousedown and mouseup. In Chromium touch the tap on the scrolled-in Save note then sends mouseup to a tip and click to the content: Save gets no click. Keeping the cap until the hide's start (or until the click has dispatched) would let the tap land; worth a case in PopoverKeyboard.interaction.browser.spec.ts.
There was a problem hiding this comment.
Fixed in cd5c2f1: while a pointer that went down inside the popover is pressed, a falling keyboard's height change waits for that press's click, or for 350 ms after a lift that makes no click. A new Chromium case, "with the keyboard up, a tap on the scrolled-in Save note clicks it", failed before (0 clicks) and passes now; unit cases cover both the click and the no-click paths.
| const limit = viewportHeight - inset - popover.keyboardClearancePx; | ||
| if (surface.bottom <= limit) { | ||
| return null; | ||
| } | ||
| return Math.max(0, limit - surface.top); |
There was a problem hiding this comment.
This assumes the surface's top stays where it is, but the layer re-positions the shorter box from its anchored side. A popover placed above a trigger near the bottom, capped to 351 px, moved from 16–505 px to 131–482 px and still ended 105 px under a keyboard top at 377 px. Compute the cap from the edge the layer holds, or skip it when it cannot bring the bottom above the keyboard.
There was a problem hiding this comment.
Fixed in cd5c2f1: after capping, the hook measures again. If the bottom did not come up, because the layer holds the bottom edge (a popover above its trigger), it takes the cap off and leaves the popover where the layer placed it. The unit case "leaves a popover whose bottom the layer holds as it is" failed before.
| @@ -140,6 +140,7 @@ Consumer migration instructions belong in consumer docs and release notes. | |||
| | stray-click window | 400 ms, or the next `pointerdown` | how long after a tracked release the browser's click is still swallowed | `MENU_PRESS_STRAY_CLICK_MS` | `module:DropdownMenu/useMenuPress` | stable | | |||
| | settle time | 300 ms | proposed: how long an opening press must last before its release may act | `MENU_PRESS_SETTLE_MS` | `module:DropdownMenu/useMenuPress` | proposed | | |||
| | long-press delay | 500 ms | proposed: how long a finger rests on the trigger before the menu opens under it | `longPressDelayMs` | `module:DropdownMenu/useMenuPress` | proposed | | |||
There was a problem hiding this comment.
This row still says 500 ms, while useMenuPress.ts now defaults longPressDelayMs to 150 ms (menu.holdOpenMs, plus 150 ms inside a scroller). An owner approving this record would approve a number the code does not ship; please update the row, or the code.
There was a problem hiding this comment.
This record comes from the DropdownMenu press change below this one in the stack, and that change is being brought in line with the code (the 150 ms hold-open and continueGesture). Nothing to change in this pull request.
3c17f4f to
fc6757e
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: no current spec says a Popover should shrink above the software keyboard (AST-059 FR3 allows a component's lower cap, but AST-059 DEC-1 and the layer runtime's INV11 give the viewport inset to the layer), and the pull request also adds unapproved requirements in the draft AST-094, in useMenuPress (FR9, DEC-8 and the touch open row) and in DropdownMenu FR8. An engineering owner can name the record that should govern the keyboard rule (Popover, AST-094 or the layer runtime) and record the decision by commenting /approve-spec fc6757e15246a0f0b1fc6c91005eb39397aad18e on this pull request; the author may do so as an engineering owner. Whatever the ruling, these need fixing:
usePopoverKeyboardAvoidanceremoves a caller's inlinemax-block-sizeandoverflow-yfrom the surface on every close, so a Popover givenstyle={{maxBlockSize: 160}}reopens uncapped.- A tap on a button in the capped popover blurs the field, and the full height comes back between the tap's press and release, so the tap can miss the Save button the cap made reachable.
- A popover placed above its trigger is shortened from its top and still ends under the keyboard.
- The useMenuPress record lists a 500 ms long-press delay while the code defaults to 150 ms, and AST-094 FR13 does not state the iOS-only rule that Popover now follows and DropdownMenu does not.
- The compare holds 33,976 authored lines from the dependency stack; land those first so this pull request carries only its 780-line change.
[Automated review]
| const restore = () => { | ||
| style.removeProperty('max-block-size'); | ||
| style.removeProperty('overflow-y'); | ||
| }; |
There was a problem hiding this comment.
restore() removes max-block-size and overflow-y from the surface whether or not this hook set them, and it runs before every measurement and on every close (:189). A caller's style={{maxBlockSize: 160, overflowY: 'auto'}} is merged onto this same element, and React will not write it again while its value is unchanged, so the Popover reopens without it. Remember the inline values before the first write and put exactly those back.
| // Falling (it uncovers the full height as it goes) or at once. | ||
| avoid(transition.to); |
There was a problem hiding this comment.
A tap on a button in the capped popover blurs the field; the guide publishes a predicted hide from focusout, and this branch restores the full height at once, between the tap's mousedown and mouseup, so the click can land elsewhere than the Save the cap made reachable. Keep the cap until the viewport shows the keyboard going down, or until the tap's click has dispatched.
There was a problem hiding this comment.
Fixed in cd5c2f1, as in the thread above: the cap holds through a press inside the popover until its click, and the Chromium Save-tap case covers it.
| if (surface.bottom <= limit) { | ||
| return null; | ||
| } | ||
| return Math.max(0, limit - surface.top); |
There was a problem hiding this comment.
This assumes the top edge stays put, but the layer places the popover with position-area: a popover above its trigger keeps its bottom edge, so the shorter box moves its top down and still ends under the keyboard. Compute the cap from the edge the layer holds, or skip it when it cannot clear the keyboard.
There was a problem hiding this comment.
Fixed in cd5c2f1, as in the thread above: when the layer holds the bottom edge, the cap comes off and the popover is left where it is.
| @@ -140,6 +140,7 @@ Consumer migration instructions belong in consumer docs and release notes. | |||
| | stray-click window | 400 ms, or the next `pointerdown` | how long after a tracked release the browser's click is still swallowed | `MENU_PRESS_STRAY_CLICK_MS` | `module:DropdownMenu/useMenuPress` | stable | | |||
| | settle time | 300 ms | proposed: how long an opening press must last before its release may act | `MENU_PRESS_SETTLE_MS` | `module:DropdownMenu/useMenuPress` | proposed | | |||
| | long-press delay | 500 ms | proposed: how long a finger rests on the trigger before the menu opens under it | `longPressDelayMs` | `module:DropdownMenu/useMenuPress` | proposed | | |||
There was a problem hiding this comment.
This row still says 500 ms, while useMenuPress.ts now defaults longPressDelayMs to 150 ms (menu.holdOpenMs, and 300 ms inside a scrolling region). An owner approving this record would approve a number the code does not ship.
There was a problem hiding this comment.
This record comes from the DropdownMenu press change below this one in the stack, and that change is being brought in line with the code (the 150 ms hold-open and continueGesture). Nothing to change in this pull request.
fc6757e to
84ceb52
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: no current spec says a Popover should shrink above the software keyboard (AST-059 FR3 allows a component's lower cap, while AST-059 DEC-1 and the layer runtime's INV11 give the viewport inset to the layer), and the pull request also adds unapproved requirements in the draft AST-094, in useMenuPress (FR9, DEC-8 and the touch open row) and in DropdownMenu FR8. An engineering owner can name the record that should govern the keyboard rule (Popover, AST-094 or the layer runtime) and record the decision by commenting /approve-spec 84ceb521692e1e863e2cf46f77a7608b1c9838b5 on this pull request; the author may do so as an engineering owner. Whatever the ruling, these need fixing:
usePopoverKeyboardAvoidanceremoves a caller's inlinemax-block-sizeandoverflow-yfrom the surface on every close, so a Popover givenstyle={{maxBlockSize: 160}}reopens uncapped.- A Popover whose content fits now declares
touch-action: none, which blocks page pinch-zoom over it;pinch-zoom, as the menus use, keeps the page still and zoomable. - The 744 px compact test contradicts AST-012 FR9 (width below 768 px or height below 500 px), and the touch Reduce Motion fades run longer than the 200 ms that the reduced-motion design rule allows.
- A tap on a button in the capped popover restores the full height between press and release, and a popover placed above its trigger is shortened from its top and still ends under the keyboard.
- The useMenuPress record says 500 ms and "the same settle rule" where the code ships 150 ms and a 10 px handoff, and the compare holds 34,027 authored lines from the dependency stack; land those first so this pull request carries only its 780-line change.
[Automated review]
| const {style} = surface; | ||
| let isDisposed = false; | ||
| const guide = getKeyboardLayoutGuide(win); | ||
| const restore = () => { |
There was a problem hiding this comment.
restore() removes max-block-size and overflow-y from the surface whether or not this hook set them, and it runs before every measurement and on every close. The surface stays mounted while closed and React re-writes a caller's style only when its value changes, so a Popover given style={{maxBlockSize: 160, overflowY: 'auto'}} reopens without them. Recording the inline values before the first write and putting exactly those back would keep the caller's styles.
| return; | ||
| } | ||
| // Falling (it uncovers the full height as it goes) or at once. | ||
| avoid(transition.to); |
There was a problem hiding this comment.
A tap on a button in the capped popover blurs the field; the guide publishes a predicted hide from focusout, and this branch restores the full height at once, between the tap's mousedown and mouseup, so the click can land on another element than the Save it was aimed at. Keeping the cap until the viewport reports the keyboard going down (or the tap's click has dispatched) avoids that.
| if (surface.bottom <= limit) { | ||
| return null; | ||
| } | ||
| return Math.max(0, limit - surface.top); |
There was a problem hiding this comment.
This assumes the top edge holds, but the layer places the box with position-area against its trigger: a popover above its trigger keeps its bottom edge, so the cap moves its top down and its bottom stays under the keyboard.
| // scroll: the page behind the popover stays where it is, as it does | ||
| // behind a UIKit popover. A scrolling surface keeps its own pan above. | ||
| surfaceStill: { | ||
| touchAction: 'none', |
There was a problem hiding this comment.
touch-action: none also stops page pinch-zoom for any finger that lands on the popover, on every touch screen. pinch-zoom still admits no one-finger pan and leaves the pinch to the browser, which is what DropdownMenu does for a menu whose rows fit (touchPinchZoom) and what draft AST-094 FR10 asks for.
| export const TOUCH_QUERY = '(pointer: coarse)'; | ||
|
|
||
| /** The viewport is compact: layout behaviours. */ | ||
| export const COMPACT_QUERY = '(width < 744px), (height < 500px)'; |
There was a problem hiding this comment.
Current AST-012 FR9 defines compact as a viewport width below the effective md point (768) or a height below 500, and says the web touch presentation resolver keeps no size numbers of its own. With 744 here, a coarse-pointer screen 744–767 px wide and at least 500 px tall gets the popover where FR9 makes it compact.
| prefersReducedMotion: boolean, | ||
| ): SpringDescription { | ||
| if (prefersReducedMotion) { | ||
| return springs.menuReducedMotion; |
There was a problem hiding this comment.
Under reduced motion with a coarse pointer, the current reduced-motion design rule (DR2) asks for an opacity-only fade that finishes in 200 ms or less. menuReducedMotion is a 0.4 s critically damped spring, about 82% of the way at 200 ms.
| @@ -140,6 +140,7 @@ Consumer migration instructions belong in consumer docs and release notes. | |||
| | stray-click window | 400 ms, or the next `pointerdown` | how long after a tracked release the browser's click is still swallowed | `MENU_PRESS_STRAY_CLICK_MS` | `module:DropdownMenu/useMenuPress` | stable | | |||
| | settle time | 300 ms | proposed: how long an opening press must last before its release may act | `MENU_PRESS_SETTLE_MS` | `module:DropdownMenu/useMenuPress` | proposed | | |||
| | long-press delay | 500 ms | proposed: how long a finger rests on the trigger before the menu opens under it | `longPressDelayMs` | `module:DropdownMenu/useMenuPress` | proposed | | |||
There was a problem hiding this comment.
This row still says 500 ms, while useMenuPress.ts now defaults longPressDelayMs to 150 ms (menu.holdOpenMs, and 300 ms inside a scrolling region). An owner approving the record would approve a number the code does not ship.
| | FR7 | While a press is tracked in a menu that scrolls, a pointer resting within the edge zone of the menu's top or bottom MUST scroll the menu toward that edge and re-read the row under the pointer as the rows move. | `useMenuPress.test.tsx` autoscroll case | verify | | ||
| | FR8 | On a mouse a DropdownMenu trigger MUST open its menu on press-down (button 0, no Control), and a drag from the trigger into the menu that releases over an enabled row MUST act on it. The opening gesture's release MUST act only if the pointer entered the menu or the press lasted the settle time; otherwise nothing acts and the menu stays open. Pressing the trigger of an open menu MUST close it and MUST NOT reopen it in the same gesture. | Proposed DEC-4; `DropdownMenu.test.tsx` press model suite, `menuPressGesture.test.ts` trigger states | proposed; pending owner review | | ||
| | FR9 | Under a finger a tap on the trigger MUST open the menu through its click. A press held on the trigger for the long-press delay MUST open the menu with the finger down, prevent the default of the finger's later `touchmove`s until the gesture ends, and continue under FR1–FR4 with the same settle rule. | Proposed DEC-4; `DropdownMenu.test.tsx` held-touch case, `useMenuPress.test.tsx` | proposed; pending owner review | | ||
| | FR9 | By default a finger's tap on the trigger MUST open the menu through its click, and a press held for the long-press delay MUST open it with the finger down and prevent the default of the finger's later `touchmove`s until the gesture ends. With touch open `press` the press-down MUST open it at once, and a cancel before the pointer enters the menu MUST close it again. Both continue under FR1–FR4 with the same settle rule. | Proposed DEC-4, DEC-8; `DropdownMenu.test.tsx` touch cases, `useMenuPress.test.tsx` | proposed; pending owner review | |
There was a problem hiding this comment.
Both finger opens are said to continue "with the same settle rule", but under a finger the machine replaces the settle rule with a 10 px handoff (MENU_PRESS_HANDOFF_PX, menuPressGesture.ts:324-349, :364-369) that no record states.
84ceb52 to
cd5c2f1
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: no current spec says a Popover should shrink above the software keyboard (AST-059 FR3 allows a component's lower cap, while AST-059 DEC-1 and the layer runtime's INV11 give the viewport inset to the layer), and the pull request also adds unapproved requirements in the draft AST-094, in useMenuPress (FR9, DEC-8 and the touch open row) and in DropdownMenu FR8. An engineering owner can name the record that should govern the keyboard rule (Popover, AST-094 or the layer runtime) and record the decision by commenting /approve-spec cd5c2f1acdb0b4b961199b57d1626232d5ca494c on this pull request; the author may do so as an engineering owner. The inline-style, mid-tap and above-trigger fixes in this push look right in the code; whatever the ruling, these still need fixing:
- A Popover whose content fits declares
touch-action: none, which blocks page pinch-zoom over it;pinch-zoom, as the menus use, keeps the page still and zoomable. - The 744 px compact test contradicts AST-012 FR9 (width below 768 px or height below 500 px), and the touch Reduce Motion fades run longer than the 200 ms the reduced-motion design rule allows.
- The useMenuPress record says 500 ms and "the same settle rule" where the code ships 150 ms and a 10 px handoff, and AST-094 FR13 keys touch behaviour on a coarse pointer where Popover keys on WebKit on iOS.
- The compare holds 34,358 authored lines from the dependency stack; land those first so this pull request carries only its 993-line change.
[Automated review]
| // scroll: the page behind the popover stays where it is, as it does | ||
| // behind a UIKit popover. A scrolling surface keeps its own pan above. | ||
| surfaceStill: { | ||
| touchAction: 'none', |
There was a problem hiding this comment.
touch-action: none also stops page pinch-zoom for any pinch that starts on a Popover whose content fits, on every touch screen. The menus keep the pinch with touch-action: pinch-zoom (current useMenuPress FR4), and this head's own draft AST-094 FR10 and FR18 say page pinch-zoom must stay the browser's. pinch-zoom still admits no one-finger pan, so the page behind stays still.
| export const TOUCH_QUERY = '(pointer: coarse)'; | ||
|
|
||
| /** The viewport is compact: layout behaviours. */ | ||
| export const COMPACT_QUERY = '(width < 744px), (height < 500px)'; |
There was a problem hiding this comment.
Current AST-012 FR9 defines compact as a viewport width below the effective md point (768) or a height below 500, and says the web touch presentation resolver keeps no size numbers of its own. With 744 here, a coarse-pointer screen 744–767 px wide (an iPad mini in portrait) gets the anchored popover in every adaptive presentation instead of the sheet.
| prefersReducedMotion: boolean, | ||
| ): SpringDescription { | ||
| if (prefersReducedMotion) { | ||
| return springs.menuReducedMotion; |
There was a problem hiding this comment.
Under reduced motion with a coarse pointer, current design rule DR2 asks for an opacity-only fade that finishes in 200 ms or less. springs.menuReducedMotion (response 0.4 s, critically damped) is about 82% of the way at 200 ms and settles near 420 ms; the iOS Popover's springs.popoverPresent fade settles near 350 ms.
| @@ -140,6 +140,7 @@ Consumer migration instructions belong in consumer docs and release notes. | |||
| | stray-click window | 400 ms, or the next `pointerdown` | how long after a tracked release the browser's click is still swallowed | `MENU_PRESS_STRAY_CLICK_MS` | `module:DropdownMenu/useMenuPress` | stable | | |||
| | settle time | 300 ms | proposed: how long an opening press must last before its release may act | `MENU_PRESS_SETTLE_MS` | `module:DropdownMenu/useMenuPress` | proposed | | |||
| | long-press delay | 500 ms | proposed: how long a finger rests on the trigger before the menu opens under it | `longPressDelayMs` | `module:DropdownMenu/useMenuPress` | proposed | | |||
There was a problem hiding this comment.
This row still says 500 ms, while useMenuPress.ts defaults longPressDelayMs to 150 ms (menu.holdOpenMs), and 300 ms inside a scrolling region. An owner approving the record would approve a number the code does not ship.
| | FR7 | While a press is tracked in a menu that scrolls, a pointer resting within the edge zone of the menu's top or bottom MUST scroll the menu toward that edge and re-read the row under the pointer as the rows move. | `useMenuPress.test.tsx` autoscroll case | verify | | ||
| | FR8 | On a mouse a DropdownMenu trigger MUST open its menu on press-down (button 0, no Control), and a drag from the trigger into the menu that releases over an enabled row MUST act on it. The opening gesture's release MUST act only if the pointer entered the menu or the press lasted the settle time; otherwise nothing acts and the menu stays open. Pressing the trigger of an open menu MUST close it and MUST NOT reopen it in the same gesture. | Proposed DEC-4; `DropdownMenu.test.tsx` press model suite, `menuPressGesture.test.ts` trigger states | proposed; pending owner review | | ||
| | FR9 | Under a finger a tap on the trigger MUST open the menu through its click. A press held on the trigger for the long-press delay MUST open the menu with the finger down, prevent the default of the finger's later `touchmove`s until the gesture ends, and continue under FR1–FR4 with the same settle rule. | Proposed DEC-4; `DropdownMenu.test.tsx` held-touch case, `useMenuPress.test.tsx` | proposed; pending owner review | | ||
| | FR9 | By default a finger's tap on the trigger MUST open the menu through its click, and a press held for the long-press delay MUST open it with the finger down and prevent the default of the finger's later `touchmove`s until the gesture ends. With touch open `press` the press-down MUST open it at once, and a cancel before the pointer enters the menu MUST close it again. Both continue under FR1–FR4 with the same settle rule. | Proposed DEC-4, DEC-8; `DropdownMenu.test.tsx` touch cases, `useMenuPress.test.tsx` | proposed; pending owner review | |
There was a problem hiding this comment.
Both finger opens are said to continue "with the same settle rule", but under a finger the machine hands off after MENU_PRESS_HANDOFF_PX of travel instead (menuPressGesture.ts:324-368), which no record states.
cd5c2f1 to
1dee9e0
Compare
github-actions
Bot
added
the
needs:design-review
|
|
There was a problem hiding this comment.
Approval waits on one record decision: this pull request carries architecture:layer-runtime INV13 and its Exit copy text from the exit-copy work, which an engineering owner approved for earlier revisions, but this revision's new commit ended that approval, so an engineering owner needs to approve it for this head or move it to #8186 alone. An engineering owner records that decision by commenting /approve-spec ac74241df3d999d2ba3dcb52eb4b96d691df1ebe on this pull request, and the author may do so as an engineering owner. The keyboard cap follows current spec:AST-059 FR8–FR10, and this revision's pinch-zoom and swapped-element reads look right; one change is still needed whatever that decision is:
- Land or rebase the dependency stack so this pull request holds only the keyboard cap (about 2,370 of its 32,028 authored lines) and merges into
mainagain, where the CLI icon-set search goldens now conflict.
[Automated review]
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about spec text this pull request carries from its dependencies: architecture:layer-runtime INV13 and its Exit copy section (from #8186), and two new spec:AST-057 sentences that put the row swipe's slide and spring on theme motion roles, neither of which is on main or approved for this revision. An engineering owner records that decision by commenting /approve-spec 33c21c5efa2c42ed166a9fa00e17eb3d2540c415 on this pull request, and the author may do so as an engineering owner; otherwise INV13 should land through #8186 and the AST-057 sentences go back to main's text. The keyboard cap itself follows current spec:AST-059 FR8–FR10, and two changes are needed whatever that decision is:
- Land or rebase the dependency stack so this pull request holds only the 2,368-line keyboard cap; today 41,264 of its 43,632 authored lines come from fourteen open dependency pull requests.
- Bring
architecture:layer-runtimeINV11, INV12 and its owning code along with the cap, as the current-state impact inspec:AST-059describes, or get an engineering owner's agreement that the record may follow.
[Automated review]
| - **Gesture tuning.** Axis-lock distance, commit ratio, fling velocity, | ||
| resistance past the panel: behavior constants in source, not public API and | ||
| not design tokens (the same ruling the touch press model received for its | ||
| clocks). The row's slide and spring motion plays on theme motion roles | ||
| (`spec:AST-094` FR4). |
There was a problem hiding this comment.
These sentences arrived with the #8186 merge and are not on main: current spec:AST-057 keeps "slide and spring durations" among the swipe's behavior constants ("not public API and not design tokens"), and no row-swipe code is in this pull request. Restore main's text here and under the API table (lines 223-226), unless an engineering owner approves the change for this head.
| correct, and the runtime polls nothing: an anchor that a scrolled | ||
| container hides is followed again when an intersection observer reports it | ||
| back in view. | ||
| - **INV13 — One exit copy per closing layer.** A layer's close plays on at most |
There was a problem hiding this comment.
33c21c5 to
c881018
Compare
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Changes requested: the software-keyboard cap follows spec:AST-059 FR8–FR10 and leaves desktop rendering unchanged, but two shared-path problems in the new hook need fixing first.
- Newly live check: the hook's wait for a press's click (
useLayerKeyboardClearance.ts:99and:264-313) copiesawaitBrowserClickinutils/browserClickWait.ts, so call the shared wait at the lift and delete the copy (architecture:react-component-runtimeINV16). - Newly live check: the every-commit Effect at
useLayerKeyboardClearance.ts:396-399tracks a replaced layer element thatpopoverRefCallbackinuseLayer.tsx(:1547-1575) already receives, so move that work into the callback ref and delete the Effect (architecture:react-component-runtimeINV6).
Two smaller suggestions: on Chrome for Android the cap waits UIKit's 383 ms although Gboard's slide takes 267 ms, and the 56-line file header repeats the hook's JSDoc. The architecture:layer-runtime INV11/INV12 update that AST-059 schedules for this change can follow in its own spec pull request.
[Automated review]
| function handleRelease(event: PointerEvent) { | ||
| if (!pressed.has(event.pointerId)) { | ||
| return; | ||
| } | ||
| if (event.type === 'pointercancel') { | ||
| runDeferred(); | ||
| return; | ||
| } | ||
| if (releaseTimer == null) { | ||
| releaseTimer = view.setTimeout(runDeferred, PRESS_CLICK_GRACE_MS); | ||
| } | ||
| } | ||
| function handleClick() { | ||
| if (pressed.size > 0) { | ||
| runDeferred(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Newly live check: this lift timer and window click subscription re-implement awaitBrowserClick from utils/browserClickWait.ts, Core's one wait for the browser's click after a tap (architecture:react-component-runtime INV16). At the lift, call awaitBrowserClick(layer, {onClicked: runDeferred, onNoClick: runDeferred}), keep pointercancel running runDeferred, and drop PRESS_CLICK_GRACE_MS and the click subscription.
| }, [isOpen, layerRef]); | ||
|
|
||
| // Every commit: the layer element may have been replaced while open. | ||
| useIsomorphicLayoutEffect(() => { |
There was a problem hiding this comment.
Newly live check: this Effect runs after every commit to notice a replaced layer element. popoverRefCallback in useLayer.tsx already receives each new element and has a branch for a replacement while open (:1547-1575), so the listener move and the re-measure can run there (architecture:react-component-runtime INV6), and this Effect can go.
On iOS 26.2 a popover that would run under the software keyboard keeps its top on its source and shrinks so its far edge stays 10 pt above the keyboard; its content scrolls, and it grows back when the keyboard goes down. On main the keyboard covers the lower part of any anchored layer (a Popover, a menu, a Selector's list), and a button there cannot be reached until the keyboard is dismissed. The layer runtime now caps every open anchor-mode layer. useLayerKeyboardClearance, imported through the runtime's touch module LayerTouch.ts, reads the keyboard module's one height (getKeyboardHeight, and the layout guide for timing). It writes the room above the keyboard as a custom property on the layer's own element. The runtime's block-size cap (layerViewportInset.maxBlockSize) folds it in, so the layer box and every surface capped by that definition shrink together. A layer nested in a capped one sets its own value, so it is not clamped. - Timing: a rising keyboard covers the difference on its way up, so the cap comes when it has arrived: UIKit's notification duration after it started, or the end of Gboard's slide in Chrome. A falling one takes it off at once, and a keyboard that changes at once is followed at once. A change under a press inside the layer waits for its click, through the shared wait for the browser's click. - A layer opened with the keyboard up is capped in the frame before its first paint. Its box is read with settledBox, as the fallback order reads it: without the entrance transform. Only a layer whose committed element carries data-layer-anchored is capped. - A layer element the runtime swaps in while open is measured again from its own box before it paints: the runtime's layer ref callback hands it to the cap. The viewport is read in layout px. - The Selector, MultiSelector, Typeahead, Breadcrumbs and ContextMenu lists cap their 300 px at the layer's block-size cap. A panel with a search field is a flex column capped as a whole. - Left as they are: a layer clear of the keyboard, a layer whose bottom the fallback order holds (above its trigger), and a layer in custom or fixed positioning. - A menu's on-demand sheet keeps its keyboard code on demand: the cap imports the keyboard module's own files (lazy-sheet-bundle test).
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, because the pull request is a draft from a fork. Next step: mark it ready for review, and a maintainer adds the ci:run label so CI runs.
The keyboard cap follows spec:AST-059 FR8–FR10: with a stand-in keyboard, the searchable Selector, MultiSelector, Typeahead and the new Keyboard Avoidance popover each end exactly 10 px above it, and six layered stories render pixel-identically on desktop. The shared click wait and the callback-ref handoff resolve the earlier findings.
Non-blocking:
- Miss:
Popover.tsxlines 11–12 add header prose thatarchitecture:knowledge-contractsINV29 keeps out of file headers; consider dropping it. architecture:layer-runtimeINV11 and INV12 still need the keyboard cap thatspec:AST-059schedules for this ship, in a follow-up spec pull request, and the description's note that #8506 records it in INV11 should be corrected.
[Automated review]
| * Like every anchored layer, a popover that would run under the software | ||
| * keyboard shrinks to stay above it (the layer runtime). |
There was a problem hiding this comment.
Miss (non-blocking): architecture:knowledge-contracts INV29 keeps a file header to @input, @output, @position and SYNC in about 12 lines, with no prose. This sentence describes the layer runtime's keyboard cap, which Popover's own code does not implement; consider dropping it.
Main's meta#9963 adds exitAnimations to useLayer.tsx, next to the transform helpers this branch moved to settledBox.ts. The merge keeps the move and main's new helper.
|
/approve-spec 9d4645e |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, because the pull request is a draft from a fork. Next step: mark it ready for review, and a maintainer adds the ci:run label so CI runs.
The keyboard cap follows spec:AST-059 FR8–FR10 and DEC-6, and the merge of main keeps the reviewed change: with a stand-in keyboard, the searchable Selector and MultiSelector, Typeahead, DropdownMenu, a Breadcrumb menu and the new Keyboard Avoidance popover each end exactly 10 px above it, and six layered stories render pixel-identically on desktop.
Non-blocking:
Popover.tsxlines 11–12 add header prose about the layer runtime's cap; Popover's code does nothing for it, so the sentence can go.- The
architecture:layer-runtimeINV11/INV12 update that AST-059 schedules for this ship still needs its own spec pull request.
[Automated review]
A popover opened while a software keyboard was up took its cap in a frame callback the open scheduled. Its surface became a scroll container only once its own overflow measure ran, a frame or more after the cap. Between the two, its content painted past its shorter box, down to the keyboard. The cap now commits in a microtask right after the open's commit, in the task that opens the layer. The runtime has shown the layer by then, and a read places it, as the fallback order's read does. Each layer also writes the overflow its surface takes beside the cap: `auto` while capped, `visible` otherwise. The surface reads it, so it scrolls from the commit that caps it.
|
/approve-spec 4ce27c6 |
There was a problem hiding this comment.
Changes requested: the software-keyboard cap still follows spec:AST-059 FR8–FR10, and the new first-frame fix works in Chromium, but its Safari evidence is out of date.
- The changeset claims Safari on iOS, and this revision moves the read for a popover opened with the keyboard already up into a microtask after the opening commit (
useLayerKeyboardClearance.ts:320-326) and makes Popover's surface scroll from that commit (Popover.tsx:440), while the iOS simulator captures in the description are of an earlier revision; please re-run the "opened with the keyboard already up" check (and the rising case) on iOS at this head and update the Evidence label, sincespec:AST-013FR10 says Chromium and Playwright WebKit do not stand in for Safari.
Non-blocking: the comment at useLayer.tsx:70-72 reads as if every capped layer's surface scrolls through the overflow property, but only Popover's surface reads it.
CI has not run on this head yet: the pull request is a draft from a fork, so it runs once it is marked ready and a maintainer adds the ci:run label.
[Automated review]
| // as the fallback order's read in the same task does. `flushSync` cannot | ||
| // run inside a commit, so the cap runs in a microtask right after it. | ||
| if (getKeyboardHeight(win) > 0) { | ||
| win.queueMicrotask(() => { |
There was a problem hiding this comment.
This moves the read for a layer opened with the keyboard already up from a frame callback to a microtask after the opening commit. The changeset claims Safari on iOS, and the iOS simulator captures in the description are of an earlier revision, so please re-run the "opened with the keyboard already up" check on iOS at this head (spec:AST-013 FR10: Chromium and Playwright WebKit do not prove Safari).
| // inside a layer read it through `layerViewportInset.maxBlockSize`. The | ||
| // content of a capped layer overflows. So the cap also turns its surface | ||
| // into a scroll container, in the same commit, through the overflow | ||
| // property. The content never paints past the shorter box. |
There was a problem hiding this comment.
Non-blocking: only Popover's surface reads --astryx-layer-keyboard-overflow, so this reads broader than it is. Suggest: "The cap also writes the overflow a capped surface takes. Popover's surface reads it, so it scrolls from the commit that caps it."
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, because the pull request is a draft from a fork. Next step: mark it ready for review, and a maintainer adds the ci:run label so CI runs.
The software-keyboard cap follows spec:AST-059 FR8–FR10 and DEC-6, and the iOS Safari captures in the description, now taken at this revision for both open orders, give the changeset's Safari claim the evidence spec:AST-013 FR10 asks for.
Non-blocking: the comments at useLayer.tsx:70-72 and Popover.tsx:11-12 (noted inline earlier) still read broader than the code, and the architecture:layer-runtime INV11/INV12 update that AST-059 schedules should follow in its own spec pull request.
[Automated review]
There was a problem hiding this comment.
Approved pending CI: CI is running on this head and has not finished yet.
The software-keyboard cap follows spec:AST-059 FR8–FR10 and DEC-6, and the new commit only merges main without a conflict: anchored layers keep their top and end 10 px above the keyboard, and desktop rendering is unchanged. On the merged head, the Layer, Popover, menu, Selector, Typeahead and keyboard unit suites, the bundle test and Core's typecheck pass. Two earlier non-blocking comment suggestions still stand (useLayer.tsx:70-72, Popover.tsx:11-12).
[Automated review]
There was a problem hiding this comment.
Approval waits on one engineering-owner decision. The software-keyboard cap itself follows spec:AST-059 FR8–FR10 and DEC-6, its unit, browser and CI checks pass on this head, and desktop rendering is unchanged.
- Miss: the layer's
keyboardCapstyle (packages/core/src/Layer/useLayer.tsx:75-78) declares--astryx-layer-keyboard-max-block-sizeand--astryx-layer-keyboard-overflowin Core'sastryx.css, so both become public rows in Core's API report while no current record admits them (architecture:public-component-apiINV11 and INV25), and an engineering owner (the author may answer as one) either admits them on this pull request or asks for private--_*names, which keep them out of the public surface with no behavior change.
Two earlier non-blocking comment suggestions still stand (useLayer.tsx:70-72, Popover.tsx:11-12).
[Automated review]
| '--astryx-layer-keyboard-max-block-size': cap, | ||
| '--astryx-layer-keyboard-overflow': overflow, |
There was a problem hiding this comment.
These two names are declared in Core's astryx.css by this style, so the API report lists them as public stylesheet API (var --astryx-layer-keyboard-max-block-size (./astryx.css) and var --astryx-layer-keyboard-overflow (./astryx.css)), and no current record admits them (architecture:public-component-api INV11, INV25). If they are runtime state only, private names such as --_layer-keyboard-max-block-size and --_layer-keyboard-overflow keep them out of the public surface; the reads in layerViewportInset.stylex.ts and Popover.tsx, the two LAYER_KEYBOARD_*_PROPERTY constants and the theme registry entries would follow.
The layer runtime writes the software keyboard's cap, and the overflow a capped surface takes, as custom properties on each layer's element. Their public names made them rows of Core's stylesheet API, though no app or theme sets them. They are runtime state, so they take private names: --_layer-keyboard-max-block-size and --_layer-keyboard-overflow. No behavior changes. Two comment notes: the cap's comment names the one surface that reads the overflow, and Popover's file header drops a line that repeated the layer runtime's.
|
/approve-spec 8b6d26a |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet because the pull request is a draft. It already carries the ci:run label, so marking it ready for review starts CI.
The software-keyboard cap follows spec:AST-059 FR8–FR10 and DEC-6, and the rename to private --_layer-keyboard-* properties keeps Core's API report unchanged; both earlier comment suggestions are applied. Non-blocking: the architecture:layer-runtime INV11/INV12 update that AST-059 schedules should follow in its own spec pull request.
[Automated review]
Behavior module: the layer runtime's software keyboard cap.
On
main0588a13, which has the keyboard module (#8559); nothing below it is left. A layer element the runtime swaps in while open is measured again before it paints. A popover opened with the keyboard up is capped, its content clipped, from its first frame. A press inside the layer waits for the browser's click through the shared wait.Why
On a phone, the software keyboard covered the lower part of an anchored layer, such as a Popover, a Selector's list or a menu. Someone typing into its search field could not reach the buttons under the keyboard. Safari's keyboard does not shrink the layout viewport, so the layer runtime's size cap never saw it.
What led to it
We are bringing Astryx's touch components to the behavior of native iOS controls. On iOS, a popover shrinks with its top held so that it ends 10 pt above the keyboard. The owner decided the rule for Astryx (#8208 (comment)), recorded by #8506 (
spec:AST-059FR8–FR10 and DEC-6). AST-059 schedules the matchingarchitecture:layer-runtimeINV11 and INV12 update for when this ships; that update follows in its own spec pull request.Options and tradeoffs
Why this choice
It matches iOS and the owner's decision. It joins the runtime's existing caps, so every layered component gets it from one rule. Lists and menus fold it into their own 300 px.
How to check it
PopoverKeyboardinteraction specs and the list and menu specs pass in Chromium and WebKit.On the Android emulator (Chrome 133 with Gboard), every check passes at this head. One platform limit is under Known limits. The changeset still names iOS only.
Evidence
iPhone 17 Pro simulator, iOS 26.2, Safari with its address bar at the top. After: this head, 4ce27c6, from its preview build. Before:
main0588a13 with this pull request's Keyboard Avoidance story added, built locally, sincemainhas no such story. A pink dot marks each tap.Opening the popover focuses its Title field, so the keyboard comes up. A tap on the popover's heading puts the keyboard away. Before, the popover runs under the keyboard: it spans 68–509 px against the keyboard's top at 349. After, the popover keeps its top and ends 10 px above the keyboard once the keyboard has arrived, its content clipped and scrolling. The cap and the clip land in the same frame. Its height steps back as the keyboard starts down. Nothing slides.
Opened with the keyboard already up (Search notes is focused first). After, the first frame that shows the popover already shows it capped, its content clipped:
First frame of an open with the keyboard up, in Chromium and WebKit with a stand-in keyboard whose top is at 377 px. Before, a frame callback the page started earlier read the popover uncapped (68–509 px). The frame then painted it capped, with its content running past its box. Its surface became a scroll container a frame or more later. Now the cap lands in the task that opens the popover. The surface scrolls from the same commit, so the first frame shows 68–367 px with its content clipped.
Every frame of the iOS captures was checked, 3 takes of each case at this head. In each take, the first frame that shows a popover opened over the keyboard already shows it capped and clipped. In the rising case, no frame shows the content past the shorter box.
Known limits
On Android, Gboard can add its autofill row before Chrome reports the smaller viewport (
visualViewport495 → 447 px). Until Chrome reports it, the popover follows the last reported viewport. For that moment it can sit 20–38 px under the new row.Testing
At 8b6d26a on
main0200dfe:pnpm test:ui): 843 files, 21,166 tests pass.scripts/lazy-sheet-bundle.test.mjs: 4 of 4. A menu's on-demand sheet keeps the keyboard code on demand.PopoverKeyboard,keyboardCappedLists): 14 of 14, twice.pnpm api:report --package core) adds no rows. The cap and the overflow it writes are private custom properties (--_layer-keyboard-*), so they are not stylesheet API. With their earlier public names, the report listed two rows.tsc,CI=true pnpm lint(no errors) and Prettier pass.pnpm review:pre: no findings.