Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
the
needs:design-review
|
AI review status for this pull request.
|
![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: the UIKit motion foundation and the zoom geometry rest only on draft spec:AST-094, so an engineering owner (the author qualifies, as an engineering owner) needs to approve or amend its DEC-1, DEC-2, FR1–FR3 and FR5–FR6, together with the amendments it proposes to current records, by commenting /approve-spec <full head SHA> on this pull request. Independently of that decision, two changes are needed:
- Run the new interaction proofs inside an existing CI owner job or the required
testjoin instead of the separately routedpr-interactionjob, whichspec:AST-030FR11 and FR7 do not allow. - Rebase or retarget so this pull request carries only its own zoom commit once its predecessors land; as issued it carries 13,842 authored lines of the stack.
[Automated review]
| # them, so a change to an instrument is proven by its negative controls | ||
| # before a component spec relies on it. Chromium only on these runners; | ||
| # see the harness README for running WebKit. | ||
| pr-interaction: |
There was a problem hiding this comment.
This job routes itself (Check interaction scope) and no join requires it: the test join and .github/scripts/ci-test-join.mjs still list only test-ui, test-node, test-build, registry-contract and test-native-swift. spec:AST-030 FR11 says new coverage inside an existing surface must join those owners and must not create a separately routed lane or required context, and FR7 says an owned lane's failure must fail its join. Hosting these steps in an existing owner job (as pr-a11y hosts the other Chromium guards) or requiring this job from the test join would conform.
| * it rests with no gap at either edge. All constants come from | ||
| * motion/uikit/zoomMedia.ts and motion/uikit/scroll.ts. | ||
| * | ||
| * SYNC: When modified, update zoomStage.test.ts and useZoomStage.ts. |
There was a problem hiding this comment.
useZoomStage.ts is not in the tree at this head (nor is the useZoomStage named on line 12), so this SYNC line points at nothing; point it at a file that exists, or add the hook together with its consumer.
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsLightbox (@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. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: No accessibility violations detected. Visual Regression55 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 35 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
8bdfcde to
64a8d76
Compare
github-actions
Bot
removed
the
needs:design-review
There was a problem hiding this comment.
This pull request is on hold for a spec decision: the zoom geometry and the motion and gesture foundation it carries rest only on draft spec:AST-094, whose FR4 and DEC-1 keep touch motion out of themes, while this head also ships theme motion roles and a platform adaptation condition outside the closed vocabulary of spec:AST-012 FR2. An engineering owner (the author qualifies, as one) records the decision by commenting /approve-spec <full head SHA> once the record and the code agree, or asks for the record to be amended first. Independently of that decision, these changes are needed:
- Rebase or retarget so this pull request carries only its own zoom commit once its predecessors land; as issued it carries 29,365 authored lines of the stack.
- Run the interaction proofs inside an existing CI owner job or the required
testjoin instead of the separately routedpr-interactionjob, whichspec:AST-030FR11 and FR7 do not allow. - Mark
zoomMedia@experimental, since it is exported asuikit.zoomMediafrom@astryxdesign/core/motion/experimentalandspec:AST-017FR21 requires the marker on every declaration there.
[Automated review]
|
|
||
| import {registerMotionTable} from '../roles'; | ||
|
|
||
| export const zoomMedia = { |
There was a problem hiding this comment.
zoomMedia is reachable as uikit.zoomMedia from @astryxdesign/core/motion/experimental (export * as uikit in motion/experimental.ts), but unlike the other declarations there it has no @experimental doc block, which spec:AST-017 FR21 requires ("Declarations carry @experimental"). Adding the doc block, and naming it in the uikit comment and in motion/experimental.test.ts, would conform.
There was a problem hiding this comment.
Right, FR21 asks for the marker on every declaration the experimental subpath exposes. Fixed in ff5cf88: zoomMedia carries a doc block with @experimental, the uikit comment names it, and motion/experimental.test.ts checks both (the doc-block check failed before).
There was a problem hiding this comment.
| ); | ||
| } | ||
|
|
||
| const ease = cubicBezier(zoomMedia.settle.curve); |
There was a problem hiding this comment.
This fixes zoomMedia.settle.curve when the module loads, but the same curve is registered as the motion role zoomMedia.settle.curve and listed by astryx docs motion as something a theme can retune, so no theme value can reach the settle. Either keep the curve out of the role registry or let settleCurves take the curve resolved for the element, whichever the spec:AST-094 FR4 decision calls for.
There was a problem hiding this comment.
Right, the role was listed but no theme value could reach the settle. Fixed in ff5cf88: settleCurves takes the curve the element's theme resolves for zoomMedia.settle.curve (from #8148 on, the shared zoom stage, useZoomCore.ts, passes resolveMotion(stage, zoomMedia.settle.curve)). zoomStage.test.ts runs a linear theme value through it and failed before.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] `presentation="adaptive"` on DropdownMenu, ContextMenu, MoreMenu, Selector and MultiSelector chooses the bottom sheet on a compact touch screen: a coarse primary pointer and a viewport narrower than 744px or shorter than 500px (it was 768px wide and below). An iPad mini in portrait keeps the anchored popover; a phone in landscape now gets the sheet. |
There was a problem hiding this comment.
The same query also decides sub-menu drill-in (useMenuDrillIn), and a sub-menu's presentation defaults to adaptive, so with no opt-in sub-menus now drill in on a phone in landscape and fly out on 744–768 px touch tablets. Worth naming here beside the root presentation.
64a8d76 to
ff5cf88
Compare
github-actions
Bot
added
the
needs:design-review
ff5cf88 to
b4db6a7
Compare
There was a problem hiding this comment.
This pull request is on hold for its governing spec: the zoom geometry and the motion, gesture and theme-motion changes it carries rest on spec:AST-094, which is still proposed in #8378 and not on main, and no record yet covers the press-feedback selection lock or the new undefinedVariables build option. An engineering owner (the author is one) needs to land that record on main, or identify other current spec for these additions. Independently of that decision, these changes are needed:
- Rebase or retarget so this pull request carries only its own zoom commit once its predecessors land; as issued it carries 34,536 authored lines of the stack.
- Run the interaction proofs inside an existing CI owner job or the required
testjoin instead of the separately routedpr-interactionjob, whichspec:AST-030FR11 and FR7 do not allow.
[Automated review]
| @@ -2,7 +2,15 @@ | |||
| "extends": "../../tsconfig.json", | |||
| "compilerOptions": { | |||
| "outDir": "./dist", | |||
| "rootDir": "./src" | |||
| "rootDir": "./src", | |||
| "paths": { | |||
There was a problem hiding this comment.
These paths let Lab typecheck imports of core modules that core's exports do not list (@astryxdesign/core/gestures/* includes the velocity tracker). A Lab file using them would fail for every Lab consumer, and architecture:interaction-modality INV10 says Lab cannot import the tracker. No Lab file uses them at this head; dropping the mappings here and in tsconfig.build.json would conform.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] Under a finger or pen, holding a control or a cell that shows the touch press (the press feedback surfaces) no longer starts text selection or the iOS callout, as on UIKit controls. Selection and the callout come back when the finger lifts. Text fields keep their selection, a link keeps its native menu and preview, and a mouse keeps desktop selection. |
There was a problem hiding this comment.
spec:AST-017 FR7 reserves [fix] for corrections; turning off selection and the callout on every touch press surface is a new behaviour no current record asks for, so it reads as a capability ([feat]) once its authority is settled.
| '@astryxdesign/build': patch | ||
| --- | ||
|
|
||
| [fix] `astryxStylex` resolves `defineConsts` values read from another file. With the `{stylexOptions}` form (the one Storybook uses), a file that read only constants from a `.stylex.ts` module lost that import once StyleX compiled it, so the constants never reached the CSS: a constant used as a media-query key failed the build ("Invalid empty selector") and one used as a value shipped as a custom property nothing declares. A constant defined on the library side of the layer split and read on the product side stayed unresolved in either form. A production build now also warns, naming the variable and the rule that reads it, when its stylesheet reads a StyleX-generated custom property that nothing declares; set `undefinedVariables: 'error'` to fail the build instead. |
There was a problem hiding this comment.
Besides the defineConsts correction, this [fix] adds a capability: the undefinedVariables option and a default production warning. spec:AST-017 FR7 tags compatible capabilities [feat], so splitting it into the fix and a [feat] for the option would conform.
| /** | ||
| * Registers the table's springs and curves as motion roles a theme may | ||
| * retune; the limits, scales and duration law are geometry. Idempotent: | ||
| * Lightbox's touch module (Lightbox/useZoomStage.ts) calls it before its |
There was a problem hiding this comment.
Lightbox/useZoomStage.ts does not exist at this head and nothing calls registerZoomMediaTable() yet; zoomStage.ts:12 has the same useZoomStage reference. Naming the module that will call it once it lands keeps the pointer true.
b4db6a7 to
a33800d
Compare
|
On the review's points:
|
There was a problem hiding this comment.
This pull request is on hold for its governing spec: the zoom geometry and the motion, gesture and theme-motion changes it carries rest on spec:AST-094, which is still proposed in #8378 and not on main, and no record covers the press-feedback selection lock or the new undefinedVariables build option. An engineering owner (the author is one) needs to land that record on main, recording the decision with /approve-spec <full head SHA> on the pull request that carries it, or identify other current spec for these additions, and settle the selection lock and the build option too. Independently of that decision, these changes are needed:
- Rebase or retarget so this pull request carries only its own zoom commit once its predecessors land; as issued it carries 37,668 authored lines of the stack.
- Run the interaction proofs inside an existing CI owner job instead of their own routed
interaction-scopeandpr-interactionlane, whichspec:AST-030FR11 does not allow. - Drop Lab's
tsconfigpaths and the docsite and Sandbox resolvers for core modules core does not export: no Lab file uses them, and the gestures mapping reaches the velocity tracker Lab may not import. - Add real-browser evidence for the moved adaptive edges (852 × 393 and 768 × 1024 with a coarse pointer), as
spec:AST-013FR10 requires.
[Automated review]
| echo 'applies=true' >> "$GITHUB_OUTPUT" | ||
| fi | ||
|
|
||
| pr-interaction: |
There was a problem hiding this comment.
The test join now requires this job, which meets spec:AST-030 FR7, but the proofs still run in a lane of their own with their own routing job (interaction-scope, line 3759). FR11 says new coverage inside an existing surface "MUST join those owners and MUST NOT create a separately routed lane or required context", and the same record names pr-a11y as the pull-request owner that keeps fast interaction guards. Running these steps inside that owner, or inside test-ui, would conform.
| 'springs.alertDismiss': 'spring', | ||
| 'springs.alertPresent': 'spring', | ||
| 'springs.banner': 'spring', | ||
| 'springs.bouncy': 'spring', |
There was a problem hiding this comment.
Once released, a role id is theme input a theme pins, and this one (like springs.swiftUIDefault, springs.uikitDefault and timing.curves.caDefault) names a platform preset rather than a use. architecture:theme-tokens INV3 asks for purpose-led names, and #8378's FR4 renames these presets (springs.emphasis for .bouncy), so a theme that sets them now would break when that record lands.
| content: [ | ||
| { | ||
| type: 'prose', | ||
| text: 'Touch motion has a table of springs, durations and curves measured on iPhone and iPad. Each value is a motion role named by its table path, such as `springs.sheetPresent` or `timing.curves.easeInOut`. A theme retunes a role through `motion.springs`, `motion.durations` or `motion.curves`, at its root or in an adaptation rule; a role it leaves alone keeps its measured default. A motion reads its roles as it starts, so a theme change applies from the next motion.', |
There was a problem hiding this comment.
No shipped component calls resolveMotion or the animator at this head, so a theme that follows this section writes variables nothing reads yet. Saying so here and in the [feat] changeset, or shipping the fields with the first component that reads them, would keep the docs true.
|
Posted by Cindy's agent on her behalf. Cindy decided touch text selection (recorded in #8454, This PR's selection lock should follow that rule: suppress selection only on hold-driven elements, not on every pressable. vjeux, could you update the PR to match? |
…it/c24-zoom-geometry
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on three deliberate departures from the current spec:AST-094 (the author may make it, as an engineering owner): either amend the record, and architecture:react-component-runtime INV13 where it repeats the rule, on main through a spec pull request approved there with /approve-spec <full head SHA>, or direct this code to conform.
- FR4: the zoom settle's length (0.1 s + 0.3 s × |ln zoom ratio|, at most 0.4 s) stays plain constants instead of motion roles.
- FR19 and FR24: a press tracker attached with
perEventPointerregisters touch listeners and paints a finger's press where the primary pointer is fine, which the code itself calls an exception pending an amendment. - FR19 and FR24: an element attached with
actionGrouphands mouse, pen and finger presses to its recognizers where the primary pointer is fine, citingfamily:action-groupsFR5, which is not onmain.
Whatever that decision, please reduce this pull request to its 941-line zoom commit: the other 30,270 lines are open predecessor work, including two released [fix] changes (astryxStylex and the press controller) that the description's "Public delta and caller burden: None" does not mention.
[Automated review]
| export function settleSeconds(fromScale: number, toScale: number): number { | ||
| const {baseSeconds, secondsPerLnRatio, maxSeconds} = zoomMedia.settle; | ||
| if (!(fromScale > 0) || !(toScale > 0)) { | ||
| return baseSeconds; | ||
| } | ||
| return Math.min( | ||
| maxSeconds, | ||
| baseSeconds + secondsPerLnRatio * Math.abs(Math.log(toScale / fromScale)), | ||
| ); | ||
| } |
There was a problem hiding this comment.
The settle's length reads baseSeconds, secondsPerLnRatio and maxSeconds straight from zoomMedia.settle. Current spec:AST-094 FR4 says every spring, duration and curve a touch presentation plays MUST be a motion role, with a component's own measured roles joining through the generated table (DEC-5). This waits on an engineering owner: amend FR4 (or land component:Lightbox's roles) on main, or register these values as roles.
| * only (an FR19 exception for press feedback only, pending in | ||
| * `spec:AST-094`'s amendment): no control event, nothing kept from the |
There was a problem hiding this comment.
perEventPointer gives a press tracker passive touch listeners and paints a finger's press where the primary pointer is fine, which this comment calls an FR19 exception pending an amendment. Current spec:AST-094 FR19/FR24 and architecture:react-component-runtime INV13 allow a per-event pointer read only for spec:AST-057's row swipe, so this waits on an engineering owner's ruling.
| /** | ||
| * A press on an action group under a fine pointer (`family:action-groups` | ||
| * FR5): the group's recognizers and consumer take it, nothing else on the | ||
| * path does (`spec:AST-094` FR19), and the document listens only until it | ||
| * lifts (FR24). Where the primary pointer is coarse the host's own | ||
| * listeners serve the group, as every other recognizer. | ||
| */ | ||
| private beginGroupPress( |
There was a problem hiding this comment.
beginGroupPress hands mouse, pen and finger presses to an action group's recognizers and consumer where the primary pointer is fine, citing family:action-groups FR5, which is not on main. Current FR19, FR24 and INV13 register no listener there, so this waits on an engineering owner's ruling.
| if (next === 'cancelled') { | ||
| this.releaseClaim(recognizer, this.movingIds); |
There was a problem hiding this comment.
Keeping a moving finger claimed after its recognizer cancels itself means cancelIfClaimed (GestureHost.ts:1650-1666) keeps cancelling that finger's touchmoves until it lifts, while no recognizer owns it. Current spec:AST-094 FR19 says a touchmove of a touch no recognizer owns MUST NOT be cancelled, with three listed exceptions, and this is not one of them; FR14, which the comment above cites, decides who takes a drag that starts inside a scroll area. Either the record gains this exception or the claim is released.
| * @file zoomMedia.ts | ||
| * @input Nothing; data only | ||
| * @output Exports `zoomMedia`: UIScrollView | ||
| * zooming as a photo viewer uses * it — the overshoot past the zoom limits, the eased curve and duration |
There was a problem hiding this comment.
A stray * it sits inside this @output sentence.
| overshoot past the limits, the settle curve and its duration law, the absence | ||
| of scale momentum and of a two-finger-tap action, the content point under the | ||
| centroid staying put, Photos' aspect-fill double tap and its maximum zoom | ||
| (M-ZM2). A zoomed pan flicked into an edge coasts and bounces like any scroll |
There was a problem hiding this comment.
This lists the maximum zoom as done "(M-ZM2)", but line 287 lists M-ZM2 as a capture still needed; one of the two should change.
| * @input The source of utils/pressFeedback.ts and every module it imports | ||
| * @output The press feedback controller's import graph reaches no module | ||
| * under gestures/: a page whose only touch work is press feedback carries | ||
| * none of the gesture system (`spec:AST-094` FR24, FR43: pointer queries |
There was a problem hiding this comment.
Current spec:AST-094 has no FR43, so this cites a requirement that is not on main.
| * @position Core's own barrel. Not exported from the package; Lab imports it | ||
| * through internal workspace imports. |
There was a problem hiding this comment.
architecture:react-component-runtime INV13 has Core and Lab reach the touch modules through @astryxdesign/core/internal/touch, not through internal workspace imports.
|
Posted by Cindy's agent on her behalf: closed and reopened only to restart this PR's CI on the current workflow after tonight's queue-wide cancel; nothing in the PR changed. |
…base of meta#8147 A busy-window proof can judge a component against a compositor-only control animation recorded in the same window (startBusyControl, CONTROL_RGB, busyWindowProof's control option), instead of a frame-rate bound that a two-core runner's software compositor cannot meet.
…interaction harness's busy-window control) into meta#8148
…ery pointer, wheel and trackpad scrolling; the harness's pooled busy proof) into meta#8147
…every pointer, wheel and trackpad scrolling; the harness's pooled busy proof) into meta#8148
Depends on #8320 (the gesture host) and #8340 (an animator fix). Review only this PR's change: vjeux/astryx@f5ae6b7...848f031
Why
Lightbox's touch zoom (#8148) has to feel like the iOS Photos viewer, which people compare it with on their own phones: a pinch past the maximum gives and eases back, and a double tap fills the screen. A hard stop at the limit, or a bouncy spring, shows at once. This PR adds the measured values and the geometry #8148 builds on; nothing renders differently yet.
What led to it
The UIKit-parity work for Astryx measures each touch behavior in a small UIKit test app and in Photos (iOS 26.2 simulator) and records every number with its source (
spec:AST-094FR1–FR4). The zoom is the first Lightbox piece.Options and tradeoffs
Why this choice
Measured values with their source can be checked (a test enforces a source on every entry), and pure geometry is testable without a browser. One guess remains and is flagged: the zoom when the photo already fills the screen (2×).
What is in it
motion/uikit/zoomMedia.ts: past a maximum M the zoom reads M·(2 − M/s), s the pinch's scale; past a minimum m, m ÷ (2 − s/m); the settle timing above; a maximum of 6 times the photo's pixel size.Lightbox/zoomStage.ts: where a zoomed photo rests, the transform that keeps a point under the fingers, the drag's rubber band, the double-tap and release targets, and the settle as compositor animations.How to check it
Nothing changes on screen in this PR. With #8148 on top, in the "With zoom" story on a phone:
The unit tests pin each measured number (
pnpm vitest run packages/core/src/Lightbox packages/core/src/motion, all passing).Evidence
import {Lightbox}(gzip): JavaScript 52,420 → 52,679 bytes, CSS unchanged.[chore]changeset.