Repository navigation
Conversation
…from their pointer events install() reads a finger from its pointer events when the window has no ontouchstart, but the four touch handlers still took the Touch Events a browser sends anyway (Chromium after touch emulation turns on), tracking the finger twice, so a one-finger long press never recognized. Each touch handler now returns while usesTouchEvents is false, after noting the fingers down, as install()'s comment promised.
…ter events) into meta#8320
…ter events) into meta#8293
…ter events) into meta#8356
…ter events) into meta#8358
…box imports coarsePointer from utils/ (fZ moved it)
…x.tsx: this branch's imports with coarsePointer from utils/ (fZ moved it)
…) into c24-8147-base
…it/c24-zoom-geometry
…n before the click guard internal/motion.ts (meta#8562) re-exports UIKIT_CLICK from the barrel, so a tree carrying both fails typecheck and the Storybook build since this pull request dropped it. The barrel keeps it, with the click guard's window, and the table of every gesture group still leaves it out. Fails until the barrel exports it.
…'s window This pull request dropped UIKIT_CLICK when the click guard took the stray-click window over, but importers written before it read the barrel's UIKIT_CLICK (internal/motion.ts in meta#8562 re-exports it), so a tree carrying both failed typecheck and the Storybook build. UIKIT_CLICK stays in the barrel, deprecated, with the guard's 400 ms (CLICK_GUARD_WINDOW_MS), and UIKIT_GESTURES still leaves it out.
…; history only here) into c24-8147-base
…it/c24-zoom-geometry
…ill) into the wheel helper
…arrives opens when it does) into meta#8197 # Conflicts: # packages/core/src/Lightbox/Lightbox.tsx
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.
…ls, and a single photo takes it too astracat's review of meta#8746: - A keyboard activation fires a click too, with no click count and at (0, 0): after a keyboard zoom, Enter on Next armed the toggle and the next photo opened with every control hidden while Next kept the focus. The toggle arms only for a click with a count, outside the controls, and a page change calls a pending one off. - The hit test looked for a gallery page's current image only, so a single zoomable photo ignored the click; it now tests the photo the zoom stage moves, in a gallery or alone. Each has a browser test in the chrome spec that fails on the head before.
…s (0, 0) Enter on Next came while the zoom was still starting, before the photo covered (0, 0), so the test passed on the head before the fix too; once it waits for the zoom, it fails there (every control hidden) and passes with the fix.
… (spec:AST-094 FR43) The zoom stage's recognizers (pan, double tap, tap, touch-down, drag to dismiss) and the gallery's pager attach for every pointer; the pinch stays a finger's, and a ctrl-wheel reaches it through meta#8676. A mouse drag down closes the viewer, a sideways drag or a trackpad's scroll pages, a ctrl-wheel zooms and, out from the fitted size, closes, a wheel pans a zoomed photo, and a released drag coasts. A double-click is the stage's double tap (the zoom core's own mouse path goes); a pointer's zoom hides the controls and the keyboard's 2x leaves them. A release follows the pointer that began it: DR2 for a finger, DR3 for a mouse, a trackpad or a pen on a fine primary pointer. A stage tap recognized after a page change toggles nothing. The stage fills the viewer for every pointer, and meta#8242's unguarded getAnimations() uses the motion system's capability check. Tests: LightboxEveryPointer's ten proofs (each failing on meta#8746's build, with the wheel deltas the page sees); the fine-pointer proofs that asserted the old behaviour flip to FR43's; the chrome spec's page-change proof failed before the tap guard. Docs in English and Chinese; a patch changeset.
vjeux
requested review from
cixzhang,
imdreamrunner and
josephfarina
as code owners
October 28, 2026 00:01
|
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.
|
github-actions
Bot
added
the
needs:design-review
github-actions
Bot
requested review from
cvkxx,
ernestt,
kentonquatman and
rubyycheung
October 28, 2026 00:01
github-actions
Bot
added
the
needs:spec-owner-review
![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: an engineering owner needs to provide or identify the governing spec for the Lightbox photo-viewer gestures it ships and extends to a mouse, a pen and a trackpad (the Lightbox component record is still a draft), and to approve or reject the layer-runtime INV13 and AST-057 wording this head adds. Any engineering owner, the author included, can record that decision by commenting /approve-spec followed by the full head SHA. Independently of that decision, these changes are needed:
- Land #8746, #8676 and their bases first (or split them out) so this pull request covers its own 1,276-line change instead of a 59,918-line range.
- Fix the Ctrl+wheel round trip that leaves the controls hidden and the background black at the fitted size: the Control keydown marks the last input as the keyboard, so the return to the fitted size never shows them.
- Update
scripts/touch-support-bundle.test.mjs, which fails because Core's root entry now bundles the gesture modules Lightbox imports. - Show fine-pointer before-and-after evidence for the single photo's stage now filling the viewer (its focus ring and zoom cursor), as AST-017 FR59 asks, and drop or implement the AST-057 motion-role wording, which the row's code does not follow yet.
[Automated review]
| // Photos hides the controls as a gesture zooms, a mouse's, a pen's | ||
| // or a finger's alike (spec:AST-094 FR43); a zoom from the keyboard | ||
| // leaves them, so the keys that zoomed can reach them. | ||
| if (lastInputRef.current !== 'keyboard') { |
There was a problem hiding this comment.
A Ctrl+wheel zoom in and back out leaves the controls hidden and the background black at the fitted size. Holding Control fires a keydown in the dialog, so lastInputRef reads 'keyboard' (line 946) and this branch skips the show on the return, while onPinchZoomed (line 1140) hid them unconditionally; no wheel event updates lastInputRef. Ignoring modifier-only keys and recording a wheel as pointer input, or giving the hide and the show one gate, fixes it, and the ctrl-wheel proof in LightboxEveryPointer.interaction.browser.spec.ts could zoom back out and assert the controls show.
| styles.imageWrapper, | ||
| isGallery && styles.imageWrapperGallery, | ||
| isZoomTarget && !isGallery && styles.imageWrapperZoomTarget, | ||
| isZoomTarget && !isGallery && styles.imageWrapperFill, |
There was a problem hiding this comment.
With the touch gate gone, a single zoomable photo's stage fills the viewer under a fine pointer too, and this element also carries the :focus-visible outline and the zoom-in cursor (lines 1911–1912): the keyboard focus ring and the zoom cursor now cover the whole stage, including the dark area where a click closes the viewer. AST-017 FR59 asks for fine-pointer before-and-after evidence for that; keeping the outline and cursor on the photo's box would avoid the change.
| - **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 says the row's slide and spring play on theme motion roles, but packages/core/src/Item/useSwipeAction.ts still plays SWIPE_SETTLE_MS = 200, and nothing in this pull request changes it. Either drop this edit here or move the row's settle onto motion roles in the same change.
Contributor
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 | +51.1KB |
+463B |
Bundle Size Summary
Gzip figures measure each package's entry file, not the code an import pulls in.
| Package | Entry (ESM) | Entry gzip before | Entry gzip after | Delta |
|---|---|---|---|---|
| @astryxdesign/core | 4.9KB | 1.2KB | 1.2KB | 0B |
| Package stylesheet | Gzip before | Gzip after | Delta |
|---|---|---|---|
| @astryxdesign/core | 34.6KB | 34.8KB | +201B |
Accessibility Audit
Status: No accessibility violations detected.
Visual Regression
55 of 502 shot(s) changed.
A change here is a question, not a failure: check whether the after is the
picture you intended. Record that review on the PR. Baseline maintenance is an
explicit dispatch of CI; this report never rewrites the baseline or adds a release gate.
| component | story | theme | mode | pixels |
|---|---|---|---|---|
| Selector | Indicator Space Bottom Sheet Narrow End RTL | neutral | light | 753,933 |
| Selector | Indicator Space Bottom Sheet Wide Start | neutral | light | 740,072 |
| RichTextEditor | Markdown parity: side by side | probe | dark | 558,840 |
| RichTextEditor | Markdown parity: side by side | probe | light | 540,756 |
| Chat | Full AI Chat | probe | light | 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 35 more.
Generated by PR Enrichment workflow | Storybook | Sandbox | View full report
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on #8746. Review only this PR's change: vjeux/astryx@ec51222...7f75a16
Why
On a laptop, a Lightbox photo ignores a phone's gestures: a drag down does not close it, a trackpad's sideways scroll does not page, and a pinch does not zoom.
spec:AST-094FR43 says gestures attach for every pointer.What led to it
Lightbox's Photos-style gestures, from the UIKit-parity work, attached to touch screens only. #8746 made a click toggle the controls for every pointer; the rest needed the zoom stage and pager to take every pointer, which #8676 (wheel and trackpad input) allows.
Options and tradeoffs
Owner question: Should the drag to dismiss take only trackpad scrolling (
allowedScrollTypes: ['continuous']), so a wheel notch over a photo never starts a dismiss? Today it follows FR43, notches included.Why this choice
One gesture model is what FR43 asks; it removes the drift. Lightbox's own code shrinks; the size added is #8676's shared gesture modules.
How to check it
In the "Gallery with zoom" story, on a laptop:
Evidence
Desktop Chromium, 1280 × 800; before is #8746.
Ten new browser tests fail on #8746, pass here.