Repository navigation
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
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.
|
| Component | JavaScript | CSS |
|---|---|---|
| core/Lightbox | +49.0KB |
+329B |
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.9KB | 35.1KB | +145B |
Accessibility Audit
Status: No accessibility violations detected.
Visual Regression
57 of 504 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 | 230,823 |
| Chat | Full AI Chat | probe | dark | 229,805 |
| AppShell | With Mobile Nav | probe | light | 217,010 |
| AppShell | With Mobile Nav | probe | dark | 216,960 |
| 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 |
| Chat | Empty State | probe | light | 23,531 |
| Chat | Empty State | probe | dark | 22,622 |
| 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 37 more.
Generated by PR Enrichment workflow | Storybook | Sandbox | View full report
![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 can't be approved yet: sourceFor and the zoom transition are marked pending owner review, and the draft spec:AST-094 requirements they rely on (FR5, FR8 and the rest of FR1–FR23) have no owner decision. An engineering owner records that decision by commenting /approve-spec 70b9c9ce8afd7cab6dd08969809560382cce7d10 on this pull request, and the author can do so as an engineering owner; the decision should settle whether sourceFor ships as a stable prop or under experimental, whether a close may keep the dialog open until the photo lands, and whether the focused-opener fallback may change the default for existing callers. Independently of that decision, these changes are needed:
- Land #8215 and its predecessors first and retarget, so this pull request carries only the zoom-transition commit instead of 34,074 changed lines.
- Closing the viewer while the photo is still opening leaves the thumbnail hidden for good, because
zoomFromSourcesaves the opening'shiddenas the value to restore (zoomFromSource.ts:168-174). - Keep the viewer in the dismissal stack while its dialog is still open (
Lightbox.tsx:552), and give the open/close effect a cleanup for its timer and pending close.
[Automated review]
| for (const animator of fades) { | ||
| animator.play({opacity}, {reducedMotion: 'keep'}); | ||
| } | ||
| const visibility = source.style.visibility; |
There was a problem hiding this comment.
When a close interrupts the opening, this runs while the opening still holds the source at hidden (its finally is a promise callback that has not run yet), so the close saves hidden and restores it when it lands: the thumbnail stays hidden for good, and every later opening of that item keeps it hidden. Recording the source's resting visibility once, while any transition holds it, and letting the last transition restore it would avoid this.
There was a problem hiding this comment.
Right, the close saved the opening's hidden as the value to restore. Fixed in 5586095: a source is hidden by holders and shows again when the last transition holding it ends; under Reduce Motion it never hides. LightboxZoomTransition covers both (both failed before).
| return null; | ||
| } | ||
| const geometry = {source: sourceBox, detail, align: 'center' as const}; | ||
| const close = zoomCloseProgress(); |
There was a problem hiding this comment.
zoomCloseProgress() here starts every close from progress 1 with no velocity, so a close during the opening snaps the backdrop, the controls and the photo to opaque before they fade. zoomCloseProgress({from, velocity}) already supports continuing from the shown progress; the driver never passes it.
There was a problem hiding this comment.
Right, a close during the opening snapped everything opaque first. Fixed in 5586095: the close turns the opening around from its shown progress and velocity, on the opening's geometry. LightboxZoomTransition samples the backdrop every frame after an Escape mid-opening, and it only falls.
| ); | ||
|
|
||
| const {shouldDismissOnCloseRequest} = useLayerDismissal({ | ||
| isActive: isOpen, |
There was a problem hiding this comment.
With the zoom-back close, the dialog stays open after onOpenChange(false) until the photo lands, but isActive: isOpen drops the viewer from the dismissal stack at once, so an Escape in that window dismisses the layer behind the still-visible Lightbox (overlay-dismissal FR1: a member joins the stack “while present”). The hook's docs suggest passing true while the dialog is open, here with presses blocked until the photo lands.
There was a problem hiding this comment.
Right, the viewer left the dismissal stack while its dialog was still open. Fixed in 5586095: it stays active until the photo lands, with presses ignored meanwhile. The new InsideADialog story and LightboxZoomTransition press Escape twice, and the dialog behind stays.
| finish(); | ||
| } | ||
| }); | ||
| }, [isOpen]); |
There was a problem hiding this comment.
This effect starts a decode timer and a pending close continuation but returns no cleanup, so after an unmount mid-transition they still run against the detached dialog and finish() focuses the stored opener. A cleanup that bumps both tokens and clears the timer would release them (react-component-runtime INV3, INV5).
There was a problem hiding this comment.
Right, the timer and the continuation outlived an unmount. Fixed in 5586095: an unmount cleanup bumps both tokens, clears the timer, aborts the transition in flight and lets go of a landed thumbnail. The new MountedWhileOpen story covers a caller that renders the viewer only while it is open.
| next > 1 | ||
| ? t('@astryx.lightbox.zoomedIn') | ||
| : t('@astryx.lightbox.zoomedOut'), | ||
| triggerElementRef.current = document.activeElement; |
There was a problem hiding this comment.
The opener fallback depends on this document.activeElement. An <img> without tabindex never becomes the active element, and Safari does not focus a clicked button or link, so the documented “an image, or a button or link around one” zooms only where the browser focused the control. No test covers the fallback today.
There was a problem hiding this comment.
Right, the fallback only worked where the browser focused the control. Fixed in 5586095: the opener is the click's target (or the button or link around it), else the focused element. LightboxZoomTransition opens the Gallery story from a plain image, and it zooms.
70b9c9c to
8496423
Compare
There was a problem hiding this comment.
This pull request can't be approved yet: sourceFor, the close that keeps the viewer open until the photo lands, and the opener fallback are marked pending owner review, and the draft spec:AST-094 requirements they rest on (FR1–FR23, including FR4, FR11 and FR14's amendments to current records) have no owner decision. An engineering owner records that decision by commenting /approve-spec 849642377981655849e0800f4c86c0354fe82b1a on this pull request, and the author may do so as an engineering owner; it should settle whether sourceFor ships as a stable prop or under experimental, whether a close may hold the dialog open, and whether the fallback may change the default for existing callers. These changes are needed independently of that decision:
- Land #8215 and its predecessors first and retarget, so this pull request carries only the zoom-transition commit instead of 34,495 changed lines.
- A close during the opening leaves the thumbnail hidden for good, because the close saves the opening's
hiddenas the value to restore (zoomFromSource.ts:168-174). - Keep the viewer in the dismissal stack while its dialog is still open (
Lightbox.tsx:552), and give the open/close effect a cleanup for its timer and pending close. - The opener fallback reads
document.activeElement(Lightbox.tsx:801), so it never sees a plain image and misses a clicked button in Safari; take the opener from the activating event or narrow the docs.
[Automated review]
## Maintainer impact When a pull request that adds a pr-a11y contract step merges, pr-a11y breaks on every open pull request that branched before it. #8130 added "Run picker wheel pinch-zoom contract", and #8231's next run failed that step with `Error: No tests found.` Each open pull request hits it on its next push until it rebases, and again on the next such merge. ## Intended invariant A pr-a11y contract runs whenever its spec file exists in the checked-out PR head. Main's steps name only existing spec files. pr-a11y keeps checking out the exact PR head. ## Change Why: a pull request's run takes its workflow from the test merge with main, so it has main's newest steps, but pr-a11y checks out `pull_request.head.sha` (pinned by the provenance test). A contract step runs `pnpm exec playwright test <spec> …`, and a spec added on main after the branch point is not in that checkout. `pr-scope-touches.sh` is loaded from the base for the same reason. - `.github/scripts/skip-absent-specs.sh` defines a bash `pnpm` function. For `pnpm exec playwright test`, it drops spec files absent from the checkout and emits a `::notice` naming each one ("…is not in this pull request's head; rebase onto main to run it"). A step left with no spec passes without running Playwright. Every other pnpm command runs unchanged, and Playwright's exit status still decides the step. - A new pr-a11y step, "Skip contract specs the PR head predates", loads the script from `github.sha`, the test merge commit the steps themselves come from (an older head lacks it, and `pull_request.base.sha` can lag main), and exports it as `BASH_ENV`, so every later bash step gets the wrapper. - The ~58 contract steps are unchanged, leaving open ci.yml edits nothing to conflict with. - If the merge commit cannot be read, the step warns and the contract steps run as before. - Pull requests only; a release check's exact head has every spec. Not covered: a stale head that skips a new contract can merge without running it, as before the contract existed. A contract that main renames or moves after the branch point is skipped the same way. ## Evidence - Failure before the change: #8231's pr-a11y run, "Run picker wheel pinch-zoom contract": `Error: No tests found.`, exit 1. - Success after the change: the same step script, run on main just before #8130 (20bde84) with the wrapper as `BASH_ENV`, prints one notice and "nothing to run", and exits 0. - Product/runtime behavior verified unchanged: CI-only. With every spec present, the command reaches pnpm unchanged (unit test). ## Scope - [x] No intended public API, product behavior, visual direction, or policy change. - [x] Product changes discovered during the work were removed or split. - [x] Public text and artifacts contain no internal Meta context. ## Testing - `pnpm exec vitest run --project node .github/scripts`: 69 files, 1220 tests passed. New or changed: - `skip-absent-specs.test.mjs` (new) runs step scripts with `bash -e` and the wrapper in `BASH_ENV`, against a stand-in pnpm. It covers five cases: - every spec present: the command is unchanged; - one spec absent: it is dropped with a notice; - every spec absent: the step passes without Playwright; - Playwright fails: the step still fails; - other pnpm commands: untouched. - `ci-test-routing.test.mjs`: - every spec a pr-a11y step names exists, so main cannot point at a missing file, and a pull request that deletes a spec its steps still name fails here; - the wrapper step comes before every Playwright step, loads from `github.sha` (not the PR base) and sets `BASH_ENV`; - Playwright steps use bash and do not bypass the function. - `release-check-workflow.test.mjs`: the wrapper step runs on pull requests, not on release checks. - Failure injection: a typo'd spec path, the wrapper step moved after the contract steps, and a wrapper that keeps absent files each fail the matching test. - prettier, shellcheck, `bash -n`, `check-executable-bits`. _Maintainer note: two commits on top (c3a9cb4, cff15ae) fix the review's findings: the heading-links upload no longer expects evidence from a skipped spec, and the wrapper loads from the merge commit._
8496423 to
5288d6f
Compare
There was a problem hiding this comment.
This pull request can't be approved yet: sourceFor, the close that keeps the viewer open until the photo lands, and the opener fallback are marked pending owner review, and the draft spec:AST-094 requirements they rest on (FR1–FR23, including FR4, FR11 and FR14's changes to current records) have no owner decision. An engineering owner records that decision by commenting /approve-spec 5288d6f6f0751e32091d42329804c36b46a85749 on this pull request, and the author may do so as an engineering owner; it should settle whether sourceFor ships as a stable prop or under experimental, whether a close may hold the dialog open, whether the fallback may change the default for existing callers, and whether FR4 or the stack's theme motion roles stand. These changes are needed independently of that decision:
- Land #8215 and its predecessors first and retarget, so this pull request carries only the zoom-transition commit instead of 39,248 changed lines.
- A close during the opening leaves the thumbnail hidden for good, because the close saves the opening's
hiddenas the value to restore (zoomFromSource.ts:168-174). - Keep the viewer in the dismissal stack while its dialog is still open (
Lightbox.tsx:552-555), and give the open/close effect a cleanup for its timer and pending close. - The opener fallback reads
document.activeElement(Lightbox.tsx:806), so a plain image never zooms and a button clicked in Safari does not either; take the opener from the activating event or narrow the docs. - The
zoomMedia.transitionsprings listed as theme roles are solved straight from the constants or never played (zoomTransition.ts:179,:188-189), so a theme override changes nothing beyond the open spring's Reduce Motion fade; resolve them throughresolveMotionor keep them out of the role table.
[Automated review]
| velocity?: number; | ||
| }): {frame: MotionCurve; fade: MotionCurve} { | ||
| return { | ||
| frame: zoomProgress(zoomMedia.transition.closeSpring, 0, options), |
There was a problem hiding this comment.
closeSpring and closeFadeSpring (and openSpring above, outside its Reduce Motion cross-fade) are solved here straight from the constants and reach Animator.play as finished curves, which it plays without resolving any role against the theme. So the zoomMedia.transition.* roles this commit adds to astryx docs motion have no effect on the transition's travel, and dragFollowSpring / dragTrackingSpring have no caller at all. Resolving the springs through resolveMotion against the viewer before solving them, or leaving them out of the registered table, would keep the docs honest.
There was a problem hiding this comment.
Right, the roles were listed but never read. Fixed in 5586095: the open, close and close-fade springs are resolved against the viewer with resolveMotion before they are solved (zoomTransition.test.ts), and the two drag springs no code read left the table; their measurement is in MEASUREMENTS.md.
|
Posted by Cindy's agent on her behalf. Parts of this PR re-implement behavior that already exists, or is being specified, as a shared part of core. Converging keeps one implementation per behavior:
This is about where the behavior lives, not about the PR's intent. |
|
Agreed: one implementation.
|
5288d6f to
5586095
Compare
github-actions
Bot
added
the
needs:design-review
|
Pushed in 5586095: zoomFromSource's own reduced-motion check and its cross-fade timing are gone; the transition takes the animator's default. |
There was a problem hiding this comment.
This pull request can't be approved yet. Four choices have no governing spec on main: experimental.sourceFor, the touch close that keeps the viewer open after onOpenChange(false) until the photo lands, the opener fallback that changes the touch default for callers who omit experimental, and the new architecture:layer-runtime INV13. An engineering owner can provide or identify the governing spec, record approval by commenting /approve-spec followed by the full head SHA (the author may do this as an engineering owner), or say which of the four should change. These changes are needed whatever the decision:
- Make the gesture host attach for every pointer, as
spec:AST-094FR19 and FR43 andarchitecture:react-component-runtimeINV13 require. - Make
scripts/touch-support-bundle.test.mjspass again: its root-entry check fails on this head and passes onmain. - Land the stack first and rebase, so this pull request carries only the zoom transition instead of 51,984 authored lines; the unrelated
spec:AST-057rewording leaves with it.
[Automated review]
| * @position Browser adapter of packages/core/src/gestures: turns real events | ||
| * into GestureArena input, and carries out what the arena decides | ||
| * | ||
| * ONLY WHERE THE PRIMARY POINTER IS COARSE |
There was a problem hiding this comment.
This coarse-only attach contradicts spec:AST-094 FR19 ("Recognizers attach for every pointer, with FR43's exceptions"), FR43 and DEC-21, and architecture:react-component-runtime INV13 ("The gesture host attaches for every pointer, with no touch-only query") on main. Under a fine primary pointer the host installs nothing beyond the per-event and action-group carve-outs, so a mouse, trackpad or pen gets none of these gestures. Please attach for every pointer with FR43's exceptions (wheel and trackpad for a pan, ctrl-wheel for a pinch), or have an owner amend those claims.
| opener instanceof HTMLElement && | ||
| opener !== opener.ownerDocument.body && | ||
| (opener instanceof HTMLImageElement || | ||
| opener.querySelector('img') != null) |
There was a problem hiding this comment.
This fallback gives callers who omit experimental the zoom, a hidden opener and the held close on touch whenever the opener shows a picture. spec:AST-017 FR21 says omitting experimental "preserves the component's stable defaults and behavior", so this needs an engineering owner's decision: keep it as a stable default with FR21 or AST-094 FR25 amended, or move it inside experimental.
| 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 is a new normative invariant in a current record, and the zoom transition does not use placeLayerExitCopy. It needs an engineering owner's approval here, or a spec change of its own.
| - **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.
This rewording of the row swipe's motion is unrelated to Lightbox's zoom. Please move it to its own change, with the AST-094 alignment it comes from.
…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
…ckpad (spec:AST-094 FR43) The zoom stage's pan, double tap, tap and catch attach for every pointer; the pinch stays a finger's, and a ctrl-wheel reaches it through the gesture core (meta#8676). A double-click is the stage's double tap (the zoom core's own mouse path goes), a wheel pans a zoomed photo, a zoomed drag rubber-bands at the edges and a released one coasts. The stage fills the viewer for every pointer. A release follows the pointer that began it: under reduced motion a finger's cross-fades (DR2) and a mouse's, a trackpad's or a pen's completes at once on a fine primary pointer (DR3). Tests: LightboxEveryPointer's zoom proofs (a ctrl-wheel zooms, a wheel pans a zoomed photo, a released drag coasts); the fine-pointer and zoom mode proofs that asserted the old mouse zoom flip to FR43's; the unit tests compare a mouse's double-click with a finger's double tap. Docs in English and Chinese; a patch changeset, and the touch zoom changeset no longer says a mouse keeps the old zoom. Red: under a mouse the double-click zoomed 2x about the centre, a ctrl-wheel did not zoom, a wheel did not pan a zoomed photo, a released drag did not coast and the stage did not fill the viewer; green after.
… of meta#8197 meta#8148 now carries the gesture core of meta#8676 and meta#8665, which attaches recognizers for every pointer and reads the selection lock from the pointer alone. Two adaptations follow: usePager calls locksSelectionFor with the pointer only, and the bare pager's fine-pointer proof expects a mouse drag to page (spec:AST-094 FR43).
… pointer) into meta#8197 The zoom stage keeps this PR's pan and catch conditions on meta#8148's pointer lists, and the zoom core its surface and edge checks while its mouse path goes. The stage's options and layout take meta#8148's ungated forms; the touch presentation stays for the gallery's buttons and arrow keys. The zoom target's fine-pointer proof lists the stage's claimed touches.
…croll (spec:AST-094 FR43) usePager attaches its pan and catch for every pointer instead of only under a coarse primary pointer, and its pan takes a trackpad's or a wheel's horizontal scroll through the gesture core. Lightbox's gallery lets the mouse page too. A click beside a gallery photo with zoom on is the stage's tap for every pointer, after the double-tap interval. Tests: LightboxEveryPointer's paging proofs (a mouse drag sideways and a trackpad's horizontal scroll page a gallery); the paging mode and fine-pointer proofs and usePager's React test that asserted no input under a fine pointer flip to FR43's. Docs in English and Chinese; the every-pointer changeset gains paging, and the paging changeset no longer says a mouse pages only with the buttons. Red: under a mouse a sideways drag and a trackpad's horizontal scroll did not page, the pager attached nothing on a fine pointer and the gallery claimed no touches; green after.
meta#8197 brings the zoom's and the pager's gestures for every pointer. The drag to dismiss keeps its finger and pen pointer list (the zoom's shared list is now every pointer's) and its touch-presentation gating until its own change; its imports drop meta#8197's coarse-pointer read, and the fine-pointer proofs keep meta#8197's gallery proof beside this PR's.
…ose (spec:AST-094 FR43) The drag to dismiss attaches for every pointer (a video keeps its own controls' drags), so a mouse or a pen drag down, or a wheel or trackpad scroll that moves a fitted photo down, closes the viewer. Its release follows the pointer that began it: the stage tells the owner whether a finger released it, and under reduced motion a finger's release cross-fades (DR2) while a mouse's, a trackpad's or a pen's completes at once on a fine primary pointer (DR3), for the photo, the backdrop and the controls alike. Tests: LightboxEveryPointer's dismiss proofs (a mouse drag down closes, one brought back springs back, a pen drag down closes, a wheel notch over a fitted photo closes, a mouse's release under Reduce Motion closes at once); the recognizer unit test and the fine-pointer proof that asserted no drag to dismiss under a fine pointer flip to FR43's. Docs in English and Chinese; the every-pointer changeset gains the drag down, and the drag-to-dismiss changeset no longer says the mouse is unchanged. Red: under a mouse, a pen or a wheel the drag down did not move or close the photo, nothing attached on a fine pointer and a fitted photo claimed no one-finger drags; green after.
…er released it (spec:AST-094 FR43) A drag down that lands the photo in its thumbnail, and the thumbnail's picture that fades in over it, follow the pointer that began the drag, as the straight-down dismissal does: under reduced motion a finger's release cross-fades (DR2) and a mouse's, a trackpad's or a pen's lands at once on a fine primary pointer (DR3). The touch presentation's readers are named where it is read. Tests: the zoom transition's proof that a mouse's drag down under Reduce Motion lands in the thumbnail at once. Red: under Reduce Motion a mouse's drag down into the thumbnail cross-faded for 300 ms before the viewer closed; green after.
…nothing calls The zoom transition plays on the stage's rect morph, so the stage's own play (and firstValues, which only it used) and the zoom transition's element curves (zoomElementCurves, with mapCurve) have no caller left. They go with their test and mock entry, and the comments that named them say what they mean without them.
…erimental keeps the stable viewer (spec:AST-017 FR21) Without experimental.sourceFor, the element that opened the viewer was the opened item's source whenever it showed a picture, so callers who omit experimental got the zoom transition, the hidden opener and the held close on a touch screen. That fallback now lives inside experimental: without the object no item has a source and the viewer opens and closes plainly, and the opener record holds the shared press controller only then. The prop and component docs, the zoom transition changeset (now experimental) and the opener record's header say so; the zoom transition's plain-image proof and the listener test pass experimental where they rely on the fallback, or prove the stable opening without it. Red: without experimental a plain image opener still zoomed the photo out of it on a touch screen; green after.
…er (spec:AST-094 FR43) The zoom stage's header, its enabled flag and the drag geometry still spoke of fingers and pens only, of a mouse whose limits do not give, and of a double-click on the released zoom; the hasZoom JSDoc still limited the pinch and the double tap to a coarse pointer; the stage's styles still said the touch layer ran on a touch screen only (FR19); and the fine-pointer proofs' header still claimed touch work leaves that zoom alone. They now say what the code does: every pointer reaches the recognizers (the pinch is the fingers' and a ctrl-wheel's), the released zoom is the keyboard's, and under Reduce Motion a finger's release cross-fades while the others complete at once on a fine pointer. Comments only.
…ter) into the base of meta#8197
…very pointer (spec:AST-094 FR43) The track's style still said the stage's touches went to it under the touch layer only, the zoom stage's catch still spoke of a touch, and the gallery's navigation comment tied the pager's slide to a finger, where it follows the touch presentation. They now say what the code does. Comments only.
…nter (spec:AST-094 FR43) The zoom stage's header still described a one-finger pan the photo follows, its dismiss flag still said an image on a touch screen, the photo-without-zoom style still said the touch layer took its drags, and the dismissal geometry spoke of a finger. They now say what the code does: a finger's, a mouse's or a pen's drag, or a scroll, drags a photo down to close. Comments only.
…nto its thumbnail when it has one Since this PR a dismissed photo shrinks into its thumbnail when the stage's target gives one, but the header still said there was no thumbnail to return to yet. Comment only.
There was a problem hiding this comment.
This pull request can't be approved yet: experimental.sourceFor with its opener fallback, the close that keeps the viewer open after onOpenChange(false) until the photo lands (on touch, and for a drag-down under any pointer), and the new architecture:layer-runtime INV13 have no governing spec on main. An engineering owner can provide or identify that spec, approve these choices by commenting /approve-spec followed by the full head SHA (the author may do so as an engineering owner), or say which of them should change. These changes are needed whatever that decision is:
- Make
scripts/touch-support-bundle.test.mjspass again; its root-entry case fails on this head and passes on the base branch. - Land the stack this branch carries and rebase, so the pull request holds only the zoom transition, and bring that change under 2,000 lines or explain why it can't be split.
- Correct the changeset and
Lightbox.doc.mjs, which say a mouse closes the viewer at once, though a mouse drag-down lands in the thumbnail.
[Automated review]
|
|
||
| Closing works the same way: the Close button, Escape, a tap beside the photo, or a swipe to another photo and then a close each send the photo back into the thumbnail of the photo showing, and the viewer stays open until it lands. A photo dragged down shrinks into its thumbnail. Under Reduce Motion the photo cross-fades in place. | ||
|
|
||
| With a mouse the viewer opens and closes at once, as before. |
There was a problem hiding this comment.
With experimental.sourceFor, a mouse drag-down flies the photo into its thumbnail while the dialog stays open (about 1.4 s in Chromium), so not every mouse close is instant. Could this say which closes are instant with a mouse (Close, Escape, a backdrop click) and that a drag-down lands in the thumbnail?
| // link around one); a text button opens and closes plainly. Both are | ||
| // experimental: without `experimental` no item has a source, so the | ||
| // viewer keeps its stable opening and closing (spec:AST-017 FR21). | ||
| const isExperimental = experimental != null; |
There was a problem hiding this comment.
Any experimental object, even {}, turns on the opener fallback here, and no option names it. That belongs to the specification decision spec:AST-017 FR21 asks for an experimental Core API.
| 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 adds a normative rule to a current record that main does not state, and Lightbox's zoom never calls placeLayerExitCopy. It needs an engineering owner's /approve-spec on this head, or it can move with the exit-copy work to that work's own pull request.
| // Blocked inside the page 140 ms into the opening, once the photo is | ||
| // opaque, and for 300 ms: a block started from the test lands later in the | ||
| // opening, by a varying amount, and 150 ms left too little to judge. | ||
| const block = await blockMainThread(page, 300, { |
There was a problem hiding this comment.
spec:AST-094's verification row for FR5 asks for an animation that keeps advancing through a 1 s main-thread block; this proof blocks for 300 ms.
… thumbnail The changeset and the experimental option's docs said a mouse opens and closes the viewer at once. It does, except that a photo dragged down lands in its thumbnail under any pointer since meta#8231's every-pointer landing; the English and Chinese docs and the changeset now say so.
|
Thanks for the reviews. The new head, a140e96, answers them:
|
There was a problem hiding this comment.
This pull request can't be approved yet: experimental.sourceFor with its opener fallback, the close that keeps the viewer open after onOpenChange(false) until the photo lands (on touch, and for a drag-down under any pointer), and the new architecture:layer-runtime INV13 have no governing spec on main. An engineering owner can provide or identify that spec, record a decision on these choices by commenting /approve-spec followed by the full head SHA (the author may do so as an engineering owner), or say which of them should change. Whatever that decision is, these changes are needed:
- Make
scripts/touch-support-bundle.test.mjspass again; its root-entry case fails on this head and passes onmain. - Land the stack this branch carries and rebase, so the pull request holds only the zoom transition, and bring that change under 2,000 lines or explain why it can't be split.
Thanks for bringing the release note and docs in line with the mouse drag-down.
[Automated review]
| 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 adds a normative rule to a current record that main does not state (INV14 only names "the one shared exit copy"), and the zoom transition never calls placeLayerExitCopy. It needs an engineering owner's /approve-spec on this head, or it can land with the exit-copy work it belongs to.
| */ | ||
|
|
||
| import {useMemo, useRef, useState, type RefObject} from 'react'; | ||
| import {LongPressGestureRecognizer} from '../gestures/LongPressGestureRecognizer'; |
There was a problem hiding this comment.
These static gesture imports are part of how the gesture modules reach Core's root entry: the "is absent from the root entry" case of scripts/touch-support-bundle.test.mjs fails on this head (16 modules, 67,201 minified bytes) and passes on main. Please keep them out of the root entry's graph, or change that case to the boundary INV13 states with an engineering owner's agreement.
| if (sourceFor != null) { | ||
| return sourceFor(i); | ||
| } | ||
| if (!isExperimental) { |
There was a problem hiding this comment.
Any experimental object, even {}, turns the opener fallback on here, and no option names it. That is part of the specification decision spec:AST-017 FR21 asks for an experimental Core API.
Depends on #8215, and on #8665 and #8676 (gestures for every pointer), which this stack carries. Review only this PR's change: vjeux/astryx@11abd78...a140e96
Why
In Photos, a photo opens by growing out of its thumbnail and closes back into it, so you keep your place in the grid. A Lightbox photo appears and disappears at once, with no link to the thumbnail you tapped.
What led to it
The UIKit-parity work for Astryx (
spec:AST-094): UIKit's zoom transition, on the dismissal (#8215), so a photo dragged down lands in its thumbnail.Options and tradeoffs
experimental.sourceFor), or else the element that opened the viewer, when it shows a picture.Why this choice
Only a compositor copy can be reversed mid-flight. It all runs inside
experimental: without it, a Lightbox keeps its stable behavior (spec:AST-017FR21). A drag-down close reportsonOpenChange(false)when the pointer lifts, as every other close does, and the landing follows the pointer that released it. INV13 in the layer-runtime record belongs to #8186 (the layer runtime's exit copy), which this stack carries: this PR doesn't add it, and the zoom uses its own lifted copy.Owner question:
experimental.sourceFor. It lets a caller name the element a photo grows out of and lands back in, such as one thumbnail in its own grid. Without it, only the element that opened the viewer can be the source, which misses when one button opens a whole gallery. The alternative is to drop the option until a spec names it. My pick: keep it insideexperimental.Owner question: the held close. After a close reports
onOpenChange(false), the viewer stays until the photo lands in its thumbnail (on touch, and for a drag down under any pointer), as Photos does, so the photo returns to its place instead of vanishing mid-flight. The alternative is to remove the viewer at once and skip the landing. My pick: keep it, insideexperimental.How to check it
In the "Gallery" story, which passes
experimental, on a phone:Evidence
Safari on the iOS 26.2 simulator:
Android 15 emulator, Chromium:
The opening's busy proof judged a window too short to judge: it held no frames of a compositor-only control animation. It now blocks inside the page for 300 ms and holds 17 to 18 control frames in both engines.