Repository navigation
Conversation
… the lift, under real touch input spec:AST-094 FR20's "follows the scroll to the lift" had only a jsdom test with a synthetic takeover; the FR18-FR22 verification row asks for real touch input. In Chromium with injected touches, a vertical drag on a list a watcher pan is attached to scrolls the list natively, no touchmove is cancelled, and the watcher goes from began to ended at the lift.
… at the edge stays the page's spec:AST-094 FR10: two fingers before a claim are the page's pinch. The scroll-edge hold (DEC-18) counted any finger, so with two fingers down it cancelled the pinch's moves before any drag was claimed. It now holds only while a single finger is down. The new two-finger test fails on the previous head. Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com>
… other native-scroll cases
… the gesture watcher
… is down off the sheet (spec:AST-094 FR10) The lone-finger hold counts only the fingers the pan tracks, so a second finger on plain page content left the sheet's finger held, and the moves of a pinch across the sheet's edge were cancelled. This test fails until the host counts every finger on the screen.
…is down anywhere (spec:AST-094 FR10) The lone-finger hold counted only the fingers the pan tracks, so a second finger on plain page content left the sheet's finger held, and a pinch across the sheet's edge had its moves cancelled. The host now applies a recognizer's hold only while the move's event reports one finger on the screen; the pan's own count still covers two fingers on the sheet.
An attach reads the element's own will-change in a task of its own. A shared animator (animatorFor) is never detached, so its node can leave the document with that read still waiting, and the read then forces a style pass on a node with no style: a test run that mounts and unmounts many pagers piled up such reads until a later wait timed out. The read now skips a node out of the document; a promotion that needs it reads it then, as one on a node attached in the same task does. A detach and dispose already cancel it. The new test fails on the previous code (one read of a removed node); the in-task read test now puts its element in the document, where the read happens.
meta#8521) into the input hold
…2's wave-2 re-fold onto main)
…nto uikit/c24-zoom-stage
…nto uikit/c24-zoom-stage
…ave 2) into c24-8197-base
vjeux
requested review from
cixzhang,
imdreamrunner and
josephfarina
as code owners
October 27, 2026 20:49
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 27, 2026 20:49
meta-cla
Bot
added
the
CLA Signed
…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.
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes. Against main this branch also carries the whole open #8252 stack (about 49.7k changed lines, of which this change is 73), so please land #8252 first and rebase so this pull request is just its own commit. Two issues in the change itself:
- After zooming with Enter, pressing Enter on Next hides every control while keyboard focus stays on the now-invisible Next button; arming the toggle only for clicks with
detail >= 1outside the controls, and clearing the timer when the photo changes, would fix it. - A single zoomable photo (no gallery) ignores the click, because the hit test only finds
[data-current] img, which only gallery pages have, while the updatedhasZoomdocs say a click hides the controls.
[Automated review]
| // second click of a double-click, which zooms, calls it off. | ||
| if (onPhoto && hasZoom && !isVideo && !stageTaps && !dragged) { | ||
| clearTimeout(clickToggleRef.current); | ||
| if (e.detail < 2) { |
There was a problem hiding this comment.
A keyboard activation reaches this branch with detail 0 at (0, 0). After zooming with Enter, Enter on Next has already moved the track with pager.goTo, but the zoomed photo's box still covers (0, 0), so this arms the toggle and the next photo opens with every control hidden while Next keeps focus. Arming only for e.detail >= 1 and a target outside [data-lightbox-chrome], and clearing the timer when the item changes, avoids it.
| // double-click zooms it): the photo takes no pointer events, so the | ||
| // click lands on its box. | ||
| const onPhoto = [ | ||
| ...e.currentTarget.querySelectorAll('[data-current] img'), |
There was a problem hiding this comment.
Only gallery pages carry data-current, so for a single image with hasZoom this finds no image and a mouse click never toggles the controls, although the updated hasZoom docs say it does. Hit-testing the current image in both modes (the one stage.contentRef points at) would cover 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.
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 | +48.6KB |
+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
github-actions
Bot
added
the
needs:spec-owner-review
Contributor
Author
|
Both fixed, each with a browser test that failed before: vjeux/astryx@7f4b4d3...995bd48
The stack lands bottom-up; until #8252 is in, the description's link shows this PR's own change. |
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on architecture:layer-runtime INV13 ("One exit copy per closing layer"), which this branch carries from the #8252 stack and main does not yet state; an engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Separately, please land #8252 first and rebase so this pull request is just its own commits: against main it carries about 54.3k changed lines, of which this change is 253. One issue remains in the change itself:
- A click on the photo followed within 350 ms by Tab moves focus to Next and then hides every control, leaving keyboard focus on an invisible button; calling off the pending toggle on keydown, or when a control takes focus, with a click-then-Tab browser spec, would fix it.
The single-photo and keyboard-zoom issues raised earlier are fixed on this head.
[Automated review]
Comment on lines
+836
to
+839
| if (e.detail < 2) { | ||
| clickToggleRef.current = setTimeout(() => { | ||
| toggleChromeRef.current(); | ||
| }, UIKIT_TAP.maximumIntervalBetweenTaps.value); |
There was a problem hiding this comment.
This arms the toggle, and nothing calls it off when a keyboard focus lands on a control: click the photo, press Tab within 350 ms, and focus sits on Next while every control fades to opacity 0 with no visible focus ring. Clearing clickToggleRef on keydown, or when a control inside [data-lightbox-chrome] takes focus, with a click-then-Tab case in the chrome spec, would fix it.
Contributor
Author
|
Superseded: the click now toggles the controls through the stage's tap in #8242. Closing in its favor; the branch stays. |
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 #8252. Review only this PR's change: vjeux/astryx@bd7a9cb...995bd48
Why
With a mouse or a trackpad, nothing hides Lightbox's controls: #8242's tap works on touch screens only.
spec:AST-094FR43 says gestures attach for every pointer.What led to it
The UIKit-parity work for Astryx: the first step to every pointer, before the gesture change that follows this PR.
Options and tradeoffs
Why this choice
It is the smallest change that gives every pointer the tap's behavior today. The cost is 87 bytes of gzip JavaScript (102,259 → 102,346).
How to check it
In the "Gallery with zoom" story, with a mouse:
Evidence
CI's
pr-interactionred here is #8148's double-tap zoom busy proof, inherited from #8148 and fixed there.