Repository navigation
Conversation
The note's reword joined its next sentence onto one long line; it now wraps like the rest of the file. No code change.
Rows move by transform (an update's slide, a swipe, edit mode's slide), and the shared touch hit area measures them as they stand in one frame: during a removal it re-measured a frame into the motion and wrote fractional insets, a layout and a paint in the middle of a compositor-only motion. A measurement that arrives while the row's list has a finite animation running or held is now dropped, and once those animations end the row is watched afresh, so its insets are taken at rest.
…w press release change
…election hold change
…touch hit area change
…tree expansion change
…list updates change
cixzhang's meta#8589 keeps the host's touchend passive, and its contextmenu and click listeners off, until a claimer is attached (syncClaimingListeners). Ported onto this pull request's input stream with no change to what it does: install() subscribes the touchend passive, and syncClaimingListeners swaps it for a cancelling subscription and adds the contextmenu and click subscriptions while a claimer is attached. A group press brings its own click subscription only while the host has none, and the host's takes it over. Her watcher test reads those listeners on the window, where the input stream registers them; her new group test (a finger, then a mouse, under a fine pointer) sits before the detach test. The test file and the bare fixture import both her ScreenEdgePanGestureRecognizer and this layer's selection lock from utils.
Here the click guard (utils/clickGuard.ts) owns clicks, so cixzhang's meta#8589 syncClaimingListeners, ported at meta#8356, switches only the touchend's cancelability and the contextmenu subscription; the group press subscribes no click of its own. Her meta#8072 5c59af1 (a finger on an action group under a fine pointer reaches its recognizers) arms the click guard where it armed the swallower. Her watcher test reads the touchend and the contextmenu on the window, and no click listener: the guard's comes only once a gesture acts.
… 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
…d a test can reset it - The press-start check put a pointerdown listener on every watched element, so a reorder handle carried one (spec:AST-103 FR5 keeps a list's listeners the list's). One shared capture listener on the document, on only while the pointer is coarse, now finds the innermost watched element a press lands in and checks it. - The pointer query is read at every watch, not only the first, so an environment that replaces matchMedia (a test) never inherits a stale coarse answer from a watch that was not stopped; resetHitAreaForTests stops and forgets every watch. - watchHitArea takes its minimum size in an options object. The new tests (no element listener for two watched controls, one document listener; a fresh fine answer after a watch left behind; a clean reset) fail on the previous head.
… row menu Cindy's rule for rows that both reorder and have a row menu (her 03:55Z comment): a hold on the reorder handle picks the row up, and a hold anywhere else on the row opens its menu. A still finger held on a handle opened the menu around the list: ContextMenu's long press starts from the touchstart that bubbled up from the handle, and Chromium also sent it the finger's context request. The handle now keeps both to itself: its touchstart does not reach a menu around the row, and a touch's context request on it is refused (a mouse's right click still reaches the menu). Failing-first: a jsdom test (a hold on the handle past the menu's long press, a touch context request on it, then a hold on the label), and a real-touch proof on the Reorder From Your UI story (a hold on the handle picks the row up with no menu; a hold on the label opens the menu and picks nothing up).
…ture listeners The handle stops its touchstart in React's bubble phase so a row menu around it never starts its long press from the handle. The gesture host and its recognizers listen in the capture phase (on the document and on armed elements), so they still see every touch that starts on the handle: a capture-phase document listener and the host's finger count prove it.
|
Posted by Cindy's agent on her behalf. Thanks, this covers most of
English strings outside a provider match the record (internationalization INV3), so no change is needed there. If this stays idle, we'll make these changes after about 30 minutes, keeping your authorship. |
|
Thanks. I'm on all eight now, each failing first:
|
![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 until an owner settles the slot-layout insertion cue. That cue is the accent line turned between the target slot and the next one, and it answers design:ordered-collection-reordering OQ2, which is still open. An engineering owner needs to accept it or choose another cue, and record the decision by commenting /approve-spec <full head SHA> here; the author may do so as an engineering owner. Separately, the diff against main carries a chain of at least 22 open stacked pull requests (266 files, 50,761 authored lines): please land them first and rebase, then split positions() from attach() or explain why the remaining 3,128 lines must land together.
- In a wrapping grid, the turned line lands between the wrong cards for a drop at a row's end, and it is drawn outside the container after a key move that wraps rows (
slotLineandstepLineinreorderModel.ts). - Resolve the enclosing modal when
attach()announces, not once at attach time, so a container attached before its<dialog>opens still announces inside it (spec:AST-103FR4b), and add a browser test for that case. - Name each handle from the item's accessible name:
aria-labelledbybeforearia-label, thentitle, then text. - Defer the hint root's
root.unmount()indetach(), because React logs an error when it runs synchronously inside an effect cleanup, as in the new story.
[Automated review]
| * lands in the order; past the last slot, half a step beyond it. None at the | ||
| * item's own place. (Proposed for design:ordered-collection-reordering OQ2.) | ||
| */ | ||
| export function slotLine( |
There was a problem hiding this comment.
At a wrapping grid's row boundary, the step from the target slot to the next slot is a diagonal, so this line lands between the wrong cards. With four 120 x 80 px cards to a row and 16 px gaps, a drop after the row's last card draws a 105 px line tilted 77 degrees in the gap between the 2nd and 3rd cards, where a row would draw "between B and C". stepLine below has the same cause: after a Right move that wraps rows, reorder mode's line is centred 144 px outside the container. Whichever cue OQ2 settles on, marking the target slot's own edge on its row at a row boundary, plus a browser case for it, would keep the cue on the candidate position (design DR3, spec:AST-103 FR4).
| ); | ||
| // Inside the modal dialog that encloses the container, where assistive | ||
| // technology still reads (spec:AST-086 FR14); else the document's body. | ||
| const host = |
There was a problem hiding this comment.
The announcement host is resolved once, here, at attach time. A container attached before its <dialog> is opened with showModal() keeps its live region, and the hint root added at line 393, in document.body. The modal makes the body inert, so moves are announced only through the page-body channel, which is the failure spec:AST-103 FR4b's verification names ("an announcement inside a page-opened modal that reaches only the page-body channel"). Resolving the enclosing modal when announcing and when reorder mode starts would fix it, together with a browser test of a dialog opened after attach.
| let attachedCount = 0; | ||
|
|
||
| /** What an attached item is called: its accessible name, else its text. */ | ||
| function nameOf(element: HTMLElement): string { |
There was a problem hiding this comment.
nameOf reads aria-label before aria-labelledby and never reads title. The accessible-name computation puts aria-labelledby first and falls back to title, and FR4b names the handle from "the item element's accessible name, else its text content". As written, an image card named only by title gets a handle named "Reorder" alone, and its moves are announced by position. Suggested order: aria-labelledby, aria-label, title, then text.
| if (movedTimer != null) { | ||
| clearTimeout(movedTimer); | ||
| } | ||
| root.unmount(); |
There was a problem hiding this comment.
detach() is usually returned from a React effect, as in the new story (ReorderableAttach.stories.tsx:161–177). React DOM 19 runs effect cleanups inside its commit, and a synchronous root.unmount() there logs "Attempted to synchronously unmount a root while React was already rendering" in development, on every unmount and on Strict Mode's double-invoke. Deferring the unmount (for example, queueMicrotask(() => root.unmount()) before removing hintHost), or rendering the hint without a root of its own, avoids it.
…shared announcer, pointer changes
The owner's review of attach (spec:AST-103 FR1, FR4, FR4b, FR5; spec:AST-094
FR24, FR43; spec:AST-086 FR14):
1. A rejected or keyless item's handle is hidden (a handle that is the item
itself stays), restored on detach.
2. Their names are registered, so a refusal past one names it ("Can't move
past Pinned", not "item 1").
3. click, keydown and focusout are delegated from the container, resolving
the handle from the event's target; no listener on any handle.
4. Announcements go through the shared channel (announceMessage), or
announceInModal inside the open modal <dialog> enclosing the container,
resolved at each announcement; attach owns no live region.
5. The hint's host sits right after the container, in its theme scope, with
the providers it cannot reach documented; react-dom/client loads when
attach runs, not with the import.
6. watchCoarsePointer follows the primary pointer turning coarse or fine:
the session's pointer listeners, each handle's touch-action, the motion.
7. The commit observer watches the container's subtree, and an item under a
wrapper resolves to itself, not to the wrapper (rowOf; layout offsets
through positioned wrappers).
8. One controller method, attachPointers(isCoarse), wires the pointers for
List's host and attach.
The automated review's four:
- In a wrapping grid the slot-layout cue at a row's end, and after a key
move that wraps rows, stays on the row (slotLine and stepLine treat a step
that wraps rows as no neighbour).
- A container attached before its dialog opens announces inside the open
dialog (browser test, Attached Cards In A Dialog).
- Handle names follow the accessible-name order: aria-labelledby, then
aria-label, then title, then text.
- The hint root unmounts after the current render, so a detach inside a
React effect cleanup draws no error from React.
Failing-first: each case above failed before its fix (jsdom: hidden grips,
the rejected name, listeners on handles, the pointer change, the hint's
place, a second live region, a move among wrapped items refused, the name
order, React's unmount error; model: the row-end and wrapping cues;
Chromium: the dialog opened after attach).
|
Pushed 2e43f0f with all eight, each failing first:
The bot's four are in too:
One decision is yours: the slot-layout insertion cue ( |
|
Posted by Cindy's agent on her behalf. Thanks for fixing all eight. Loading the hint on demand is fine:
|
…rer, and the hint renders in place The owner asked for proof that attach's keyboard path never waits for the on-demand hint renderer. A test of its own file holds `react-dom/client` unloaded (a deferred mock): while the hint's host is empty, Enter enters reorder mode (aria-pressed true), ArrowDown calls onReorder and announces the place, Escape ends the mode and announces it; once the renderer loads, the hint mounts for a mode still on, and a container detached before then renders nothing. Writing it found the hint's layer moving out of its host: the host and its root were spans, phrasing content a Layer leaves for a safe host (Layer/layerHost.ts), so the hint rendered at the end of the body, outside the container's theme scope. They are divs with display: contents now, and the hint renders in place, right after the container. Failing-first: with span hosts, the hint's buttons never appeared in its host.
meta-cla
Bot
added
the
CLA Signed
|
Both are in f70367c:
Writing the test also found the hint's layer leaving its host for the end of the body, because the host was made of spans. The host is divs now, so the hint renders in place, in the container's theme scope. |
There was a problem hiding this comment.
Approval is on hold until an owner settles the slot-layout insertion cue: the accent line turned between the target slot and the next one, which answers the still-open design:ordered-collection-reordering OQ2. An engineering owner can accept that cue or choose another, and records the decision by commenting /approve-spec <full head SHA> here; the author may do this, as an engineering owner. Separately, the diff against main still carries four open stacked pull requests (267 files, 51,451 authored lines). Please land those first and rebase, then split positions() from attach() or explain why the remaining 3,848 lines must land together.
- In a
positions()layout, or a positioned container, that has been scrolled sideways,begin()andtrack()measure the dragged item on screen but compare it with slot centres in layout coordinates, so the drag proposes the wrong slot; a browser case that scrolls before dragging would pin the fix. - In right-to-left,
slotLine's fallback for an item alone on its row draws the line on the wrong side; passing the direction, asgridOfalready takes it, would fix this, with a model test.
[Automated review]
| const slots = this.slotsFor(list, rows); | ||
| const listBox = list.getBoundingClientRect(); | ||
| const originX = listBox.left + list.clientLeft; | ||
| const left = row.getBoundingClientRect().left - originX; |
There was a problem hiding this comment.
In a container that scrolls sideways, left is the item's on-screen offset, so it already has the container's scrollLeft taken off. track() then compares session.left + shiftX + width / 2 (line 1430) with slot centres laid out from the unscrolled padding box: positions(), or layoutLeft in slotsFor. After the strip scrolls two card steps, a card held over the fifth slot proposes the third, and the line is drawn there, against FR1's nearest-centre rule. Taking left from layoutLeft(row, list), as lefts does at line 1379, while keeping grabX from the on-screen box, or adding list.scrollLeft, would fix it. A browser case that scrolls the strip before the drag would pin the fix.
| return lineAcross(at, {x: 2 * at.x - back.x, y: 2 * at.y - back.y}, size); | ||
| } | ||
| // Alone on its row and column: at its edge along the reading order. | ||
| const side = forward ? size.width : -size.width; |
There was a problem hiding this comment.
This fallback puts the line on the item's right edge for every forward drop. In a right-to-left wrapping grid whose last row holds one item, a drop at the end lands after that item in reading order, but the line lands on its reading-start side. Passing isRtl, as gridOf and arrowTarget already take it, and mirroring side would keep the cue beyond the slot. A case in reorderModel.test.ts would pin it.
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsItem (@astryxdesign/core) · View in Storybook
TreeList (@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/CheckboxList | +15.0KB |
+79B |
| core/CheckboxListItem | +4.8KB |
+804B |
| core/ContextMenu | +19.7KB |
+858B |
| core/ContextMenuItem | +36.7KB |
+4.4KB |
| core/List | +15.0KB |
+184B |
| core/ListItem | +4.8KB |
+804B |
| core/Markdown | +19.8KB |
+847B |
| core/PowerSearch | +15.8KB |
+418B |
| core/Table | +19.8KB |
+843B |
| core/TableCell | +19.7KB |
+853B |
| core/TableHeaderCell | +19.7KB |
+867B |
| core/TreeList | +14.8KB |
+439B |
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 | 5.0KB | 1.2KB | 1.3KB | +31B |
| Package stylesheet | Gzip before | Gzip after | Delta |
|---|---|---|---|
| @astryxdesign/core | 34.6KB | 36.1KB | +1.5KB |
Accessibility Audit
Status: 2 accessibility violation(s) found — 2 serious.
Radio group pattern - 1 issue(s)
- 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
- Rule:
color-contrast· Affects 3/11 stories · Learn more - WCAG: 1.4.3 (Level AA)
- Rule:
RadioList - 1 issue(s)
- 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
- Rule:
color-contrast· Affects 3/14 stories · Learn more - WCAG: 1.4.3 (Level AA)
- Rule:
Visual Regression
55 of 506 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
… 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.
Approval is on hold until an engineering owner settles the slot-layout insertion cue, which answers the still-open design:ordered-collection-reordering OQ2: the accent line turned between the target slot and the next one, or another cue such as an outline on the target item. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author, as an engineering owner, may do so. Separately, the diff against main still carries at least five open stacked pull requests (258 files, 49,271 authored lines): please land them and rebase, then split positions() from attach() or explain why the remaining 3,848 lines must land together.
- When a
positions()layout or an attached container scrolls sideways,begin()measures the dragged item on screen whiletrack()compares it with slot centres in layout coordinates, so the drag proposes the wrong slot (listReorderEngine.tsx:1308,:1430); a browser case that scrolls before dragging would pin the fix. - In right-to-left,
slotLine's fallback for an item alone on its row draws the line on the reading-start side (reorderModel.ts:245); passing the direction, asgridOfalready takes it, with a model test, would fix it. createReorderable.doc.mjs:50still callsstart(key)the object's one method, besideattach.
[Automated review]
| name: 'reorderable', | ||
| type: 'Reorderable', | ||
| description: | ||
| "The collection's reorder behavior: pass it as `List`'s `reorderable` prop, with an `itemKey` on each `ListItem`. Make it once per collection (with useMemo); one made on every render still works. Its one method, `start(key)`, enters reorder mode for that item from your own UI (a Move item in its context menu, a toolbar button): the focus moves to its handle, as activating it does; a key that cannot move or is not mounted does nothing and warns in development. One object may serve several collections, each reordering on its own; `start` then uses the one mounted last. Its `attach(container, {items})` reorders plain elements with no collection component: `items()` returns one `{key, element, handle}` per item in order, each handle becomes reorder mode's toggle named after its item (a rejected or keyless item's handle is hidden), and it returns `{detach}`, which removes everything it added. Items may sit under wrappers in the container. Reorder mode's hint renders in a React root of its own right after the container: it takes the theme around the container, not a theme set on the container itself or the providers of your React tree, and speaks the shipped English strings.", |
There was a problem hiding this comment.
This sentence still says “Its one method, start(key)”, and the same description then documents attach(container, {items}) as a second method. spec:AST-103 FR4a now reads “Beside FR4b's attach, nothing else on the object is public”, so the canonical descriptor (FR7) should describe both methods.
|
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. |
Depends on #8683. Review only this PR's change: vjeux/astryx@af760b2...f70367c
Why
Card games, kanban boards and photo grids need a List's reordering (drag, keyboard, screen reader), but they're usually plain elements, rarely one column, and
createReorderableonly works withList.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. #8601 (merged) added two capabilities to
spec:AST-103, and this PR implements them:positions(): the app gives each item's slot, such as points on a fan's arc. A drag follows the pointer both ways and drops the item on the nearest slot.attach(container, {items}): plain elements get List's reordering. Grips become reorder-mode buttons, moves are announced, and focus stays on the moved item through re-renders.detach()removes everything it added.Options and tradeoffs
react-dom/client(56,867 B gzip), can ship with the import or load whenattach()runs. Loading late means a mode started in the first moment shows its hint a little later; the keyboard works meanwhile.Why this choice
The turned line keeps List's look. Loading the renderer late keeps the import small:
createReorderablegrows from 76,489 B to 81,242 B, andListdoesn't change. One PR, since #8601 specifies both andattach()reuses the slot model.How to check it
In Storybook
Core/createReorderable:I ran the unit tests and the browser tests (Chromium and WebKit) for each step with a mouse, a finger and a pen, and all pass. Two reds come from elsewhere:
touch-support-bundle, which #8671 fixes, andpr-rtl, main's newer RTL test, which this branch predates.Evidence
Browser tests and the stories.
Stack (merge in this order): … → #8140 → #8683 → this.