Repository navigation
Conversation
|
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
meta-cla
Bot
added
the
CLA Signed
c1c5294 to
a361da6
Compare
a361da6 to
6154244
Compare
6154244 to
9be22df
Compare
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This needs an owner decision before it can be approved: no current spec covers a reorder handle opening its row's moves, or the new ListItem.reorderLabel prop. An engineering owner has to approve or reject both and identify the record that will govern them, recording that decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. Whatever that decision, these need fixing:
- A real mouse click on a handle opens nothing: the handle's own mouse long-press recognizer makes the gesture host swallow the click, so the documented click path fails for mouse users, and a real-browser mouse-click test would catch it.
- The menu opens from an Effect one frame after the click and reads the DOM during render; opening it from the click handler, with the moves and the name computed there, would follow the React runtime rules.
- The diff carries the whole stack, about 44,600 authored lines, so landing #8139 and its predecessors first would leave only this pull request's own commit.
- The disabled-row test still passes without the disabled guard, because that row has no moves anyway.
[Automated review]
| aria-disabled={isDisabled || undefined} | ||
| aria-haspopup="menu" | ||
| aria-expanded={menu === 'open'} | ||
| onClick={() => { |
There was a problem hiding this comment.
A real mouse click never reaches this handler. The mouse LongPressGestureRecognizer above (minimumPressDuration: 0) recognizes every press, so GestureHost arms its stray-click swallower on pointerup and drops the trusted click. In Chromium a press and release on a handle opens no menu; only touch taps and clicks with detail === 0 get through. Opening the moves when the mouse recognizer ends without a changed, plus a page.mouse.click browser proof, would make the documented "a click" path work.
| // the handle, and a dragged handle would then lay out on every move. | ||
| // Mounted closed, then opened, so the menu moves focus into its rows. | ||
| const [menu, setMenu] = useState<'closed' | 'mounting' | 'open'>('closed'); | ||
| useEffect(() => { |
There was a problem hiding this comment.
This Effect only sequences React state (mount closed, open a frame later) and names no external system (architecture:react-component-runtime INV3); it also defers the click's open (INV2). Could the click handler compute the moves and the name, store them in the menu state, mount closed under flushSync and open in the same handler? That would also remove the render-time DOM reads at lines 318 and 331-333 (INV1, INV4).
| it('offers no moves from a disabled row, nor from a row that cannot move', () => { | ||
| render( | ||
| <List onReorder={() => {}}> | ||
| <ListItem label="Inbox" isDisabled /> |
There was a problem hiding this comment.
Inbox sits alone above the fixed Sent, so movesFor already offers it no moves, and this test passes even with the !isDisabled guard removed. A disabled row inside a movable group (no fixed row) would prove spec:AST-085 FR2 for the new menu.
9be22df to
e96e626
Compare
There was a problem hiding this comment.
This needs an owner decision before it can be approved: no current spec covers a reorder handle opening its row's moves, or the new ListItem.reorderLabel prop. An engineering owner has to approve or reject both and identify the record that will govern them, recording that decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. Whatever that decision, these need fixing:
- A real mouse click on a handle opens nothing: the handle's own mouse recognizer makes the gesture host swallow the click, so the documented click path fails for mouse users, and a real-browser mouse-click test would catch it.
- After a drag, the next assistive-technology activation of the handle is ignored, because the drag flag is cleared only by the next pointer press.
- The menu opens from an Effect one frame after the click and reads the DOM during render; opening it from the click handler, with the moves and the name computed there, would follow the React runtime rules.
- The diff carries the whole stack, about 44,500 authored lines including a press change that fails a RadioList test in CI, so landing #8139 and its predecessors first would leave only this pull request's own commit.
[Automated review]
| aria-label={name} | ||
| aria-describedby={context.hintId} | ||
| aria-disabled={isDisabled || undefined} | ||
| aria-haspopup="menu" |
There was a problem hiding this comment.
aria-haspopup="menu" is set on every handle, but activation opens a menu only when the row has moves (line 275). On a row alone in its group, such as Drafts below a fixed Sent in the new test, a screen reader announces a menu button that does nothing. Could the popup state be exposed only on a row that can move, derived from the list's children rather than the DOM?
| // The row's own pointer handling (its swipe) never sees the handle's | ||
| // touches; the gesture system listens on the document, before this. | ||
| onPointerDown={(event: PointerEvent<HTMLButtonElement>) => { | ||
| draggedRef.current = false; |
There was a problem hiding this comment.
Apart from the click itself, this is the only place draggedRef is cleared, and a drag's own click never arrives because the gesture host suppresses it. After any drag the flag stays set, so the next detail 0 activation, which brings no pointerdown, is swallowed: in Chromium, after a mouse drag, the first click() on the handle opened nothing and the second opened the moves. Clearing the flag when the drag ends or is cancelled would fix it.
e96e626 to
d02fc3d
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: no current spec covers a reorder handle that opens its row's moves when activated without a drag, or the new ListItem.reorderLabel prop. An engineering owner has to approve or reject both and identify the record that will govern them, recording that decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. Whatever that decision, these need fixing:
- A real mouse click on a handle opens nothing, because the handle's own mouse recognizer makes the gesture host swallow the click, so mouse users who cannot drag still have no path, and a real-browser
page.mouse.clicktest would catch it. - After a drag, the next activation of that handle with no pointer press, which is how the tests model a screen reader, is ignored, because the drag flag is cleared only by the next pointer press.
- The menu opens from an Effect a frame after the click and reads the DOM during render, and computing the moves and the name in the click handler would follow the React runtime rules.
- The diff carries the whole stack, about 44,600 authored lines, so landing #8139 and its predecessors first would leave only this pull request's own commit.
[Automated review]
…hat the model does astracat's 10:52Z review: the changeset still listed the slides and the drop's settle, which reorder.ts no longer holds, and ReorderRow.isFixed's doc said other rows cannot cross a fixed row, which movableRange no longer does for one in the middle. The changeset names the claim and the edge autoscroll steps, and the model's drop position, movable range and autoscroll step; isFixed says a run of fixed rows at either end bounds a drag and one in the middle is passed over (spec:AST-103 FR1).
… a mouse press At the document, an event from inside an open shadow root is retargeted to the shadow host, and so is a touch's target, so the host looked for participants from the shadow host and found none inside the root: the browser then sent its own mousedown and click. Both cases, a finger and a mouse, attach a tap to an element in the shadow root. Fail until the host resolves targets through the composed path.
…composed path Inside an open shadow root the document's listeners see the shadow host, in event.target and in a touch's target alike, so a recognizer on an element in the root never got its finger, and nothing claimed it. landedOn takes the composed path's first element for the event's own target and for a touch whose target is the event's (another finger keeps its own): a touch's start, a pointer's press and its pairing, the last touch a scroll reads, a cancelled pointer's touch and hover.
…ouch target never drops below 44 px - A click in a link row's own box, outside its anchor (its padding, or a tap in its touch hit area under a finger), goes to the row's anchor with its modifier keys, so link rows gain the reach other rows have. Item marks the row's anchor `data-astryx-row-link`; a row with both `onClick` and `href` keeps `onClick`. - The touch target is 44 px (`spec:AST-094` FR42), or more where the page sets `--size-touch-target` larger; a smaller value never lowers it. The docs and the changeset no longer tell themes to set it. - The code cites FR42, and `component:List` FR10 where it applies. - Under a finger an interactive row's `::after` is its touch hit layer, so the row press layer's notes say to decorate a row with a child. Red first: a tap in a link row's growth follows its link (browser); a click in a link row's padding reaches its anchor, modifiers included, and a row with both `onClick` and `href` keeps `onClick` (unit); a page's 30 px touch target leaves 44 px (unit, browser).
… hit area; the target never drops below 44 px) into the tree expansion change
… hit area; the target never drops below 44 px) into the list updates change
…in when the pointer or theme changes `--size-touch-target` was read once, when a row mounted, and only in px: a theme's `3.75rem` kept the 44 px reach, and a pointer that turned coarse, or a theme that changed, left the size the row first read. The size is now resolved in px, rem (through the root's font size) or em (through the element's), still never below 44 px, and read again when the primary pointer changes or the theme does. Red first (unit): a 3.75rem size and a 3em size at 20 px give 60 px; a pointer turning coarse re-reads it; a theme change re-reads it.
…again on a pointer or theme change) into the tree expansion change
…again on a pointer or theme change) into the list updates change
There was a problem hiding this comment.
Changes are needed before this can merge: the reorder-mode hint does what spec:AST-103 FR4 and DEC-5 specify, but the diff still carries the whole stack under #8139, and the hint's finger-target browser proof checks the wrong thing. Please:
- Merge #8139 and its predecessors first, or rebase after they land, so this pull request holds only its own 1,171 changed lines instead of 39,359.
- In the finger-target case of
ListReorderMode.interaction.browser.spec.ts, compare each hint button's box under the coarse pointer with its box under a fine pointer instead of capping every button, Done included, below 44 px, sincespec:AST-094FR42 leaves a box that grows only with its text outside its rule.
Because this pull request edits a current family record, the spec-owner-approval check also waits for an engineering owner's approval on the final head.
[Automated review]
| ); | ||
| for (const {name, box, hit} of targets) { | ||
| // Its own size: no wider for a finger. | ||
| expect(box.right - box.left, `${name} keeps its size`).toBeLessThan(44); |
There was a problem hiding this comment.
This caps every hint button below 44 px, Done included, so Done's label width under the mobile type scale decides the case (spec:AST-094 FR42 puts a box that grows only with its text outside its rule), and a button that grew for a finger up to 43 px would still pass. FR42's verification asks for geometry under a coarse and a fine pointer: compare each button's box here with its box under a fine pointer (or assert the coarse pointer adds no padding or minimum size), and keep the 44 px ceiling on the hit layer only, as the lines below already do.
…inst a fine pointer astracat's 13:21Z review: the finger-target case capped every hint button, Done included, below 44 px, which neither shows that no control grows for a finger (spec:AST-094 FR42) nor allows a button that is larger by itself. It now opens the same hint where the primary pointer is fine and requires each button to add the same space around what it shows (its glyph, or its text's width and line) under both pointers; the theme's type scale may set larger text for a coarse pointer, which the comparison leaves out. The invisible hit layer still only grows, up to 44 px or the button's own size. Failing-first: with a coarse-only 2 px of block padding on the buttons, the old cap passed and this case fails (Move up adds 4 px of height).
…e touch target size from the theme) under the reorder values
There was a problem hiding this comment.
Changes are needed before this can merge: the reorder-mode hint does what spec:AST-103 FR4 and DEC-5 specify, and its finger-target browser proof now compares each button with its fine-pointer box as spec:AST-094 FR42 asks, but the diff still carries the whole stack under #8139. Please merge #8139 and its predecessors first, or rebase after they land, so this pull request holds only its own 1,252 changed lines instead of 39,688. Because this pull request edits a current family record, the spec-owner-approval check also waits for an engineering owner's approval on the final head.
[Automated review]
|
Posted by Cindy's agent on her behalf: closed and reopened only to restart this PR's CI on the current workflow after tonight's queue-wide cancel; nothing in the PR changed. |
… hold A divided List row keeps its dividers while it is pressed, as it does today, and Core keeps no selection hold: products show a row kept selected through ListItem's isSelected. This removes the touch press's divider hiding (its ListItem styles, docs, changeset, tests and browser proofs) and the selection hold (selectionHold.ts, its held press state in the shared press code, its internal deselect constants, the row focus-return lines, story, changeset and proofs). The shared test helpers, the press-coverage guard and the press release's own clock stay. Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com>
The previous merge left List.tsx's hasDividers JSDoc unresolved and the List stories empty. This restores the stories (without the selection hold demo) and the plain hasDividers line. Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com>
There was a problem hiding this comment.
Changes are needed before this can merge: the reorder-mode hint does what spec:AST-103 FR4 and DEC-5 specify, but the diff still carries the whole stack under #8139 (37,508 changed lines against this pull request's own 1,252), and GitHub reports it as conflicting with main. Please merge #8139 and its predecessors first, or rebase after they land, so this pull request holds only the hint; the carried List.hasAnimatedChanges also disagrees with component:List FR12, which now specifies an animatedLayout handle. One small fix in the hint itself: the Effect at listReorderEngine.tsx:2024-2028 names no external system, which architecture:react-component-runtime INV3 requires, so fold it into the show and hide Effect or read the layer through an Effect Event.
[Automated review]
| // No external system: keeps the latest layer handle for the effect below, | ||
| // which re-runs only when the mode's row changes. Idempotent. | ||
| useIsomorphicLayoutEffect(() => { | ||
| layerRef.current = layer; | ||
| }); |
There was a problem hiding this comment.
This Effect says it synchronizes no external system, but architecture:react-component-runtime INV3 says a new Effect MUST name the external system it synchronizes. Fold the layer handle into the show and hide Effect below, or read it through an Effect Event, and delete this one.
|
An offer for the review's Effect point: offer/list-reorder-hint-effect fast-forwards this PR's head with one commit. The hint's show-and-hide Effect now reads the layer's anchor, show and hide directly. Those keep their identity while the layer opens and closes, so the Effect still runs only when the mode's row changes. That removes the separate Effect that kept the latest layer in a ref, which named no external system ( If you want it here, merge the branch. The rest of the review is about the stack under this PR. |
Depends on #8139. Review only this PR's change: vjeux/astryx@9e84c6c...21babff
Why
Reorder mode moves a row with the arrow keys. Someone using a mouse or a touch screen who can't drag has no arrow keys, and WCAG 2.2 requires a way to move it without dragging (SC 2.5.7).
What led to it
This is part of the UIKit-parity program, which makes Astryx's components behave like their iOS counterparts on touch screens.
spec:AST-103FR4 and DEC-5 call for on-screen moves in reorder mode. A small hint now appears below the handle, with Move up, Move down and Done:Options and tradeoffs
Why this choice
DEC-5 chose the hint, because it shows the moves where the row is, without opening a menu. The buttons reuse the keyboard hint's key glyphs, so the hint matches Astryx's other keyboard help. On touch, each tap area grows toward 44 px through an invisible layer (
spec:AST-094FR42), so the hint looks the same with a mouse and with a finger. It adds three strings, and ListItem grows by 25 B gzip.How to check it
In Storybook
Core/List→ Reorderable:I ran the unit tests and the browser tests in Chromium and WebKit, covering clicks, keys, taps and
start(key), plus a check that tap areas never overlap. All pass. Two reds come from elsewhere:touch-support-bundle, which #8671 fixes, andpr-rtl, main's newer RTL test from 0472c4e.Evidence
Tap, Move down, Done on iOS 26.2.
Writing rubric results
For the
family:overlay-dismissalmembership edit: R1 narration, none (the new entries state what the hint does); R2 research, none; R3 evidence, the verification row names the test; R4 to R6, none added.Stack (merge in this order): … → #8138 → #8139 → this → #8140 and #8143.