Repository navigation
feat(Lightbox): touch zoom like the iOS Photos viewer - #8148
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
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval is on hold for an owner decision: the new draft spec:AST-094 (FR1-FR23, DEC-1/DEC-2) is the only authority for the Lightbox touch zoom and its UIKit gesture and motion foundations. An engineering owner records the decision by commenting /approve-spec ccbbc4f4e1c93024e4e6a337053cb2204a42f906 on this pull request; the author may do so if they are an engineering owner. Whatever that decision, these need changes:
- Split the 27,757 authored lines (gesture recognizers, motion foundation, interaction harness, Lightbox zoom) into reviewable pull requests.
- Under Reduce Motion, a second zoom or pan key during the cross-fade leaves the photo dimmed (to 28% opacity with
+then-150 ms apart), even after reopening, becausesettleToinuseZoomStage.tsalso stops the opacity fade. - On touch, a Lightbox with
hasZoomoff no longer closes on a backdrop tap: the stage now stretches for every item, though the changeset says nothing changes there. - The new
pr-interactionjob adds a separately routed lane for component tests, whichspec:AST-030FR11 rules out.
[Automated review]
| reducedMotion: AnimateOptions['reducedMotion'], | ||
| ) => { | ||
| const from = current(); | ||
| animator.stop(); |
There was a problem hiding this comment.
Under Reduce Motion this stop() also freezes the opacity of the cross-fade dip animator.play started, and the next dip returns to that frozen value. Pressing + then - within the 500 ms fade leaves the photo dimmed (0.277 opacity with the keys 150 ms apart), and it survives closing and reopening. Settling to an explicit opacity of 1 (and resetting opacity with the item) would fix it.
| alignSelf: {default: null, '@media (pointer: coarse)': 'stretch'}, | ||
| flexGrow: {default: null, '@media (pointer: coarse)': 1}, | ||
| minHeight: {default: null, '@media (pointer: coarse)': 0}, |
There was a problem hiding this comment.
This coarse-pointer stretch (and the one on imageWrapper) applies to every item, so with hasZoom off or a video a tap on the dark area lands on the stage instead of the container, and handleBackdropClick no longer closes the Lightbox. Gating it to the zoom stage keeps the changeset's "With hasZoom off, nothing changes." true.
| ## Touch presentation | ||
|
|
||
| Whether a component presents itself the touch (UIKit) way is answered once, | ||
| in `../utils/touchPresentation.ts`: |
There was a problem hiding this comment.
../utils/touchPresentation.ts and the test said to enforce it do not exist at this head; Lightbox writes its own @media (pointer: coarse) instead.
There was a problem hiding this comment.
At this head ../utils/touchPresentation.ts is there, with touchPresentation.test.ts beside it: both come with #8076, which this stack carries through #8320. Lightbox reads the touch presentation through useTouchPresentation() from that module, one model on every touch device, and the stage's layout keys on its isTouch, with no media query of its own.
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| Lightbox · Storybook | 🔴 102.5 kB (+43.0 kB) · 9.8 kB CSS (+96 B) | 2,481 (+1,355) | ✓ · ✓ · ✓ |
| lab/CommentThread | 🔴 294.5 kB (+19.6 kB) · 2.4 kB CSS (0) | — | — · — · — |
| lab/Comments | 🔴 299.9 kB (+19.2 kB) · 2.5 kB CSS (0) | — | — · — · — |
Package size: @astryxdesign/core 6.70 MB packed (+23.7 kB); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 19 rows, core 21 rows; run pnpm api:report.
Touched components checked; add ci:run-visual or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | View full report
ccbbc4f to
54ebf13
Compare
There was a problem hiding this comment.
This pull request needs an engineering owner's decision on the new draft spec:AST-094 (FR1–FR23, DEC-1 and DEC-2, including its proposed amendments to other records) and on the Lightbox zoom built on it before it can be approved. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, or by saying which requirements should change; the author may do this if they are an engineering owner. Independently of that decision, these need fixing:
- Under Reduce Motion, pressing
+then-quickly leaves the photo at about 25% opacity, even after closing and reopening, becausesettleTostops the opacity fade along with the zoom (useZoomStage.ts:259). - On touch screens a Lightbox without
hasZoomno longer closes on a backdrop tap, because the coarse-pointer stage stretch applies to every item (Lightbox.tsx:162-180). - The new
pr-interactionjob is a separately routed lane, whichspec:AST-030FR11 does not allow, so the interaction proofs should join the existing browser test owner. - The diff carries about 27,900 authored lines from the unmerged stack, so please land or rebase the predecessor pull requests until this one holds only the Lightbox change, and drop the README and MEASUREMENTS passages that describe files not in the pull request.
[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 interaction coverage of existing surfaces (core source and Storybook) through its own scope check, which spec:AST-030 FR11 does not allow: "New coverage inside an existing surface MUST join those owners and MUST NOT create a separately routed lane or required context." Could the proofs run as a step of the existing browser test owner, as the Lightbox arrow-key contract already does, or be admitted through an AST-030 amendment (FR10)?
| where a switch outside any scroll view is needed. | ||
|
|
||
| Done (iOS 26.2 simulator, Oct 23 2026; in the switch's own table, | ||
| `uikit/switch.ts`): M-SW02, M-SW03, M-SW04, M-SW05, M-SW06, M-SW09 (UISwitch |
There was a problem hiding this comment.
uikit/switch.ts (here) and uikit/contextMenu.ts (line 254) are neither in this pull request nor on main; please drop these references until those tables land.
54ebf13 to
a467b57
Compare
There was a problem hiding this comment.
This pull request needs an engineering owner's decision on the new draft spec:AST-094 (FR1–FR23, DEC-1 and DEC-2, with its proposed amendments to other records) and on the public changes built on it: the Lightbox zoom, the two experimental subpaths, the new adaptive breakpoint, and the theme motion roles, which the record's own FR4 and DEC-1 currently rule out. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, or by saying which requirements should change; the author may do so if they are an engineering owner. Independently of that decision, these need fixing:
- Under Reduce Motion, pressing
+then-quickly leaves the photo at about 30% opacity, even after closing and reopening, becausesettleTostops the opacity fade with the zoom (useZoomStage.ts:269). - On touch screens a Lightbox without
hasZoomno longer closes on a backdrop tap, because the coarse-pointer stretch applies to every item (Lightbox.tsx:166-181). experimental-isolation.test.mjsfails on a staledocs-topics.jsongolden, and the motion docs, theme template and changesets describe motion roles and foundation usage that no component has yet.- The
platformadaptation condition needs aspec:AST-012FR2 amendment, and thepr-interactionjob should join the existing browser test owner (spec:AST-030FR11). - Please land or rebase the stacked predecessors so this diff (about 31,800 authored lines) holds only the Lightbox change, and ship the theme, adaptive-presentation and experimental-export changes separately.
[Automated review]
| reducedMotion: AnimateOptions['reducedMotion'], | ||
| ) => { | ||
| const from = current(); | ||
| animator.stop(); |
There was a problem hiding this comment.
Under Reduce Motion this stop() also freezes the opacity of a running fade-through, and when the frozen transform already equals the target nothing replays, so the photo stays dim. + then - 150 ms apart leaves it at opacity 0.299, still 0.299 after closing and reopening (the item reset below does not touch opacity either). Ending every settle at an explicit opacity of 1, and resetting opacity with the item, fixes it.
There was a problem hiding this comment.
Right, a stopped cross-fade froze the photo at about 0.3 opacity, and it survived reopening. Fixed in e282e3c: every settle starts from full opacity, and the item reset writes opacity 1 too. LightboxZoomModes presses + then − 150 ms apart under Reduce Motion, then closes and reopens: 0.285 before the fix, 1 after.
| alignSelf: {default: null, '@media (pointer: coarse)': 'stretch'}, | ||
| flexGrow: {default: null, '@media (pointer: coarse)': 1}, | ||
| flexShrink: {default: null, '@media (pointer: coarse)': 1}, | ||
| flexBasis: {default: null, '@media (pointer: coarse)': 0}, |
There was a problem hiding this comment.
This coarse-pointer stretch (and the one on mediaGroup above) applies to every item, so with hasZoom off or a video a tap on the dark area lands on the stage, not the container, and handleBackdropClick no longer closes the viewer. Moving it into imageWrapperZoomTarget and a zoom-only media-group style keeps the changeset's "With hasZoom off, nothing changes." true.
There was a problem hiding this comment.
Right, the stretched stage swallowed taps meant for the backdrop when zoom is off. Fixed in e282e3c: only a zoom target's stage fills the viewer on a coarse pointer. LightboxZoomModes taps the dark area beside a photo with zoom off, and the viewer closes (it stayed open before).
| * is everything else. Pair `ios` with `pointer: 'coarse'` for a touch | ||
| * profile, such as touch motion tuned per platform. | ||
| */ | ||
| platform?: 'ios' | 'other'; |
There was a problem hiding this comment.
spec:AST-012 FR2 makes the when vocabulary closed (width, pointer, contrast, motion) and says "Future environment fields require explicit additions to this closed vocabulary." Could the FR2 amendment for platform come with this, for the AST-012 owners to approve, or the condition wait for it?
| # 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 interaction coverage of existing surfaces through its own scope check, which spec:AST-030 FR11 rules out: "New coverage inside an existing surface MUST join those owners and MUST NOT create a separately routed lane or required context." Could the proofs run as a step of the existing browser test owner, as the harness typecheck already joins typecheck?
| content: [ | ||
| { | ||
| type: 'prose', | ||
| text: 'On a touch screen, sheets, alerts, menus, navigation and scrolling play the springs, durations and curves measured from iOS. 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.', |
There was a problem hiding this comment.
At this head only Lightbox's zoom reads the motion system; sheets, alerts, menus, navigation and scrolling are unchanged, so a theme that retunes springs.sheetPresent (the example here, in the defineTheme JSDoc and in the template) changes nothing. Could this list only the roles a shipped component plays?
| "best-practices" | ||
| ], | ||
| "skeleton": "e648d618f9141bbd7f15bf970ba9e0d41b59ff9ae89e0cbbb2042e98bd5f0109", | ||
| "skeleton": "a86303e70a6cb63931e8e29c3b470b94f8edcc0b15576a907680a376213faddc", |
There was a problem hiding this comment.
This motion skeleton is stale: the zoom-geometry commit adds the zoomMedia.settle.curve row to astryx docs motion afterwards, so experimental-isolation.test.mjs now fails on this file (and test-node (1/4) with it). Rerunning the golden update and reviewing its diff fixes it.
There was a problem hiding this comment.
Regenerated in e282e3c: the motion topic's skeleton matches again.
| where a switch outside any scroll view is needed. | ||
|
|
||
| Done (iOS 26.2 simulator, Oct 23 2026; in the switch's own table, | ||
| `uikit/switch.ts`): M-SW02, M-SW03, M-SW04, M-SW05, M-SW06, M-SW09 (UISwitch |
There was a problem hiding this comment.
uikit/switch.ts (here) and uikit/contextMenu.ts (line 254) are neither in this pull request nor on main; please drop these references until those tables land.
|
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.
|
a467b57 to
e282e3c
Compare
github-actions
Bot
added
the
needs:design-review
|
Pushed in e282e3c, on #8320 @ 41b46ea (with #8076 @ b496085 underneath). Lightbox reads touch through |
e282e3c to
3a3acec
Compare
|
/approve-spec 4340726 |
There was a problem hiding this comment.
Changes requested. The zoom follows the current Lightbox record and this head's record edits are approved, but two things need fixing before it can land.
- Desktop behavior change needs an owner decision: with
hasZoom, a mouse, a pen and a trackpad now get the stage's gestures (a drag or a trackpad's scroll pans with rubber band and coast atuseZoomStage.ts:76and:181-182, a ctrl-wheel zooms the photo, and a double-click zooms about the click atLightbox.tsx:844-851), so either gate these paths to touch input or cite the owner decision that covers this desktop behavior. - The every-pointer changeset claims behavior main already has:
.changeset/lightbox-every-pointer.mdsays a mouse, a pen and a trackpad "now zoom and pan a photo too", but on main a double-click already zooms 2× and a drag already pans a zoomed photo, so describe only what is new (ctrl-wheel or trackpad-pinch zoom, trackpad-scroll pan, the coast, the double-click about the click).
While you are there, the @input lines in the useZoomCore.ts and useZoomStage.ts headers name options the hooks no longer take.
[Automated review]
| } | ||
|
|
||
| /** Every pointer: a mouse, a pen and a finger. */ | ||
| const EVERY_POINTER = ['touch', 'pen', 'mouse'] as const; |
There was a problem hiding this comment.
Blocking: desktop behavior change needs an owner decision. With hasZoom, this list gives a mouse and a pen the pan, the tap and the touch-down, and the pan also takes a trackpad's scroll (line 182), so desktop users get a drag with rubber band and coast, a scroll that pans, and a stage that claims every touch. Either gate these recognizers to touch input, or cite the owner decision that covers this desktop behavior.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] Lightbox: a mouse, a pen and a trackpad now zoom and pan a photo too. Ctrl-wheel to zoom; drag or scroll a trackpad to pan a zoomed photo, which coasts on when released moving. A mouse wheel's notches leave a zoomed photo in place. A double-click now zooms 2× about the click instead of about the centre, and the next goes back; the keyboard keeps its 2× zoom about the centre. |
There was a problem hiding this comment.
Blocking: "a mouse, a pen and a trackpad now zoom and pan a photo too" claims something main already does (a double-click zooms 2× and a drag pans a zoomed photo under every pointer). Suggested: "[feat] Lightbox: a ctrl-wheel or a trackpad pinch now zooms a photo, a trackpad's scroll pans a zoomed photo, and a zoomed drag coasts on when released moving."
| * @input A stage element ref (clipping and centring the photo), whether zoom | ||
| * is on, the current item's key, and the zoom-change callback |
There was a problem hiding this comment.
Non-blocking: the hook no longer takes "whether zoom is on". Suggested: "@input A stage element ref, the current item's key, and the zoom and fit callbacks".
| * @input The zoom core (useZoomCore.ts), the stage element ref, whether the | ||
| * gestures are on, the current item's key, and tap and drag callbacks |
There was a problem hiding this comment.
Non-blocking: the options hold no item key. Suggested: "@input The zoom core, the stage element ref, whether the gestures are on, and tap and drag callbacks".
There was a problem hiding this comment.
Changes requested: the zoom follows the current Lightbox record and this revision's record edits are approved, but one thing needs to change before it can land.
- Desktop behavior change needs an owner decision: with
hasZoom, a mouse, a pen and a trackpad now get the stage's gestures (a drag or a trackpad's scroll pans with rubber band and coast atuseZoomStage.ts:76and:181-182, a ctrl-wheel zooms the photo instead of the page, and a double-click zooms about the click atLightbox.tsx:844-851), so either gate these paths to touch input or cite the owner decision that covers this desktop behavior.
The every-pointer changeset now describes only what is new, and the header comments match the hooks' options. Non-blocking: .changeset/lightbox-every-pointer.md releases the double-click about the clicked point as [feat], while the Lightbox record classes it as a [fix], so a separate [fix] changeset would match.
[Automated review]
| } | ||
|
|
||
| /** Every pointer: a mouse, a pen and a finger. */ | ||
| const EVERY_POINTER = ['touch', 'pen', 'mouse'] as const; |
There was a problem hiding this comment.
Blocking: desktop behavior change needs an owner decision. With hasZoom, this list gives a mouse and a pen the pan, the tap and the touch-down, and the pan also takes a trackpad's scroll (line 182), so desktop users get a drag with rubber band and coast and a scroll that pans; with the ctrl-wheel zoom and the double-click about the click (Lightbox.tsx:844-851), either gate these paths to touch input or cite the owner decision that covers this desktop behavior.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] Lightbox: a ctrl-wheel or a trackpad pinch now zooms a photo, a trackpad's scroll pans a zoomed photo, and a zoomed drag coasts on when released moving. A double-click now zooms 2× about the click instead of about the centre, and the next goes back; the keyboard keeps its 2× zoom about the centre. |
There was a problem hiding this comment.
Non-blocking: the Lightbox record's compatibility line classes the double-click about the clicked point as a [fix] (accessibility conformance, AR2), so that sentence would fit a separate [fix] changeset.
There was a problem hiding this comment.
Approval waits on an engineering owner, and one fix is needed either way.
- Owner decision needed: this pull request's Lightbox record edits have no spec-owner approval at this revision, because the record changed again after the last one: FR5's zoom-settle timing, the Motion roles table, FR11's trackpad wording and AR9's control backing. An engineering owner records the decision by commenting
/approve-spec <full head SHA>on this pull request; the author may do so if they are an engineering owner. That decision should also say whether the gestures that still reach a mouse, a pen and a trackpad are covered as desktop behavior: a ctrl-wheel or trackpad pinch zoom, a trackpad-scroll pan with give and coast, the double-click about the clicked point, and the drag that stops at the photo's edges.
Required change:
- The touch-zoom changeset and the zoom docs promise momentum this revision no longer gives:
.changeset/lightbox-touch-zoom.mdpromises a touchscreen laptop's finger a pan with momentum and give, butuseZoomStage.ts:153-155keys both on a coarse primary pointer, and thehasZoomJSDoc andLightbox.doc.mjspromise momentum for a mouse drag that now stops at the release, so key the give on a finger's pan as well or narrow the claims.
The double-click now ships in its own [fix] changeset, as the record classes it.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] Lightbox's zoom now works under a finger the way the iOS Photos viewer does, on a phone, a tablet or a touchscreen laptop. Pinch the photo to zoom around your fingers, and move them to pan while you zoom. Past the zoom limits the photo gives way, then eases back when you let go. Double-tap zooms until the photo fills the screen, at least 2×, and keeps the tapped point under your finger; double-tap again to go back. A zoomed photo pans with momentum, stretches past its edges and settles back. The keyboard keeps the zoom it had: 2× about the centre, over the same 200 ms ease-out, and keyboard panning now stops at the photo's edges instead of carrying it out of view. |
There was a problem hiding this comment.
This promises a touchscreen laptop's finger "momentum" and a pan that "stretches past its edges". At this revision the give and the coast key on isTouchPresentation, which is (pointer: coarse), and a touchscreen laptop's primary pointer is fine. There, a finger's zoomed pan stops hard at the edge and does not coast. Either key the give on a finger's pan too (see useZoomStage.ts:153-155), or narrow this sentence to phones and tablets.
| const hasGive = (pan: PanGestureRecognizer) => | ||
| options.isTouchPresentation === true || | ||
| pan.trackedTouches[0]?.scrollType != null; |
There was a problem hiding this comment.
A finger on a touchscreen laptop gets neither give nor coast here, because isTouchPresentation is false where the primary pointer is fine. Keying on the pan's own pointer as well keeps a finger's pan the same on every device, and stays touch-only for the desktop:
const hasGive = (pan: PanGestureRecognizer) =>
options.isTouchPresentation === true ||
pan.trackedTouches[0]?.pointerType === 'touch' ||
pan.trackedTouches[0]?.scrollType != null;
|
/approve-spec b2866c2 |
|
/approve-spec bf6bed0 |
There was a problem hiding this comment.
This review waits on an engineering owner's decision; no code change is requested at this revision.
- The record edits (FR5's zoom-settle length, the Motion roles table, FR11's trackpad wording and AR9's control backing) are approved at this revision, and the Chinese
hasZoomdocs now match the English ones. - With
hasZoom, a mouse and a trackpad get new gestures: a ctrl-wheel or trackpad pinch zooms the photo instead of the page, a trackpad's two-finger scroll pans a zoomed photo with give and coast, and a double-click zooms about the clicked point. These change desktop behavior, so approval needs a recorded owner decision that covers them: please cite it on this pull request, or gate those paths to touch input.
One non-blocking comment suggestion is inline.
[Automated review]
| * - pan: one or two fingers, a mouse's or a pen's drag, or a trackpad's | ||
| * scroll. It moves a zoomed photo, and its release carries a scroll view's | ||
| * velocity. |
There was a problem hiding this comment.
Non-blocking (comment): a mouse's or a pen's drag now stops at the release (lines 370-375), so "its release carries a scroll view's velocity" holds only in the touch presentation and for a trackpad's scroll. The summary at lines 124-126 ("A zoomed pan coasts and bounces like a scroll view") reads the same way. Suggested here: "pan: one or two fingers, a mouse's or a pen's drag, or a trackpad's scroll. It moves a zoomed photo. In the touch presentation and for a trackpad's scroll, its release carries a scroll view's velocity."
|
/approve-spec 33e0568 |
There was a problem hiding this comment.
This review waits on an engineering owner's decision, and one fix is needed either way.
- Miss: with
hasZoom, the photo's stage now fills the viewer and carries thezoom-incursor (Lightbox.tsx:230-236,:840), so a mouse seeszoom-inover the dark area beside a fitted photo, where a click closes the viewer; keep the zoom cursors to the photo's box so the dark area keeps its default cursor, as onmain.
With hasZoom, a mouse, a pen and a trackpad also get new gestures: a ctrl-wheel or trackpad pinch zooms the photo instead of the page, a trackpad's scroll pans a zoomed photo, and a zoomed drag starts only after 10 px of travel. These change desktop behavior, so approval needs a recorded owner decision that covers them: please cite it on this pull request, or gate those paths to touch input. The record edits are approved at this revision.
[Automated review]
| !isVideo && isZoomed && styles.imageWrapperZoomed, | ||
| !isVideo && isDragging && styles.imageWrapperDragging, | ||
| isZoomTarget && styles.imageWrapperFocusInset, | ||
| isZoomTarget && !isZoomed && styles.imageWrapperZoomable, |
There was a problem hiding this comment.
With the stage filling the viewer (imageWrapperFill, line 837), this zoom-in cursor now covers the dark area beside a fitted photo, where a click closes the viewer through the stage's tap (onTapOutside). Keep the zoom cursors to the photo's box, so the dark area keeps its default cursor as on main.
|
(Posted by Cindy's agent.) Record basis: Lightbox FR11 + DEC-2 (approved) specify ctrl-wheel and trackpad-pinch zoom and trackpad-scroll pan; AR2 lists center-zoom on double-click as a gap this PR fixes. |
There was a problem hiding this comment.
Changes requested: one fix is needed before this can land.
- With
hasZoom, a mouse click on a zoomed photo can close the viewer:isClickOutsideMedia(Lightbox.tsx:578-614) judges the click against the fitted photo's box, but the zoomed photo now spreads over the whole viewer, so a click on the photo above or below that box (for example 30 px from the top of a 1280 × 1000 window after a 2× double-click) closes it; while zoomed, test against the photo's shown box as the finger tap already does (useZoomStage.ts:486-492), and add a case that clicks the zoomed photo there.
The zoom otherwise follows the current Lightbox record, and the record edits are approved at this revision. One small test note: "reports a click beside the photo to the newest onOpenChange" uses a mouse, whose click closes through the backdrop handler, so it never reaches the stage's tap callback it was added to pin; a finger tap would.
[Automated review]
| Math.max(photo.naturalWidth, caption?.offsetWidth ?? 0), | ||
| ); | ||
| const middle = box.left + stageElement.clientWidth / 2; | ||
| const isOutside = ({x, y}: {x: number; y: number}) => |
There was a problem hiding this comment.
This box is the fitted photo's, but with zoom the photo now spreads over the whole stage. After a 2× double-click in a 1280 × 1000 window, a mouse click at y = 30 lands on the zoomed photo and closes the viewer (on main that strip showed the backdrop, because the group clipped the photo). While zoomed, could this count a point on the photo's shown box as inside, as the stage's tap does in useZoomStage.ts:486-492?
|
/approve-spec aa943dd |
There was a problem hiding this comment.
Changes requested: the zoom follows the current Lightbox record and this revision's record edits are approved, but two motion timings contradict the touch-presentation spec that landed on main today.
- Regression: a finger's double tap plays the measured zoom settle (
timing.curves.movefor up to 400 ms,useZoomStage.ts:466), butcomponent:LightboxFR5 andspec:AST-094FR45 say a double tap's zoom plays the theme's duration and easing tokens, so play it on those tokens and update the double-tap case inLightboxZoom.interaction.browser.spec.ts(lines 657-690). - Regression: FR5's new sentence says the zoom settle plays
timing.curves.move"perspec:AST-094FR45", but FR45 says a gesture's settle springs from its release velocity and FR4 says no motion playstiming.curves.move, so either amendspec:AST-094FR4 and FR45 in this pull request with an owner's approval or make the pinch-release settle a spring, and drop "iOS measures this settle keyed by zoom ratio" from the record.
The rest checks out: the Lightbox unit tests pass, and the double-click about the clicked point, the trackpad pan and the control backing behave as the record says. CI has not run its build and test jobs on this draft revision.
[Automated review]
| // A finger's double tap fills the viewer, at least 2×, and keeps the | ||
| // tapped point in place. | ||
| const point = toStage(doubleTap.location()); | ||
| settleTo(geometry, doubleTapTarget(geometry, current(), point)); |
There was a problem hiding this comment.
Blocking (regression): this plays the measured zoom settle, timing.curves.move for min(400, 100 + 300·|ln ratio|) ms, for a finger's double tap. component:Lightbox FR5 and spec:AST-094 FR45, with its Lightbox row, say a double tap's zoom plays the theme's duration and easing tokens. FR4 also says no motion plays timing.curves.move. Play this zoom on the theme's tokens, and change the double-tap browser case that asserts the ease-in-out.
| | FR2 | Viewer overlay carries the current `lightbox` target. Media, Caption, and Counter carry no Lightbox public target. | Current source and public docs | settled | | ||
| | FR3 | Close, Previous, and Next button surfaces use Button's `button` target through IconButton rather than Lightbox-owned button targets. | Current source and target docs | settled | | ||
| | FR4 | The open viewer is a focus region through Core's shared focus-region module (`component:Dialog/DEC-2`): Tab from its last control moves focus to its first and Shift+Tab from its first moves focus to its last, so focus never leaves the viewer for the browser's own interface. | `component:Dialog/DEC-2` | settled; implementation gap | | ||
| | FR5 | Under a coarse primary pointer, after hydration, Lightbox presents FR6–FR10 with no opt-in. Every spring a gesture drives in them (the page settle, the dismiss and the pinch close) is a named motion role read through the motion role table, never a literal, and starts at the gesture's velocity; motion that a tap, a double tap, a key or the program starts (the controls fade and a double tap's zoom transition) plays the theme's duration and easing tokens (`spec:AST-094` FR45). The zoom settle plays `timing.curves.move` for `zoomMedia.settle.baseMs` plus `zoomMedia.settle.lnRatioMs` times \|ln(zoom ratio)\|, at most `zoomMedia.settle.maxMs`, per `spec:AST-094` FR45; iOS measures this settle keyed by zoom ratio. Under reduced motion they follow AR6. | `spec:AST-094` FR4, FR13 | settled; implementation gap | |
There was a problem hiding this comment.
Blocking (regression): "The zoom settle plays timing.curves.move … per spec:AST-094 FR45" contradicts spec:AST-094. FR45 says a gesture's settle springs from its release velocity, and FR4 says no motion plays timing.curves.move. This pull request does not amend either. Amend spec:AST-094 FR4 and FR45 here, or make the settle a spring. "iOS measures this settle keyed by zoom ratio" is evidence, so it does not belong in the record.
|
/approve-spec 987c249 |
There was a problem hiding this comment.
Changes requested: the Lightbox record edits are approved for this revision, and the double tap now follows the theme's move tokens, but two fixes are needed before this can land.
- Regression: a pinch's settle past a zoom limit plays
timing.curves.moveover timed lengths (useZoomStage.ts:297,useZoomCore.ts:342-346), and FR5's new sentence states it "perspec:AST-094FR45", whilespec:AST-094FR4 says no motion plays that row and FR45 requires a gesture's settle to spring from its velocity, so make the settle a spring or have an engineering owner amend AST-094 FR4 and FR45. tapZoomTiming(useZoomCore.ts:137-165) copies SegmentedControl'sthemeTimingOfandINSTANT(SegmentedControl/useSegmentedSelectionMotion.ts:355-385), so move one theme-timing reader intomotion/roles.tsand call it from both.
Non-blocking: AR6 does not name the double tap's zoom or the settle past a limit under Reduce Motion, and the comment at useZoomCore.ts:331-334 describes a registration the call below it does not make.
[Automated review]
| * The timing of a tap-started zoom, read from the theme where the stage is. | ||
| * A theme without those tokens gets no motion. | ||
| */ | ||
| function tapZoomTiming(element: Element | null): { |
There was a problem hiding this comment.
Blocking (one shared path): tapZoomTiming and AT_ONCE repeat SegmentedControl's themeTimingOf and INSTANT (SegmentedControl/useSegmentedSelectionMotion.ts:355-385). Both read the computed style, parse it with parseMotionRoleValue('duration', …) and parseCssEasing, and fall back to a 1 ms motion. Move one theme-timing reader into motion/roles.ts beside parseCssEasing, taking the element and which duration and easing to read, and call it from both.
| // The photo is off its fitted size while it moves, back at it only | ||
| // once a settle there lands. | ||
| setAtFit(false); | ||
| // The settle's curve is the `timing.curves.move` role (registered |
There was a problem hiding this comment.
Non-blocking (comment): this sits above registerReducedMotionCrossFade(), which registers the Reduce Motion cross-fade, not the settle's curve, and it packs three topics into two sentences. Suggested: "A pinch's settle plays the measured curve. Keys and a double-click play the released ease-out. A double tap plays the theme's move tokens."
There was a problem hiding this comment.
Changes requested: the Lightbox record edits are approved for this revision, and the double tap now reads the theme's timing through a shared reader, but two fixes are needed before this can land.
- Regression: a pinch's settle past a zoom limit plays
timing.curves.moveover timed lengths (useZoomStage.ts:297,useZoomCore.ts:343-347), and FR5 states it "perspec:AST-094FR45", whilespec:AST-094FR4 says no motion plays that row and FR45 requires a gesture's settle to spring from its velocity, so make the settle a spring or have an engineering owner amend AST-094 FR4 and FR45. - The new
readThemeTiming(motion/themeTiming.ts) leaves SegmentedControl's ownthemeTimingOfandINSTANT(SegmentedControl/useSegmentedSelectionMotion.ts:355-385) in place, and the two already treat an unreadable duration differently, so move SegmentedControl'ssettleLensonto the shared reader and delete its copy.
Non-blocking: AR6 does not name the double tap's zoom or the settle past a limit under Reduce Motion, and the branch needs a rebase for a conflict in packages/core/src/motion/README.md.
[Automated review]
| : settleCurves( | ||
| from, | ||
| target, | ||
| resolveMotion(stageElement, zoomMedia.settle.curve), | ||
| ); |
There was a problem hiding this comment.
A pinch released past a zoom limit (and a caught photo let go past one) settles here on timing.curves.move over the zoomMedia.settle lengths. Current spec:AST-094 FR4 keeps that row as a measured reference that no motion plays, and FR45 requires a gesture's settle to be a spring that starts at the gesture's velocity. Settle on a named spring role from the release velocity, or have an engineering owner amend AST-094 FR4 and FR45.
| * value never means no motion. An unset or unparseable easing gets its | ||
| * default. | ||
| */ | ||
| export function readThemeTiming( |
There was a problem hiding this comment.
This shared reader now serves Lightbox, but SegmentedControl/useSegmentedSelectionMotion.ts:355-385 (themeTimingOf, INSTANT) still reads and parses theme timing itself, and lands in 1 ms on a duration it cannot read where this falls back to Core's default. Move settleLens onto readThemeTiming (or easedProgress) and delete the copy, so one module owns this.
|
/approve-spec 14f14d7 |
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, and Photos' double tap and maximum zoom. UIKit's eased zoom settle stays a measurement only. Lightbox's main interaction is a gesture, so its zoom settles on springs.settle (spec:AST-094 FR45). 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 springs back about the last centroid, from the photo's scale velocity at the lift. A finger's double tap zooms to the larger of filling the viewer and 2x, about the tapped point. A mouse's or a pen's double-click zooms 2x about the clicked point. Every zoom settles on springs.settle, since Lightbox's main interaction is a gesture (spec:AST-094 FR45). 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, and keyboard panning stops at the photo's edges.
|
/approve-spec a743218 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. The pull request is a draft, so once it is ready for review a maintainer adds the ci:run label so CI runs, since its branch lives in a fork.
The zoom follows component:Lightbox FR5, FR6 and FR11 and spec:AST-094 FR43 and FR45, the record edits carry an engineering owner's approval for this revision, and every zoom now settles on springs.settle, which resolves the earlier settle finding.
Non-blocking:
- The doc comment in
useZoomStage.tsstill calls the return past a zoom limit "one eased animation"; it now springs from the photo's scale velocity. - AR6 does not yet name the zoom settle or a double tap's zoom under Reduce Motion.
[Automated review]
| * Past a limit, the return is one eased animation anchored at the last | ||
| * centroid. In the touch presentation, a zoomed pan coasts and bounces like a |
There was a problem hiding this comment.
Non-blocking: this still describes the return past a zoom limit as "one eased animation", but it now springs on springs.settle from the photo's scale velocity (lines 294-312). Suggested: "Past a limit, the photo springs back from its scale velocity at the lift, anchored at the last centroid."
|
/approve-spec a743218 |
On a WebKit runner held to four CPUs, a blocked main thread leaves the compositor a frame every 100 to 135 ms. The double tap's spring moves for half a second, so its busy proof judged two or three frames, under the four it needs. The proof now plays the viewer at tempo 3, through the tempo property Lightbox's motion reads, with a longer block. It checks that each zoom curve ran three times as long as at tempo 1 and still ran on the compositor. Two controls show it still fails a zoom that the main thread drives and one that freezes. The past-the-minimum pinch rests 300 ms before it lifts. On a loaded runner a 100 ms rest could still read as moving, so the return started at the pinch's speed instead of from rest. Under Reduce Motion, the fade a pinch past the maximum plays is held from the lift's own task. On a loaded runner it could end before a later hold took it.
|
/approve-spec 2a1e19c |
There was a problem hiding this comment.
Approved: CI passes on this head, and nothing blocks.
The zoom follows component:Lightbox FR5, FR6 and FR11 and spec:AST-094 FR43 and FR45, and the record edits carry an engineering owner's approval for this revision. The newest commit only hardens two browser proofs for slower runners and adds no new issue.
Non-blocking:
- The doc comment in
useZoomStage.tsstill calls the return past a zoom limit "one eased animation"; it now springs from the photo's scale velocity. - AR6 does not yet name the zoom settle or a double tap's zoom under Reduce Motion.
[Automated review]
Stack: #8147 → #8148 → #8197 → #8215, #9810 (both on #8197)
Stacked on #8147 (70a405d), on main 1dce318. Review this pull request's one commit: vjeux@a743218
This round rebases onto main with #9915's motion classes and #10068's shared theme-timing reader, and follows #9915's class for Lightbox. Its main interaction is a gesture, so every zoom settles on the shared
springs.settlerole (spec:AST-094FR45, DEC-26;component:LightboxFR5 on main). Specs decision, Nov 4 2026: "Lightbox: shared springs.settle, no new zoom role".--duration-medium(300 ms) on--ease-move, and the double-click and the keys played a 200 ms ease-out. All now playsprings.settle(SwiftUI's smooth spring, duration 0.5 s, no bounce). The page settle and the drag-down dismiss are feat(Lightbox): swipe between gallery photos like Photos #8197's and feat(Lightbox): drag a photo down to close, like Photos #8215's and keep their springs.createMotionEngine.Why
On a phone, a pinch on a Lightbox photo zooms the whole page, and a double tap zooms 2× about the centre wherever it lands. People compare it with Photos on their own phone, where the photo zooms about the fingers and gives at its limits.
hasZoomshould work with fingers, and with every other pointer (component:LightboxFR5–FR13, FR11).What changes
Record change
component:LightboxFR11 reads "a trackpad's scroll to pan a zoomed photo; a mouse wheel's notches leave it in place (spec:AST-094FR43)" where it read "a wheel to pan a zoomed photo". Its verification row adds the trackpad scroll and the mouse wheel's notch.dblclicklistener (FR14), since only the browser knows the system's double-click speed.hasZoom, Close, Previous, Next and the Counter sit on Lightbox's own translucent backing, with icons and text on the on-dark colour, so the buttons keep 3:1 and the Counter's text 4.5:1 over any photo, in every shipped theme (WCAG 2.2 1.4.3, 1.4.11).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
springs.settle(its duration was 251 ms, the eased curve's); the double tap fails (maxError 0.82 of scale against 0.02); the keyboard's and a mouse double-click's zoom fail (off the spring's progress by 0.32 and 0.26 at 100 ms).springStep, response 0.5 s, critically damped), within 0.01 and 0.02 of scale.Testing
At a743218 (the full battery ran at a9f70a1, whose code differs only in the busy proof's judging window and two comments):
pnpm install --frozen-lockfileexits 0; coretscis clean; strict ESLint on every changed file 0; comment sentence lengths 0 over;pnpm check:knowledge,check:changesets,check:repo,check:i18n-catalog,check:i18n-runtime,check:use-clientandcheck:package-boundariespass;