Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval waits on an engineering owner's decision: drag to dismiss is new Lightbox behavior with no current spec, and it relies on the draft spec:AST-094 this stack adds, whose FR4, FR11 and FR14 also amend current records. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (you may do so yourself as an engineering owner), or says which requirement to change. Independently of that decision, these need fixing before approval:
- Land the earlier PRs of the stack and rebase, so this PR carries only its own commit rather than 32,471 authored lines.
- Restore every page the dismissal faded when the viewer reopens, since reopening a gallery at the item next to the dismissed one currently leaves the current page at
opacity: 0(Lightbox.tsxL684–688). - Restore the viewer when the caller keeps
isOpentrue after a drag dismissal, instead of leaving an open, invisible dialog. - Give the
data-lightbox-chromelayers azIndexso Close and Previous stay above a gallery's pages while the layers fade.
[Automated review]
| fadedRef.current = false; | ||
| const {backdrop, chrome} = dismissFades(); | ||
| for (const animator of backdrop == null ? chrome : [backdrop, ...chrome]) { | ||
| animator.set({opacity: 1}); | ||
| } |
There was a problem hiding this comment.
Reopening at a neighbour of the dismissed item: dismissFades() runs the same [data-lightbox-page]:not([data-current]) query again, so the page that was a faded neighbour at the dismissal and is now current keeps the opacity: 0 the release animated it to, and the viewer opens blank (the Gallery story's thumbnails reach this). Restoring exactly the animators the release faded, or every [data-lightbox-page], fixes it; a proof that dismisses at index 1 and reopens at index 2 would guard it.
There was a problem hiding this comment.
Right, a page faded as a neighbour stayed at opacity 0 once it became current. Fixed in e1a3389: the viewer restores exactly the animators the release faded. LightboxDismiss dismisses at the second item and reopens at the third: opacity 0 before the fix, 1 after.
| animator.animate({opacity}, {spring}, {reducedMotion: 'keep'}); | ||
| } | ||
| }, | ||
| onDismissed: handleClose, |
There was a problem hiding this comment.
If the caller keeps isOpen true after this onOpenChange(false) (the overlay-dismissal family leaves that to the caller), the photo stays below the stage, the backdrop and controls stay at opacity 0 and new drags are refused, while the modal dialog stays open over an inert page. Only a reopen restores it. Restoring the viewer when it is still open after the callback avoids the invisible state.
There was a problem hiding this comment.
Right, a caller that keeps the viewer open got an invisible dialog. Fixed in e1a3389: a frame after onOpenChange(false), a viewer still open puts the photo, the backdrop and the controls back. The new ConfirmClose story (it asks before closing) and LightboxDismiss cover it.
| chromeLayer: { | ||
| position: 'absolute', | ||
| inset: 0, | ||
| pointerEvents: 'none', |
There was a problem hiding this comment.
While the animator writes opacity and will-change: opacity here, this layer becomes a stacking context, so the buttons' zIndex: 1 no longer lifts them above a gallery's positioned pages, which come later in tree order. For the drag, its cancel spring plus 500 ms, and 500 ms after a reopen, taps on Close and Previous land on the page. zIndex: 1 on this layer keeps both layers above the stage.
There was a problem hiding this comment.
Right, a fading layer became a stacking context under the pages. Fixed in e1a3389: the control layers carry zIndex: 1. LightboxDismiss hit-tests the Close button mid-drag in a gallery: a page before the fix, Close after.
e620a7e to
2a31746
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision: drag to dismiss is new Lightbox behavior with no current spec, and it relies on the draft spec:AST-094 this stack adds, whose FR4, FR11 and FR14 also amend current records. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or names the requirement to change. Independently of that decision, these need fixing before approval:
- Land the earlier PRs of the stack and rebase, so this PR carries only its own commit rather than 32,892 authored lines; the rebase also clears
pr-a11y, which fails only because a spec it runs exists onmainbut not on this branch. - Restore every page and caption the dismissal faded when the viewer reopens, since reopening a gallery at the item next to the dismissed one leaves the current page at
opacity: 0(Lightbox.tsxL684–688). - Restore the viewer when the caller keeps
isOpentrue after a drag dismissal, instead of leaving an open, invisible dialog. - Give the
data-lightbox-chromelayers azIndex, so Close and Previous stay above a gallery's pages while the layers fade and for the half second after.
[Automated review]
2a31746 to
13266ce
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision: drag to dismiss is new Lightbox behavior with no current spec, and it relies on the draft spec:AST-094 this stack adds, whose FR4, FR11 and FR14 also amend current records (and FR4, as written, rules out the theme-retunable touch springs the stack ships). An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or names the requirement to change. Independently of that decision, these need fixing before approval:
- Land the earlier PRs of the stack and rebase, so this PR carries only its own commit rather than 37,202 authored lines.
- Restore every page and caption the dismissal faded when the viewer reopens, since reopening a gallery at the item next to the dismissed one leaves the current page at
opacity: 0(Lightbox.tsxL681–694). - Restore the viewer when the caller keeps
isOpentrue after a drag dismissal, instead of leaving an open, invisible dialog. - Give the
data-lightbox-chromelayers azIndex, so Close and Previous stay above a gallery's pages while the layers fade and for the half second after. - Regenerate
packages/cli/test/__golden__/no-icon-set/docs-topics.json, whosemotiontopic skeleton no longer matches the motion docs, sotest-node (1/4)passes again.
[Automated review]
|
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.
|
13266ce to
e1a3389
Compare
github-actions
Bot
added
the
needs:design-review
|
Pushed in e1a3389: the dismissal's release and its cancel take the animator's default and pass |
e1a3389 to
c6329eb
Compare
…s on a touch screen, and the zoom fades the backdrop without its blur sourceFor moves under the experimental options (experimental.sourceFor, stability experimental, Tokenizer's and Drawer's pattern) while the owner question on its API is open. The release note says the transition runs on a touch screen. The opening and the close fade the backdrop without its blur (meta#8215's switch), so a 2-core runner keeps the busy opening's frames; the blur is back once the photo lands. Two browser specs pin it; both failed on the merge alone. The zoom transition spec's non-null assertion becomes a guard.
…ring-back brings it back The pinch close fades the backdrop through the same helper as the drag to dismiss, so meta#8215's switch reaches it: the blur comes off while the pinch moves the backdrop and returns once a spring-back rests. The spec failed with the switch on the drag alone (blur(2px) mid-pinch, both engines).
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about the drag's pointer scope: this head attaches drag to dismiss only under a coarse pointer, while the current spec:AST-094 FR19 and FR43 attach gestures for every pointer (a mouse drag and wheel or trackpad scrolling included) and list no Lightbox exception, and spec:AST-017 FR59 leaves a new fine-pointer feel to an owner decision. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request to keep it touch-only (the author may do so as an engineering owner), or says it should attach for every pointer as FR43 describes. Independently of that decision, these need fixing before approval:
- Land the earlier pull requests of the stack and rebase, so this pull request carries only its own changes rather than 44,239 authored lines.
- Record the stage's top when the pan begins instead of reading
getBoundingClientRect()at the lift (useZoomStage.ts:582,spec:AST-094FR9). - Update the description, which still says the backdrop stops blurring under every pointer, while this head keeps the blur at rest.
[Automated review]
| geometry, | ||
| shown, | ||
| stageElement != null && view != null | ||
| ? view.innerHeight - stageElement.getBoundingClientRect().top |
There was a problem hiding this comment.
This reads layout at the lift: getBoundingClientRect() forces a synchronous style and layout just before the exit spring starts, and spec:AST-094 FR9 asks for "no layout read after the gesture begins". The stage's top can be taken when the pan begins, where measure() already reads layout, and kept with dismissRef for dismissExitY.
| // Touch work, like the zoom's: nothing attaches where the primary | ||
| // pointer is fine (spec:AST-094 FR19). | ||
| isEnabled: isOpen && !isVideo && isTouch, |
There was a problem hiding this comment.
The current spec:AST-094 FR19 says "Recognizers attach for every pointer, with FR43's exceptions", and FR43 lists no Lightbox exception, so this comment cites FR19 for a rule it no longer states. Whether the drag stays touch-only (an FR43 exception) or attaches for every pointer is an engineering owner's decision; either way this comment and the jsdom suite titled after FR19 should cite that decision.
targetRef is not read in this PR's zoom stage (the drag to dismiss, in meta#8215, is its first reader); its unused destructure was a lint warning.
Resolution: this PR's open/close effect (the zoom transition's) replaces the plain one; it takes main's meta#8741 change: with no item there is no dialog, so the effect waits for one (`hasItem` in its guard and its dependencies) and a viewer opened before its first item arrives opens when it does. meta#8741's test failed on this effect without it.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about the drag's pointer scope: this head attaches drag to dismiss only under a coarse pointer, while the current spec:AST-094 FR19 and FR43 attach gestures for every pointer (a mouse drag and wheel or trackpad scrolling included) and list no Lightbox exception, and spec:AST-017 FR59 leaves a new fine-pointer feel to an owner decision. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request to keep the drag touch-only (the author may do so as an engineering owner), or says it should attach for every pointer as FR43 describes. Independently of that decision, these need fixing before approval:
- Land the earlier pull requests of the stack and rebase, so this pull request carries only its own changes rather than 44,239 authored lines.
- Keep a dragged photo above its caption: in a single-item viewer the caption is painted across the photo while it is dragged (
Lightbox.tsxL1244–1250). - Record the stage's top when the pan begins instead of reading
getBoundingClientRect()at the lift (useZoomStage.tsL582,spec:AST-094FR9).
[Automated review]
| // Touch work, like the zoom's: nothing attaches where the primary | ||
| // pointer is fine (spec:AST-094 FR19). | ||
| isEnabled: isOpen && !isVideo && isTouch, |
There was a problem hiding this comment.
This keeps the drag to dismiss touch-only and cites FR19, but the current spec:AST-094 FR19 reads "Recognizers attach for every pointer, with FR43's exceptions", and FR43's exceptions do not name Lightbox (with a mouse a pan also takes wheel and trackpad scrolling), while spec:AST-017 FR59 leaves a new fine-pointer feel to an owner decision. Whether the drag stays touch-only or attaches for every pointer is an engineering owner's call; either way the comment should cite the rule that keeps it.
| {!isGallery && currentItem.caption && ( | ||
| <div | ||
| data-lightbox-caption="" | ||
| {...stylex.props(styles.caption)}> |
There was a problem hiding this comment.
While a drag moves the photo, this caption is painted on top of it: once the drag writes its opacity it becomes its own layer after the photo in paint order, and the photo now draws outside its box. At 50 px of finger travel the caption is still at 0.88 opacity across the photo's lower edge (Chromium, Pixel 7). Could the stage sit above the caption while data-zoom-dismissing is set, below the control layers so Close stays reachable?
| geometry, | ||
| shown, | ||
| stageElement != null && view != null | ||
| ? view.innerHeight - stageElement.getBoundingClientRect().top |
There was a problem hiding this comment.
getBoundingClientRect() here runs at the lift, a layout read after the gesture began, which spec:AST-094 FR9 rules out and the zoom core's own comment (useZoomCore.ts L175) says never happens. measure() already reads the stage's rect when the pan begins (L541), so the stage's top could be kept with the drag's geometry and used here.
…positor-only control On a two-core runner the software compositor and the screencast share the core the blocked main thread spins on, so every compositor animation presents fewer frames there: the proof's frame-rate bound failed 7 runs in 10 pinned to two cores (the pr-interaction red on meta#8215). It now records the interaction harness's control animation in the same window and passes when the photo keeps pace with it (busyWindowProof's verdict); a main-thread animation still fails.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about the drag's pointer scope: this head attaches drag to dismiss only under a coarse pointer, while the current spec:AST-094 FR19 and FR43 attach gestures for every pointer (a mouse drag and wheel or trackpad scrolling included) and list no Lightbox exception, and spec:AST-017 FR59 leaves a new fine-pointer feel to an owner decision. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request to keep the drag touch-only (the author may do so as an engineering owner), or says it should attach for every pointer as FR43 describes. Independently of that decision, these need fixing before approval:
- Land the earlier pull requests of the stack and rebase, so this pull request carries its own 2,236-line change rather than 48,916 authored lines, and show why that change cannot be split further.
- Fix the red
test-node (2/4)check:scripts/touch-support-bundle.test.mjsnow finds the gesture core in Core's root entry through Lightbox. - Conform the stack's gesture host and pager to
architecture:react-component-runtimeINV13 and FR43, which attach for every pointer (GestureHost.tsL11–22,usePager.tsL1252–1254). - Record the stage's top when the dismiss pan begins instead of calling
getBoundingClientRect()at the lift (useZoomStage.tsL582,spec:AST-094FR9). - Keep a dragged photo above its caption: in a single-item viewer the fading caption is painted over the photo (
Lightbox.tsxL1244–1250). - Register the four
zoomMedia.dismisssprings as motion roles, asspec:AST-094FR4 requires.
[Automated review]
| dismiss: { | ||
| // Touch work, like the zoom's: nothing attaches where the primary | ||
| // pointer is fine (spec:AST-094 FR19). | ||
| isEnabled: isOpen && !isVideo && isTouch, |
There was a problem hiding this comment.
This keeps drag to dismiss touch-only, and the comment cites spec:AST-094 FR19 for "nothing attaches where the primary pointer is fine". Current FR19 says the reverse: "Recognizers attach for every pointer, with FR43's exceptions", and FR43's exceptions do not name Lightbox (a pan "also takes wheel and trackpad scrolling"). Keeping it touch-only needs an engineering owner's decision (spec:AST-017 FR59 reserves a new fine-pointer feel to one); either way the comment should quote the current rule.
| geometry, | ||
| shown, | ||
| stageElement != null && view != null | ||
| ? view.innerHeight - stageElement.getBoundingClientRect().top |
There was a problem hiding this comment.
getBoundingClientRect() at the lift is a layout read after the gesture began, which spec:AST-094 FR9 rules out ("no layout read after the gesture begins"). The measure() at the pan's began already reads layout; taking the stage's top there and using it here keeps the release free of layout.
| {!isGallery && currentItem.caption && ( | ||
| <div | ||
| data-lightbox-caption="" | ||
| {...stylex.props(styles.caption)}> | ||
| {currentItem.caption} | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
This caption comes after the photo's box in tree order and has no position or z-index. Once the drag writes its opacity it becomes a stacking context painted after the transformed photo, so the caption is drawn over the dragged photo (CI's lightbox-drag-to-dismiss storyboard shows it 50–100 ms into the drag). Giving the photo's box a stacking order above the caption while data-zoom-dismissing is set would fix it.
| * | ||
| * - Recognizers attach, and the host registers listeners, only while the | ||
| * primary pointer is coarse (`spec:AST-094` FR19, FR24; coarsePointer.ts). | ||
| * Where it is fine, an attach is recorded but nothing is installed: no | ||
| * document, window or element listener, no `touch-action` or other style, | ||
| * nothing delivered, so a finger on a touchscreen laptop gets the | ||
| * browser's defaults. The host follows the query: turning coarse installs | ||
| * it and styles every registration; turning fine cancels every touch in | ||
| * flight and removes it all. A development warning names the first attach | ||
| * under a fine pointer. |
There was a problem hiding this comment.
Current architecture:react-component-runtime INV13 says "The gesture host attaches for every pointer, with no touch-only query", and spec:AST-094 FR19/FR43 and DEC-21 ("Rejected: keeping gestures and press tracking touch-only") agree, so this coarse-pointer-only install contradicts current spec; it predates DEC-21 and belongs to the earlier pull request that adds the host.
| */ | ||
| pageGap: 40, | ||
| }, | ||
| dismiss: { |
There was a problem hiding this comment.
spec:AST-094 FR4: "Every spring, duration and curve a touch presentation plays MUST be a motion role named for its use". This table never calls registerMotionTable, so the four dismiss springs the release and cancel play are not roles and no theme motion knob can reach them; registering them, as springs.ts does for the shared springs, would conform.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about the new architecture:layer-runtime INV13 ("One exit copy per closing layer") and its Exit copy section, which this head writes into a current architecture record that main does not state; an engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or asks for it to be settled in #8186 instead. Independently of that decision, these need fixing before approval:
- Land the earlier pull requests of the stack and rebase, so this pull request carries its own 2,435-line change rather than 54,838 authored lines, and show why that change cannot be split further.
- Fix the red
test-node (2/4)check:scripts/touch-support-bundle.test.mjsnow finds the gesture core in Core's root entry through Lightbox. - Register the four
zoomMedia.dismisssprings as motion roles, asspec:AST-094FR4 requires. - Record the stage's position when the dismiss pan begins instead of calling
getBoundingClientRect()at the release (useZoomStage.tsL609,spec:AST-094FR9). - Say in the changesets and the
onOpenChangedocs that a wheel or trackpad scroll that moves a fitted photo down closes the viewer. - Keep a dragged photo above its caption: in a single-item viewer the fading caption is painted over it (
Lightbox.tsxL1232–1238). - Add fine-pointer evidence before and after (a mouse drag and a wheel notch over a fitted photo, on
mainand on this head), asspec:AST-017FR59 asks.
[Automated review]
| geometry, | ||
| shown, | ||
| stageElement != null && view != null | ||
| ? view.innerHeight - stageElement.getBoundingClientRect().top |
There was a problem hiding this comment.
This getBoundingClientRect() runs at the release, after the gesture began, which spec:AST-094 FR9 rules out ("no layout read after the gesture begins"), and it forces a synchronous layout at the lift. The began branch already calls measure() (L568); recording the stage's top or the exit distance there would remove this read.
| * (`CASpringAnimation` mass 1, stiffness 918.09, damping 60.60), | ||
| * Oct 24 2026 | ||
| */ | ||
| dismissPositionSpring: {mass: 1, stiffness: 918.09, damping: 60.6}, |
There was a problem hiding this comment.
The four dismiss springs are played directly (useZoomStage.ts L617, L633, L640; Lightbox.tsx L809–811), but this table never calls registerMotionTable, so none of them is a motion role as spec:AST-094 FR4 requires, and a theme's motion knobs never reach the drag's settle.
| {!isGallery && currentItem.caption && ( | ||
| <div | ||
| data-lightbox-caption="" | ||
| {...stylex.props(styles.caption)}> | ||
| {currentItem.caption} | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Once the drag writes this caption's opacity, it becomes a stacking context painted after the transformed photo, so its text lies across the moving photo; CI's storyboard of this head shows it 50 ms into a touch drag on the Default story. Keeping the dragged stage above the caption while data-zoom-dismissing is set would fix it.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] Lightbox: a mouse, a pen and a trackpad now use the same gestures as a finger. Drag a photo down to close the viewer; drag sideways, or scroll a trackpad sideways, to move through a gallery; ctrl-wheel to zoom; drag or scroll to pan a zoomed photo, which coasts on when released moving. A double-click zooms in at the click, as a double tap does; the keyboard keeps its 2× zoom about the centre. |
There was a problem hiding this comment.
One wheel notch or trackpad scroll that moves a fitted photo down also closes the viewer (the LightboxEveryPointer proof "a wheel notch that moves a fitted photo down closes the viewer"), but this note lists scrolling only for paging a gallery and panning a zoomed photo. Worth saying so here and in the onOpenChange docs.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision: the stack rewrites the current component:Lightbox record (FR11 so a mouse wheel's notches no longer pan a zoomed photo, FR5 with Motion roles for the zoom settle, and a new AR9 that puts a zoomable viewer's controls on dark backings, a new desktop look that spec:AST-017 FR59 says needs its own owner decision). An engineering owner records it by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner; otherwise the owner names the requirement to change. These still need changes after that decision:
- The diff is 10,529 authored lines because it carries #8147, #8148 and #8197; land them, rebase, and show why the drag's own 2,558-line commit cannot be split further.
- Under a mouse, a pen or a trackpad the committed drag animates the photo away before closing, while FR11 and DEC-2 say the viewer closes at once under a fine pointer (
useZoomStage.ts:641–653). - The four dismissal springs are played as literals instead of named motion roles, against FR5 and
spec:AST-094FR4 (zoomMedia.ts:218–219). - During the drag the caption is painted across the moving photo, because it becomes a stacking context after the transformed photo (
Lightbox.tsx:1413–1420). - The committed release calls
getBoundingClientRect(), a layout read after the gesture began, againstspec:AST-094FR9 (useZoomStage.ts:636). - The restore on open runs in its own Effect (
Lightbox.tsx:946–950) when the existing open branch (Lightbox.tsx:650–654) can do it. - The description shows no fine-pointer evidence before and after, which
spec:AST-017FR59 and Lightbox's Compatibility section require.
[Automated review]
| const handle = animator.animate( | ||
| {x: shown.x, y}, | ||
| { | ||
| spring: zoomMedia.dismiss.dismissPositionSpring, | ||
| velocity: {x: velocity.x, y: velocity.y}, | ||
| }, | ||
| {crossfadeAs: 'exit', touchRelease: fingerRef.current.dismiss}, | ||
| ); |
There was a problem hiding this comment.
Under a fine primary pointer this still plays the exit spring, and onOpenChange(false) waits for it. component:Lightbox FR11 says "the viewer opens and closes at once", and its verification row says "a fine-pointer close that animates fails". Where the release is not a finger's and the primary pointer is fine, close at the lift with no exit spring, as the reduced-motion path does.
| geometry, | ||
| shown, | ||
| stageElement != null && view != null | ||
| ? view.innerHeight - stageElement.getBoundingClientRect().top |
There was a problem hiding this comment.
getBoundingClientRect() at the release is a layout read after the gesture began (spec:AST-094 FR9). measure() already records the stage's origin when the pan begins (useZoomCore.ts:207–210); compute the exit distance from that.
| registerMotionTable('zoomMedia', zoomMedia, { | ||
| roles: ['settle.baseMs', 'settle.lnRatioMs', 'settle.maxMs'], |
There was a problem hiding this comment.
Only the settle lengths become roles. The four dismiss.* springs are then played as literals (useZoomStage.ts:644, :660, :667; Lightbox.tsx:895–897), against Lightbox FR5 ("dismiss" is a named role) and spec:AST-094 FR4. Register them here and read them through resolveMotion, then add them to the record's Motion roles table.
| useIsomorphicLayoutEffect(() => { | ||
| if (isOpen) { | ||
| restoreAfterDismissal(); | ||
| } | ||
| }, [isOpen, restoreAfterDismissal]); |
There was a problem hiding this comment.
This is a second Effect for the open event: the existing open/close Effect's open branch (:650–654) already handles it and clears --_lightbox-backdrop-filter there, and this Effect's comment names no external system (architecture:react-component-runtime INV3). Call restoreAfterDismissal() in that branch and drop this Effect.
| {!isGallery && isRenderable(currentItem.caption) && ( | ||
| <div | ||
| ref={captionRef} | ||
| data-lightbox-caption="" | ||
| {...stylex.props(styles.caption)}> | ||
| {currentItem.caption} | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Once the drag writes the caption's opacity, the caption becomes a stacking context painted after the transformed photo, so its text lies across the moving photo. Keep the stage above the caption, and below the control layers, while data-zoom-dismissing is set.
| // The backdrop dims and blurs the page as on main: the theme's overlay | ||
| // colour over a 2 px blur. While a drag to dismiss and its release fade | ||
| // it, the blur comes off through the dialog's | ||
| // --_lightbox-backdrop-filter. On a 2-core limit, a full-viewport blur | ||
| // repainted every frame of the fade drops frames. |
There was a problem hiding this comment.
Non-blocking (INV29): "as on main" narrates history. Suggest: "The backdrop is the theme's overlay colour over a 2 px blur. A drag to dismiss and its release remove the blur through --_lightbox-backdrop-filter. A full-viewport blur repainted every frame of the fade drops frames."
There was a problem hiding this comment.
Approval waits on an engineering owner's decision. The stack rewrites the current component:Lightbox record: FR11 so a mouse wheel's notches no longer pan a zoomed photo, FR5 with Motion roles for the zoom settle, and a new AR9 that puts a zoomable viewer's controls on dark backings, a new desktop look that spec:AST-017 FR59 says needs its own owner decision. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner; otherwise the owner names the requirement to change. These still need changes after that decision:
- The diff is 10,399 authored lines because it carries #8147, #8148 and #8197; land them, rebase, and show why the drag's own 2,556-line commit cannot be split further.
- Under a mouse, a pen or a trackpad the committed drag animates the photo away and closes the viewer about 0.4 s after the release, while FR11 and DEC-2 say the viewer closes at once under a fine pointer (
useZoomStage.ts:640–653). - In a gallery the dragged photo stays clipped by its page's photo box, because
pageMediakeepsoverflow: hiddenwhile only the single-item stage lets the photo draw outside (Lightbox.tsx:316). - The four dismissal springs are played as literals instead of named motion roles, against FR5 and
spec:AST-094FR4 (zoomMedia.ts:218–219). - During the drag the caption is painted across the moving photo, because it becomes a stacking context after the transformed photo (
Lightbox.tsx:1358–1364). - The committed release calls
getBoundingClientRect(), a layout read after the gesture began, againstspec:AST-094FR9 (useZoomStage.ts:636). - The restore on open runs in its own Effect (
Lightbox.tsx:939–943) when the existing open branch (Lightbox.tsx:630–634) can do it. - The description shows no fine-pointer evidence before and after, which
spec:AST-017FR59 and Lightbox's Compatibility section require.
[Automated review]
| spring: zoomMedia.dismiss.dismissPositionSpring, | ||
| velocity: {x: velocity.x, y: velocity.y}, | ||
| }, | ||
| {crossfadeAs: 'exit', touchRelease: fingerRef.current.dismiss}, |
There was a problem hiding this comment.
Under a fine primary pointer this still plays the exit spring, and onOpenChange(false) waits for it: probed with a mouse, the photo travels from 162 px to 801 px and the dialog closes 419 ms after the release. component:Lightbox FR11 says "the viewer opens and closes at once", and its verification row says "a fine-pointer close that animates fails". Close at the lift with no exit spring when the release is not a finger's and the primary pointer is fine, as the reduced-motion path already does.
| geometry, | ||
| shown, | ||
| stageElement != null && view != null | ||
| ? view.innerHeight - stageElement.getBoundingClientRect().top |
There was a problem hiding this comment.
getBoundingClientRect() at the release is a layout read after the gesture began (spec:AST-094 FR9). measure() already records the stage's origin when the pan begins (useZoomCore.ts:207–210); compute the screen's bottom from that.
| */ | ||
| export function registerZoomMediaTable(): void { | ||
| registerMotionTable('zoomMedia', zoomMedia, { | ||
| roles: ['settle.baseMs', 'settle.lnRatioMs', 'settle.maxMs'], |
There was a problem hiding this comment.
Only the settle lengths become roles. The four dismiss.* springs are then played as literals (useZoomStage.ts:644, :660, :667; Lightbox.tsx:889–890), against Lightbox FR5 ("dismiss" is a named role) and spec:AST-094 FR4. Register them here and read them through resolveMotion.
There was a problem hiding this comment.
New motion roles are barred by the owners' rule, so these four stay unregistered. The drag to close is a gesture exit: its springs will read the existing gesture roles once #9706 lands, and the record's Motion roles table will name only those.
| display: 'flex', | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| overflow: 'hidden', |
There was a problem hiding this comment.
In a gallery the stage is this page box, so data-zoom-dismissing lands here, but the box keeps overflow: hidden: 140 px into a mouse drag the photo reaches 866 px while the box ends at 768 px, so its bottom is cut off. Give it the same ':is([data-zoom-dismissing])': 'visible' override as imageWrapper, and run the clipping test on the gallery story.
| animator.set({opacity: 1}); | ||
| } | ||
| }, [setBackdropMoving]); | ||
| useIsomorphicLayoutEffect(() => { |
There was a problem hiding this comment.
This is a second Effect for the open event: the open/close Effect's open branch (:630–634) already handles it and clears --_lightbox-backdrop-filter there, and this Effect's comment names no external system (architecture:react-component-runtime INV3). Call restoreAfterDismissal() in that branch and delete this Effect.
| <div {...stylex.props(styles.caption)}> | ||
| {!isGallery && isRenderable(currentItem.caption) && ( | ||
| <div | ||
| ref={captionRef} |
There was a problem hiding this comment.
Once the drag writes the caption's opacity, the caption becomes a stacking context painted after the transformed photo, so its text lies across the moving photo. Keep the stage above the caption, and below the control layers, while data-zoom-dismissing is set.
| backgroundColor: 'transparent', | ||
| overflow: 'hidden', | ||
| outline: 'none', | ||
| // The backdrop dims and blurs the page as on main: the theme's overlay |
There was a problem hiding this comment.
Non-blocking (INV29): "as on main" narrates history. Suggest: "The backdrop is the theme's overlay colour over a 2 px blur. A drag to dismiss and its release remove the blur through --_lightbox-backdrop-filter. A full-viewport blur repainted every frame of the fade drops frames."
There was a problem hiding this comment.
Approval waits on an engineering owner's decision. This head rewrites the current component:Lightbox record (FR11 so a mouse wheel's notches no longer pan a zoomed photo, FR5 with the zoom settle's length roles, and a new AR9 that puts a zoomable viewer's controls on dark backings, a new desktop look that spec:AST-017 FR59 says needs its own owner decision). It also ships behavior no current clause settles: the four drag-to-close springs stay literals while FR5 and spec:AST-094 FR4 require motion roles, the drag fades the controls, caption and neighbouring photos and drops the backdrop's blur where FR8 names only the backdrop's fade, and arrow-key panning now stops at the photo's edges. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner; otherwise the owner names the requirement or behavior to change.
These still need changes after that decision:
- Desktop behavior change needs an owner decision: with default props, a mouse or pen drag down a fitted photo, or a trackpad scroll, now closes the viewer, and a mouse drag or sideways trackpad scroll pages a gallery (
Lightbox.tsx:914,Lightbox.tsx:1012-1016,useZoomStage.ts:342-348); gate these to touch input or cite the owner decision that covers them. - The diff is 10,603 authored lines because it carries #8147, #8148 and #8197, and no size exemption applies; give all four descriptions the same
Stack:line with #8147's head under #8148's, or land the three and rebase, since the branch also no longer merges cleanly withmain.
The earlier animated desktop close, gallery clipping, caption stacking and release layout read are fixed.
[Automated review]
| dismiss: { | ||
| // Every pointer's, like the zoom's. A video keeps its own controls' | ||
| // drags. | ||
| isEnabled: isOpen && !isVideo, |
There was a problem hiding this comment.
Desktop behavior change needs an owner decision: this attaches the drag to close for every pointer with default props, so a mouse or pen drag down a fitted photo, or a trackpad scroll, closes the viewer where main does nothing (the pager's panOptions at line 1013 does the same for paging). Gate it to touch input, or cite the owner decision that covers this desktop behavior.
| } | ||
| }, []); | ||
|
|
||
| // Opening again after a dismissal: the photo, the backdrop and the |
There was a problem hiding this comment.
Non-blocking (comment style): "Only then: a write promotes the layers" is a fragment. Suggest: "Restore the photo, the backdrop and the controls only when the viewer opens after a dismissal. Each write promotes a layer, and releasing it later repaints during a gesture."
|
/approve-spec 6a6505e |
The UIKit constants table gains zoomMedia, measured on iOS 26.2 in a UIKit lab app and in Photos: the give past the zoom limits, the eased settle and its length law, and Photos' double tap and maximum zoom. The settle's three lengths are internal motion roles. component:Lightbox names them in a new Motion roles table, and the generated motion role table carries them. Lightbox/zoomStage.ts is the pure geometry of a zooming scroll view in a photo viewer, checked against those numbers. A double tap zooms to the larger of filling the stage and 2x, about the tapped point, as component:Lightbox FR6 asks. Nothing renders the stage yet: Lightbox's touch zoom plays it in the next change.
Lightbox's zoom moves to the gesture recognizers and the compositor animator. A pinch zooms about the fingers with the pan riding along, and it gives past the zoom limits and eases back about the last centroid. A finger's double tap zooms to the larger of filling the viewer and 2x, about the tapped point, over the theme's move easing and medium duration. A mouse's or a pen's double-click zooms 2x about the clicked point. In the touch presentation a zoomed pan has UIScrollView's rubber band, coast and edge bounce. A mouse's or a pen's drag pans 1:1 and stops at the edges and at the release, as before. The zoom takes every pointer: a ctrl-wheel or a trackpad pinch zooms, and a trackpad's scroll pans a zoomed photo; a mouse wheel's notches never pan. The photo's stage fills the viewer and takes the pinch; the controls and caption keep the page's pinch-zoom. Since a zoomed photo can now lie under Close, Previous, Next and the counter, a zoomable Lightbox sets them on its own translucent backing and keeps their icons and text on the on-dark colour, so they keep 3:1, and 4.5:1 for the counter's text, over any photo in every theme. The keyboard keeps its 2x zoom about the centre over the released 200 ms ease-out, and keyboard panning stops at the photo's edges. The double tap reads its tokens through the shared theme-timing reader in motion/themeTiming.ts. A theme that leaves --duration-medium unset gets Core's default duration there, not a zoom that lands at once.
A Lightbox gallery moved only through its two arrow buttons. A finger's drag or flick now pages between items with UIKit's paging release: one page at most, with Photos' measured 40 pt gap, settled on the compositor. The drag paging is part of the touch presentation, which a coarse primary pointer brings (spec:AST-094 FR13); under a fine pointer the buttons and the arrow keys page, as before. A zoomed photo keeps the drag until its edge, as nested scroll views do, and a pinch never pages. A video pages from its frame. Its native controls are taken as a 48 px band at its bottom edge, since browsers expose no control geometry, so a drag that starts there stays the video's (component:Lightbox FR7). Only the current page takes focus and is in the accessibility tree. Next, Previous and any other change of `index` slide the row under the touch presentation; under a fine pointer the row moves at once, so a button the owner refuses shows nothing. A drag the owner refuses springs back. Under Reduce Motion they fade. On a zoomable gallery the photo rows take the stage's touches while the track allows the page's pinch-zoom, so a pinch on a caption still zooms the page. The pan and the pinch read where their touches began from the shared recognizers, and without zoom a tap beside a gallery's photo closes the viewer through a shared tap recognizer.
A Lightbox photo closed only through its Close button or a tap beside it. Dragged down, a fitted photo now follows the drag, shrinks, and the backdrop fades with it. The release closes the viewer when the drag's projected end lies more than 10 pt below its start, so a short flick closes and a drag brought back springs back. The release animates on the compositor. The drag to close is part of the touch presentation, which a coarse primary pointer brings (spec:AST-094 FR13). Where the primary pointer is fine, a mouse's, a pen's or a trackpad's drag leaves the photo in place, as before. The backdrop's blur comes off while the photo moves and returns at rest, since a full-screen blur every frame drops frames on low-end phones. Under Reduce Motion the photo fades out. The close is reported when the photo has left the screen, about 0.3-0.5 s after the lift; component:Lightbox names that gap until the shared exit hold lands.
|
/approve-spec 67076bd |
Batch: B7 Lightbox
Stack: #8147 → #8148 → #8197 → #8215, #9810 (both on #8197)
Depends on #8197, and on #8665 and #8676 (gestures for every pointer), which this stack carries. Review only this PR's change: vjeux/astryx@dc3a360...67076bd
Rebased with the stack on #8197's dc3a360 (main f10bc91). Since 6a6505e, the head Cindy passed card 40 on:
At 67076bd on box 6:
pnpm install --frozen-lockfile0; coretsc0; strict ESLint on every changed file 0;CI=true pnpm lint0 errors; the Lightbox, motion, gestures and hooks unit files 2,673/2,673;pnpm check:knowledgeandpnpm check:changesetspass; a root build and a Storybook build of this head, then Chromium Lightbox a11y specs 25/25, Chromium Lightbox interaction specs 96/96 (4 WebKit-only skips), WebKit 91/91 (9 Chromium-only skips).spec:AST-094FR4 and FR45 for the dismissal's and the cancel's measured springs, by the spec owners' ruling;@playwright/testa lint error in browser specs, so the Dismiss and DismissCancel specs importtestandexpectfrom@astryxdesign/a11y-spec/playwright, as main's do;Lightbox.tsxand fails with a draft that set the flag only for a zoomed drag.At de1e56f:
pnpm install --frozen-lockfileexits 0; coretscis clean, and so are strict ESLint andCI=true pnpm linton the changed files; the Lightbox,usePager, motion/uikit and gestures unit files pass 1325/1325;pnpm check:knowledgeandpnpm check:changesetspass; on a root and a Storybook build of it, Chromium Lightbox a11y specs 25/25, Chromium Lightbox and hook interaction specs 118/118 (6 WebKit-only skips) and WebKit 112/112 (12 Chromium-only skips).Earlier: rebased on #8197's 2b36122 (main 5a373df). Since 9896c40, the head that went up for the iPhone check, this pull request's own lines change only with the stack below it:
useZoomStage's options directly, as feat(Lightbox): touch zoom like the iOS Photos viewer #8148 now does, instead of a ref an Effect kept copied;useZoomStage's header names the two options this pull request adds (the current item's key and the drag to close).Against 11abd78:
spec:AST-094FR13), as the spec owners ruled. Where the primary pointer is fine, a mouse's, a pen's or a trackpad's drag leaves the photo in place, as on main; FR11 keeps its approved text, and the record's Current-state list names the drag to close as not built for a fine pointer;Size
This pull request's own commit is about 2,500 authored lines: about 1,750 are tests, about 700 source, and the rest stories, changeset, measurements and the record line. The drag and its release are one gesture: the follow, the shrink and the fades only make sense with the release that closes or springs back, and both releases share one decision and one set of measured springs. Most of the size is the proof against Photos' measured curves for the follow and both releases, a blocked main thread, Reduce Motion and every pointer.
Why
On a phone, the only ways out of a Lightbox photo are a small Close button or a tap beside it. In Photos, people drag the photo down: it follows the finger, shrinks, the background fades, and letting go closes it, or brings it back if the drag returns.
What led to it
The UIKit-parity work for Astryx (
spec:AST-094), after zoom (#8148) and paging (#8197): Photos' dismissal, with its shrink and fade measured in Photos.Options and tradeoffs
Why this choice
It behaves like Photos and keeps moving when the page is busy. The cost is 1.6 KB of gzip JavaScript (95,206 → 96,776 bytes).
Record change
component:Lightbox's Current-state impact list: FR8 leaves the touch-presentation line, which now names only the pinch to close, the tap to hide the controls and the thumbnail transition. A new line names FR8's implementation gap: "Drag to close (FR8). Implementation gap: the close is reported when the photo has left the screen, about 0.3–0.5 s after the lift, until the shared exit hold lets the viewer stay up after a reported close." The fine-pointer line ("A mouse drag on a fitted photo does not close the viewer") goes, since a mouse's drag now closes it. No requirement's text changes. The record needs a spec owner's approval.Writing rubric results
Run over
docs/specs/components/Lightbox/Lightbox.spec.md:How to check it
In the "Gallery with zoom" story, on a phone:
Evidence
Safari on the iOS 26.2 simulator:
Fine pointer, before and after (desktop Chromium, a mouse, "With zoom"; at rest, 120 px into a drag down, and 120 ms after the release): the drag to close is touch only, so a mouse drag leaves the photo in place and the viewer open on both.
main:
this pull request:
Android 15 emulator, Chromium:
The cancel's busy proof judged too short a window in WebKit: 5 to 6 frames of a compositor-only control animation, and fewer on a busy machine. It now pulls farther and blocks inside the page, for 13 to 14 frames; the dismissal's proof holds 8 to 10.
Testing
At 6a6505e on #8197's 2b36122 (main 5a373df):
pnpm install --frozen-lockfileexits 0; coretscis clean, and so is strict ESLint on every changed file; INV29 finds no long sentence;usePager, motion/uikit and gestures unit files pass 1312/1312;pnpm check:knowledgeandpnpm check:changesetspass;pinch-zoom). "A photo dragged to dismiss draws past the box it was fitted in" covers the single photo and the gallery;