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
needs:spec-owner-review
meta-cla
Bot
added
the
CLA Signed
4e34b63 to
234d2f6
Compare
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request is on hold for a spec decision. Its head adds spec:AST-094 and new requirements in spec:AST-059 (FR9, FR10, DEC-6), architecture:layer-runtime (INV13, INV11) and the Popover, DropdownMenu, ContextMenu, useMenuPress and Selector records, and main has none of them yet. Approval needs an engineering owner to approve or amend those requirements and record the decision by commenting /approve-spec <full head SHA> on this pull request; the author, who is an engineering owner, may do so. Whatever that decision, three changes are needed:
- Load Popover's touch-only code lazily, or flag and justify its cost: about 14 KB gzip on every Popover import, including every springs-table row (
architecture:component-style-authoringINV6). - Record an INV7 exception for the presentation hold, which adds one element to the open Popover under a finger (43 against 42), or block those touches without the extra element.
- Land the predecessor pull requests first, so that this diff is the four-file refactor instead of about 41,900 authored lines.
[Automated review]
| import {devWarn} from '../utils/devWarning'; | ||
| import type {BaseProps} from '../BaseProps'; | ||
| import {usePopover} from './usePopover'; | ||
| import {usePanOutsideDismiss, usePopoverSurfaceMotion} from './PopoverTouch'; |
There was a problem hiding this comment.
This static import puts the whole touch path (animator, rect morph, springs table, exit copy, hold) in every Popover import: about 14,280 B gzip for PopoverTouch.ts alone, measured with esbuild. architecture:component-style-authoring INV6 flags touch-only JavaScript over 2 KB, so either load it lazily once a coarse pointer is seen, or flag and justify the cost in the description.
| edgeGrowPath, | ||
| type MorphPath, | ||
| } from '../motion/rectMorph'; | ||
| import {registerSpringsTable, springs} from '../motion/uikit/springs'; |
There was a problem hiding this comment.
Importing springs brings the whole table into Popover's bundle: navigation, sheet, alert, switch and keyboard rows. Popover reads only popoverPresent and popoverDismiss, and INV6 says a tuning table ships a component only the rows it uses.
| // hold as inside the popover. | ||
| hold.addEventListener('mousedown', preventDefault); | ||
| hold.addEventListener('touchend', preventDefault, {passive: false}); | ||
| layer.append(hold); |
There was a problem hiding this comment.
During the growth this adds an element to the open layer: 43 elements under a finger against 42 under a mouse, measured in the default story. architecture:component-style-authoring INV7 caps the coarse-pointer layer at the fine-pointer count unless the pull request states an exception with the measured counts and it is recorded as an owner decision.
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsMultiSelector (@astryxdesign/core) · View in Storybook
Popover (@astryxdesign/core) · View in Storybook
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included.
|
| Component | JavaScript | CSS |
|---|---|---|
| core/BottomSheet | +16.0KB |
+2B |
| core/ChatLayout | +17.1KB |
+39B |
| core/DateInput | +15.1KB |
+3B |
| core/DateTimeInput | +15.2KB |
+3B |
| core/Popover | +30.6KB |
+4B |
| core/PowerSearch | +14.9KB |
+4B |
| core/TimeInput | +15.3KB |
+4B |
Bundle Size Summary
Gzip figures measure each package's entry file, not the code an import pulls in.
| Package | Entry (ESM) | Entry gzip before | Entry gzip after | Delta |
|---|---|---|---|---|
| @astryxdesign/core | 4.9KB | 1.2KB | 1.2KB | 0B |
| Package stylesheet | Gzip before | Gzip after | Delta |
|---|---|---|---|
| @astryxdesign/core | 34.6KB | 34.6KB | +26B |
Accessibility Audit
Status: 2 accessibility violation(s) found — 2 serious.
ChatComposer - 1 issue(s)
- 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
- Rule:
color-contrast· Affects 1/19 stories · Learn more - WCAG: 1.4.3 (Level AA)
- Rule:
ChatComposerInput - 1 issue(s)
- 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
- Rule:
color-contrast· Affects 1/18 stories · Learn more - WCAG: 1.4.3 (Level AA)
- Rule:
Visual Regression
55 of 502 shot(s) changed.
A change here is a question, not a failure: check whether the after is the
picture you intended. Record that review on the PR. Baseline maintenance is an
explicit dispatch of CI; this report never rewrites the baseline or adds a release gate.
| component | story | theme | mode | pixels |
|---|---|---|---|---|
| Selector | Indicator Space Bottom Sheet Narrow End RTL | neutral | light | 753,933 |
| Selector | Indicator Space Bottom Sheet Wide Start | neutral | light | 740,072 |
| RichTextEditor | Markdown parity: side by side | probe | dark | 558,840 |
| RichTextEditor | Markdown parity: side by side | probe | light | 540,756 |
| Chat | Full AI Chat | probe | light | 440,175 |
| Chat | Full AI Chat | probe | dark | 438,105 |
| AppShell | With Mobile Nav | probe | light | 217,010 |
| AppShell | With Mobile Nav | probe | dark | 216,960 |
| Chat | Empty State | probe | light | 122,185 |
| Chat | Empty State | probe | dark | 120,076 |
| Collapsible | Dividers | probe | dark | 66,445 |
| Collapsible | Dividers | probe | light | 66,445 |
| AppShell | Playground | neutral | dark | 65,198 |
| AppShell | Playground | neutral | light | 63,788 |
| Chat | Default | neutral | dark | 36,304 |
| Chat | Default | neutral | light | 35,767 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | dark | 15,378 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | light | 14,107 |
| Chat | With Attachments | probe | light | 13,406 |
| Chat | With Attachments | probe | dark | 11,232 |
and 35 more.
Generated by PR Enrichment workflow | Storybook | Sandbox | View full report
There was a problem hiding this comment.
This pull request is on hold for a spec decision: its head adds spec:AST-059 FR9, FR10 and DEC-6 (the software-keyboard cap), architecture:layer-runtime INV13 with its exit-copy section and the reworded INV11 and INV12, and the spec:AST-057 motion edit, none of which main states yet. An engineering owner can approve or amend them by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Whatever that decision, these changes are needed:
- Resolve Popover's touch springs through the theme: a theme's
springs.popoverPresentorsprings.popoverDismisscurrently changes nothing (spec:AST-094FR4). - Load the Popover and layer touch paths lazily, or flag and justify their cost (about 14 KB and 4 KB gzip on every import), and ship each component only its springs rows (
architecture:component-style-authoringINV6). - Record an INV7 exception for the presentation hold (154 elements against 153 under a mouse) or block those touches without the extra element, and create the touch exit's listener only for a touch open (
spec:AST-094FR24). - Give the new
pr-interactionjob a pull-request timeout and add it toci-duration'sneeds, whichmain's CI test now requires, and split out theastryxStylexundefinedVariablesoption. - Land the predecessor pull requests first, so this diff is the four-file refactor instead of about 42,000 authored lines.
[Automated review]
| play: RectMorphPlayOptions, | ||
| velocity: number, | ||
| ) => { | ||
| const progress = progressCurve(spring, {velocity}); |
There was a problem hiding this comment.
The spring goes straight into progressCurve, so a theme's value for the role never reaches the curve: with --motion-springs-popover-present: 2s 0 on the root, resolveMotion reads {duration: 2, bounce: 0}, yet the popover still plays the table's curve. spec:AST-094 FR4 requires a theme value to reach the motion at its start; resolving spring against element here would cover every rect-morph user.
| typeof view?.matchMedia === 'function' && | ||
| view.matchMedia(TOUCH_QUERY).matches; | ||
| isTouchPresentationRef.current = isTouch; | ||
| const motion = motionFor(layer); |
There was a problem hiding this comment.
motionFor runs before the pointer check, so a Popover's first open under a mouse creates the touch motion and its beforetoggle listener. spec:AST-094 FR24 says no touch-only listener is active where the primary pointer is fine; returning before motionFor when the open is not a touch one, and no motion exists yet, avoids it.
| import {layerTextReset} from './layerTextReset.stylex'; | ||
| import {layerStructureReset} from './layerStructureReset.stylex'; | ||
| import {layerViewportInset} from './layerViewportInset.stylex'; | ||
| import {useLayerKeyboardClearance} from './LayerTouch'; |
There was a problem hiding this comment.
This static import puts the keyboard cap in every anchor-mode layer's import, on desktop too: LayerTouch.ts's graph is about 4 KB gzip on its own, and Tooltip grows from 15,057 to 18,859 B. architecture:component-style-authoring INV6 flags touch-only JavaScript over 2 KB per component: load it once a coarse pointer is seen, or flag and justify the cost in the description.
There was a problem hiding this comment.
This pull request is on hold for a spec decision: its head adds spec:AST-059 FR9, FR10 and DEC-6 (the software-keyboard cap) and architecture:layer-runtime INV13 with its Exit copy section and the reworded INV11 and INV12, none of which main states yet. An engineering owner can approve or amend them by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Whatever that decision, these changes are needed:
- Merge
main: 11 motion files conflict with the internal motion roles that landed in #8257, so the pull request cannot merge and CI has not run on this head. - Load the Popover and layer touch paths lazily, or flag and justify their cost: every Popover import grows from about 46 KB to 79 KB gzip, including the gesture host through the presentation hold and every springs row (
architecture:component-style-authoringINV6). - Restore the INV14/INV15 verification row in
docs/architecture/layer-runtime.md, which this head deletes. - Land the predecessor pull requests first, so this diff is the four-file refactor instead of about 41,900 authored lines.
[Automated review]
| */ | ||
|
|
||
| import {useEffect, type RefObject} from 'react'; | ||
| import {getGestureHost} from '../gestures/GestureHost'; |
There was a problem hiding this comment.
This import puts the gesture host, with the pan, tap and hover recognizers, into every Popover import through PopoverTouch.ts: its graph is 29,336 B gzip, measured with esbuild (React and StyleX external). architecture:component-style-authoring INV6 flags touch-only JavaScript over 2 KB; loading the hold's touch gate only once a coarse pointer is seen would keep it out of Popover's static graph.
| | INV8 | `useLayerDismissal.test.tsx`, `layerDismissalInvariants.test.tsx`, and `useFocusTrap.test.tsx` | Current top registered layer is skipped, two layers close, or a blocker leaks through | | ||
| | INV9 | Representative Dialog, ContextMenu, Tooltip/HoverCard, and BottomSheet source/tests | A current local channel silently changes ownership or policy | | ||
| | INV10 | `LayerProvider.tsx`, `LayerProvider.test.tsx`, `useToast.tsx`, `ToastViewport.test.tsx`, and `ToastViewport.modalHost.test.tsx` | Provider begins relocating ordinary layers, Toast fallback loses its current lifecycle, or a default provider renders differently from no provider | | ||
| | INV14, INV15 | Source boundary check over Core for top-layer hosts, exit copies, backdrop elements, lift, and rectangle-morph code outside the runtime and the lift module | A component building its own copy of a mechanism INV14 lists (top-layer mounting, close-request delivery, hold, exit copy, or backdrop element), or re-implementing the lift or the grow | |
There was a problem hiding this comment.
This INV14/INV15 verification row is not re-added in the new table, so INV14 and INV15 (one exit copy, one rectangle morph) lose their only check. If the removal is not intended, restore it beside the new INV11 and INV13 rows.
| ### DEC-6 — The runtime caps a layer above the software keyboard, holding its start edge | ||
|
|
||
| **Reference:** `spec:AST-059/DEC-6` | ||
| **Decider:** pending owner review |
There was a problem hiding this comment.
DEC-6 is pending, while the record still ends "None. Every decision above is the owner's." Listing DEC-6 as an open question until an owner approves it, or naming its decider once approved, keeps the two statements in agreement.
There was a problem hiding this comment.
This pull request is on hold for a spec decision: its head adds spec:AST-059 FR9, FR10 and DEC-6 (the software-keyboard cap) and architecture:layer-runtime INV13 with its Exit copy section and the reworded INV11 and INV12, none of which main states yet. An engineering owner can approve or amend them by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Whatever that decision, these changes are needed:
- Merge
main: six files of the interaction harness and its Playwright config conflict with #8044, so the pull request cannot merge and CI has not run on this head. - Load Popover's touch path lazily, or state and justify its cost: every Popover import grows by about 33 KB gzip, including every springs-table row (
architecture:component-style-authoringINV6). - Reword the current-gaps paragraph of
docs/architecture/layer-runtime.md, which still says no shared exit copy exists, and list AST-059 DEC-6 as open until it is decided. - Land the predecessor pull requests first, so that this diff is the four-file refactor instead of about 41,200 authored lines.
[Automated review]
| Otherwise a layer never paints at a geometry it is about to 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.
INV13 makes the exit copy a shared runtime mechanism, but the current-gaps paragraph further down (line 538) still says "no shared presentation hold, exit copy, runtime backdrop tint, or lift module exists". Rewording that paragraph so it names the exit copy as shipped keeps the record from contradicting itself.
There was a problem hiding this comment.
This pull request is on hold for a spec decision: its head adds spec:AST-059 FR9, FR10 and DEC-6 (the software-keyboard cap) and architecture:layer-runtime INV13 with its Exit copy section and the reworded INV11 and INV12, none of which main states yet. An engineering owner can approve or amend them by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Whatever that decision, these changes are needed:
- Merge
main: seven motion files conflict with the compositor animator from #8055, so the pull request cannot merge and CI has not run on this head. - Load Popover's touch path lazily, or state and justify its cost: every Popover import grows by about 32 KB gzip (
architecture:component-style-authoringINV6). - Make Popover's two springs motion roles, or have an engineering owner record that component-measured springs may stay internal constants (
spec:AST-094FR4). - Reword the current-gaps paragraph of
docs/architecture/layer-runtime.md, which still says no shared exit copy exists, and list AST-059 DEC-6 as open until it is decided. - Land the predecessor pull requests first, so that this diff is the four-file refactor instead of about 40,900 authored lines.
[Automated review]
| import {devWarn} from '../utils/devWarning'; | ||
| import type {BaseProps} from '../BaseProps'; | ||
| import {usePopover} from './usePopover'; | ||
| import {usePopoverSurfaceMotion} from './PopoverTouch'; |
There was a problem hiding this comment.
This static import puts the whole touch path (animator, rect morph, exit copy, presentation hold and the gesture host behind it) into every Popover import: esbuild with React and StyleX external measures 64,917 B gzip at the base and 97,699 B here, 28,260 B of it from PopoverTouch.ts, while the path runs only under a coarse pointer. architecture:component-style-authoring INV6 asks for a lazy load or a stated justification above 2 KB; loading PopoverTouch.ts on the first coarse-pointer open would keep the default import as it was.
| // The popover's two springs are this component's measured values, not | ||
| // motion roles (touch motion stays internal, spec:AST-094 DEC-11): they | ||
| // play as given, so no table is registered and no other row is bundled. |
There was a problem hiding this comment.
spec:AST-094 FR4 says every spring a touch presentation plays "MUST be a motion role named for its use" and that a component "MUST read touch motion through these roles"; DEC-11 keeps the roles internal, it does not exempt a spring from being one. The registered spring roles (motion/uikit/springs.ts:521-534) do not include the popover springs, so once the public set lands Popover's motion would not follow it. Registering the two springs as Popover's roles without bundling the other rows would satisfy FR4 and INV6 together; otherwise an engineering owner would need to record the exception.
| transitions of a layer and its content across the re-placement's display | ||
| reset. | ||
| - `Layer/gestureCounter.ts` owns physical pointer/key gesture identity. | ||
| - `Layer/layerExitCopy.ts` owns the exit copy a touch presentation's close plays |
There was a problem hiding this comment.
This entry and INV13 (line 479) make the exit copy shipped runtime code, but the current-gaps paragraph at line 538 still says "no shared presentation hold, exit copy, runtime backdrop tint, or lift module exists". A reader of that paragraph is told the mechanism this record now owns does not exist; it needs rewording alongside these additions.
| ### DEC-6 — The runtime caps a layer above the software keyboard, holding its start edge | ||
|
|
||
| **Reference:** `spec:AST-059/DEC-6` | ||
| **Decider:** pending owner review |
There was a problem hiding this comment.
DEC-6 is pending, while the record still ends "None. Every decision above is the owner's." (line 578). Listing DEC-6 under Open questions until an owner decides it, or naming its decider then, keeps the record consistent.
bfb1d6b to
45e6ff3
Compare
There was a problem hiding this comment.
This pull request is on hold for a spec decision: its head adds spec:AST-059 FR9, FR10 and DEC-6 (the software-keyboard cap) and architecture:layer-runtime INV13 with its Exit copy section and the reworded INV11 and INV12, none of which main states yet. An engineering owner can approve or amend them by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Whatever that decision, these changes are needed:
- Land the predecessor pull requests first, so that this diff is the four-file refactor instead of about 42,000 authored lines.
- Make Popover's two springs motion roles, or have an engineering owner record that component-measured springs may stay constants (
spec:AST-094FR4). - Load Popover's and the layer's touch paths lazily, or state and justify their cost in the description: every Popover import grows by about 33 KB gzip (
architecture:component-style-authoringINV6). - Make the WebKit run of the FR35 proof (a tap while the popover grows) pass or show on a device that the popover stays open, and list AST-059 DEC-6 as open until it is decided.
[Automated review]
| element: HTMLElement, | ||
| options: Pick<AnimatorOptions, 'now'> = {}, | ||
| ): PopoverSurfaceMotion { | ||
| // The popover's two springs are this component's measured values, not |
There was a problem hiding this comment.
spec:AST-094 FR4 says every spring a touch presentation plays "MUST be a motion role named for its use", with a component's own measured roles joining through the generated motion role table (DEC-5), and that a component "MUST read touch motion through these roles and MUST NOT play a literal". DEC-11 keeps the roles internal; it does not exempt a spring from being one, and SPRING_ROLES in motion/uikit/springs.ts:521-534 does not include these two. Registering them as Popover's roles, without bundling the other rows, would satisfy FR4 and INV6 together; otherwise an engineering owner would need to record the exception.
| import {devWarn} from '../utils/devWarning'; | ||
| import type {BaseProps} from '../BaseProps'; | ||
| import {usePopover} from './usePopover'; | ||
| import {usePopoverSurfaceMotion} from './PopoverTouch'; |
There was a problem hiding this comment.
This static import puts the whole touch path (animator, rect morph, exit copy, presentation hold and the gesture host behind it) into every Popover import: CI's size job on this head measures Popover at 84,305 B gzip against 50,905 B at the merge base. architecture:component-style-authoring INV6 asks for a lazy load or a justification in the pull request above 2 KB of coarse-pointer JavaScript; PopoverTouch.ts defers the cost to an unlanded touch module (#8464), and the description does not mention it.
| expect(log.closed).toEqual([]); | ||
| }); | ||
|
|
||
| test('a tap or a drag that lands while it grows closes nothing, moves no focus and scrolls nothing (spec:AST-094 FR35)', async ({ |
There was a problem hiding this comment.
The description reports this case failing in the webkit project (aria-expanded reads false after the tap during the growth), here and four times out of four on #8223's head. That is the spec:AST-094 FR35 case, on the engine Safari uses: either the hold needs a WebKit fix, or a device run should show the popover staying open and the harness difference be recorded.
| ### DEC-6 — The runtime caps a layer above the software keyboard, holding its start edge | ||
|
|
||
| **Reference:** `spec:AST-059/DEC-6` | ||
| **Decider:** pending owner review |
There was a problem hiding this comment.
DEC-6 is pending, while the record still ends "None. Every decision above is the owner's." (line 578). Listing DEC-6 under Open questions until an owner decides it, or naming its decider then, keeps the record consistent.
…task ended at once, however late the first read WebKit under load from other tasks: the call's task ends at once, another task holds the main thread, and the browser commits the animation after it, dated at the call. Nothing in the call's task delayed it, but the first read still moved its start to the read, about 95 ms late on a loaded box, so whatever chains on its end (the second half of a Reduce Motion cross-fade, a cancel fading on the previous fade's end) ran that much late and the cross-fade missed design:reduced-motion DR2's 200 ms. Fails now: the start moves to 1100.
e7b0676 moved a call's start from rest to its first read whenever WebKit had dated it more than two frames before that read. Under load from other tasks the first read comes late with nothing in the call's task delaying it, so the start moved about 95 ms on a loaded box, and whatever chained on the call's end ran that late: the second half of a Reduce Motion cross-fade, so SearchField's cancel missed DR2's 200 ms. The call's task now records when it ended, at its first microtask (or a read in those microtasks is that time), and only a call whose own task ran past the call by more than two frames, the block e7b0676 fixed, moves its start; a read other tasks delay keeps the browser's start.
…s its start past the block Core Animation dates an animation at the commit, when the calling task ends, however short the block before it. A 20 ms block in the call's task, with WebKit's start dated at the call, left the start at the call: the two-frame bound skipped it. Fails now.
… task, with no threshold Core Animation dates an animation at the commit, when the calling task ends, whatever the block's length. The call's task already records when it ended, at its first microtask, so a start the browser dated before that (WebKit's last frame time before the task) now moves to it: a task that ended at once leaves the start at the call, a blocked one moves it past the block, however short, and a read other tasks delay changes nothing. The two-frame bound, which told a read's own frame from a block before the task's end was known, goes, and the start no longer moves to the read.
…e Popover touch presentation AST-057 takes the dependencies' text, which is main's (meta#8646): the tuning bullet this branch once restored is superseded, and the Popover touch presentation does not touch row actions.
…e Popover presentation hold
…shows at each progress Menus, Popover, the lifted preview, Lightbox's zoom and a sub-menu's parent card grow a surface out of what the finger touched and shrink it back, and each needs the same geometry. morphPath.ts names it: lerpPath, a straight line between two rects with an optional companion opacity; edgeGrowPath, UIKit's popover growth out of a point on its edge (iOS 26.2); each path's restartFrom, how an interrupted motion continues in its own law; and morphCurves, the translate, scale and opacity curves that put an element laid out in the path's box on its rect. travelAt is exported for the rect morph's player. Internal to core; the tests check both laws, their restarts and the curves at every 1/120 s, and that each presentation's model is the same rect at every progress within 0.5 px.
… ChatLayout BottomSheet's useSheetKeyboard moved to keyboard/sheetKeyboard.ts, and it subscribes to the keyboard layout guide and rides Gboard with rideKeyboard as ChatLayout does, but the two modules' headers named only ChatLayout, and BottomSheetPanel's related-files list and the keyboard notes still pointed at keyboardHeight.ts. They now name the hook and its file.
… keeps only its player The paths (lerpPath, edgeGrowPath, morphCurves and their geometry) now live in morphPath.ts, their own pull request, which this one merges. The morph imports them and re-exports them where its callers import them, so no caller changes; its file keeps createRectMorph, present and dismiss, and its tests keep the player's cases (the popover's model stays for the present that turns around on its edge growth). The README row and the changeset say what is left.
…the clock's own resolution 'a dismissal mid-present starts from the box: reads either side of the switch' read the box and switched in a timer task, with the morph on the timeline's clock. In a timer task WebKit can hand out a timeline time a frame stale (11-15 ms on a loaded box) while it samples the running box fresher, so the morph's start sat behind what was on screen: steps of 1.9-4.5 px against the 0.5 px bound (4 in 48 runs). The reads and the switch now run inside a frame callback, where the timeline's time is the frame's. What is left is the clock's resolution: WebKit counts it in whole milliseconds and samples the box between them, up to 0.54 px at this point of the travel. The bound adds the box's travel in one tick of that clock, measured over the frame before the switch, to the subpixel; the restart the proof has teeth against still steps tens of px. 40 of 40 in WebKit at load 95, 10 of 10 in Chromium.
…the Popover touch presentation
…sent's first frame on the path 'on screen, the box keeps moving through the switch without a jump' failed 2 to 5 in 32 in WebKit on a loaded box with a 235-238 px worst step, always between the first two tracked frames: the screencast opened on the last frame composited before the present's first, the box at rest, then showed the present at its start 240 px below. That step onto the path is not the switch, as the proof's notes already say. The proof now tracks from the first frame off the rest position, where the present starts. On both the published head and this one the same frame showed up, so it is the screencast's, not the morph's. 64 of 64 in WebKit at load 88, with the container's process cap lifted (its hang is the harness's).
Once a component in the root entry holds its layer's input (a popover's presentation hold, meta#8223), Layer/inputHold's static imports of gestures/GestureHost and gestures/nativeScroll put eight gesture modules (about 47 KB) in the root entry, against architecture:react-component-runtime INV13. Bundle a component that imports the input hold and require no gesture bytes. Fails while the input hold imports from gestures/.
…e host asks it Layer/inputHold registered a touch gate with the gesture host (getGestureHost(doc).addTouchGate(isInHeldLayer)) and walked the flat tree through gestures/nativeScroll, so any component that holds its layer statically carried the gesture system into its bundle and the root entry (architecture:react-component-runtime INV13). Invert it: the host asks isInHeldLayer for every touch itself (gestures may import Layer), the hold registers nothing, and it walks the flat tree through utils/flatTree, where parentAcrossShadow already lives. addTouchGate stays for other gates. A host test holds a held layer's touches with no gate (it fails if the host stops asking the hold).
…this head The headers of morphPath.ts and its test named rectMorph.ts and rectMorph.test.ts, which arrive with the pull request stacked on this one, and cited 'law M1', which no document defines. They now describe the player without naming its file and cite spec:AST-094 FR5, which keeps touch motion to translate, scale and opacity on the compositor.
The header cited 'law M1' and 'laws M2, M3', which no document defines. It now cites spec:AST-094 FR5 for compositor-only motion and FR6 for an interruption that starts from the presented rect with its velocity, and its paragraph wraps within the line width.
…e input hold imports nothing from gestures/
…ut hold imports nothing from gestures/) into the Popover presentation hold
There was a problem hiding this comment.
This pull request is on hold for a spec decision: its head adds architecture:layer-runtime INV13 with its Exit copy section, coupling rule and verification row, and new spec:AST-094 FR41 sentences for a mouse or pen drag that a recognizer claims, none of which main states yet. An engineering owner can approve or amend that text by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Whatever that decision, these changes are needed:
- Land the predecessor pull requests first, so this diff is the four-file refactor instead of about 47,300 authored lines.
- Make Popover's two springs motion roles, or have an engineering owner record that a component's measured springs may stay constants (
spec:AST-094FR4). - Name the job of the input hold's shield, the element added inside the open popover while it grows, in Popover's record with its element counts (
spec:AST-017FR60). - Restore
spec:AST-057's gesture-tuning text, which now says the row plays motion roles while it still settles onSWIPE_SETTLE_MS, and move theastryxStylextreeshakeCompensationdefault into its own pull request.
[Automated review]
| // the opacity in (design:reduced-motion). | ||
| const handle = morph.present( | ||
| pathOf(measured.surface, collapsed!), | ||
| POPOVER_PRESENT_SPRING, |
There was a problem hiding this comment.
spec:AST-094 FR4 says every spring a touch presentation plays "MUST be a motion role named for its use" and that a component "MUST NOT play a literal"; POPOVER_PRESENT_SPRING here and POPOVER_DISMISS_SPRING at line 238 are played directly, and SPRING_ROLES (motion/uikit/springs.ts:521-534) does not register them. DEC-15 lets a theme's tempo and character transform every role (spec:AST-108 FR3), so these two would never follow it. Registering them as roles measured in Popover's record (DEC-5) and playing them through the resolver would conform.
| for (const type of SWALLOWED) { | ||
| shield.addEventListener(type, swallow, {capture: true, passive: false}); | ||
| } | ||
| layer.append(shield); |
There was a problem hiding this comment.
While a Popover grows under a finger this appends the shield inside the open layer. spec:AST-017 FR60 allows an open layer up to 5 more elements under touch only where "Each added element is an internal node with a job its component's record names"; component:Popover names only the touch sheet's frame and the fallback close control. Naming the shield's job (it takes pointer input while the layer enters, AST-094 FR35) with its element counts would conform.
| - **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 |
There was a problem hiding this comment.
The row still settles on SWIPE_SETTLE_MS (packages/core/src/Item/useSwipeAction.ts:86, :368) and no motion role names it, so this sentence describes motion the row does not have; an earlier commit on this branch removed it for that reason and the merge of #8223 brought it back. Restoring main's text here and at line 223 keeps the record true until the row moves onto a role.
| * | ||
| * Touch-only code a pointer page carries (architecture:component-style-authoring | ||
| * INV6): components use the global touch module once it is installed | ||
| * (`TouchSupport.install()`, proposed in #8464), so per-component lazy swaps |
There was a problem hiding this comment.
architecture:react-component-runtime INV13 states "There is no install call": components import the shared touch modules statically and the bundler keeps one copy per page. Popover's own glue outside those modules is about 1.4 KB gzip, so this note can say the cost is the shared modules' instead of waiting on TouchSupport.install().
| // compiled (the TypeScript transform drops it as unused), the module is | ||
| // never loaded, and its constants never reach the CSS: a media-query key | ||
| // fails the build and a value ships as an undefined custom property. | ||
| treeshakeCompensation: true, |
There was a problem hiding this comment.
This changes the default an app gets from astryxStylex({stylexOptions}) when it omits treeshakeCompensation (StyleX's false before, true now). No record governs these options and nothing ties the change to Popover; it reads better as its own pull request with its changeset.
Depends on #8223 and #8329 (with #8909, #8497, #8340 and their predecessors). Review only this PR's change: vjeux/astryx@45d79b7...39db3ce.
Why
Popover's touch presentation (#8187) keeps its own copy of "grow out of a source rect, shrink back into it": its own scale curves about the contact point, opacity, interruption handling and exit copy. Five components carry such copies, and each fix has to land five times.
What led to it
This comes from the work to make Astryx's touch components behave as native iOS ones do (sheets, menus, gestures and their motion). The rect morph (#8329) is the shared implementation of that motion; this moves Popover onto it.
Options and tradeoffs
Why this choice
The morph produces the same rects frame for frame (below), so Popover gains the shared interruption and exit handling without a visible change (
spec:AST-094FR31).What changes
popoverSurfaceMotion.tsdescribes the popover's motion as anedgeGrowPathfrom the contact point and plays it withcreateRectMorph: translate and scale about the box's corner instead of scale about the contact point, for the same rects on screen. The dismissal plays on the layer's exit copy (#8186), and nothing stays inline once the presentation settles.How to check it
Evidence
The box in every frame of that story, before (#8223's curves) and after (the morph):
Scope
Stack (merge in this order): #8076 → #8186 → #8187 (with #8176) → #8342 → #8587 → #8223, and #8497 → #8329 → this PR.