Repository navigation
Conversation
|
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
d91ce8c to
9795834
Compare
9795834 to
6a3b76c
Compare
![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 needs an engineering owner's decision before it can be approved: no current spec governs List's new hasAnimatedChanges and followingContent props, and the draft AST-094 FR8 (a move fades through under Reduce Motion) conflicts with the code keeping the slide (REDUCED_MOTION = 'keep'). An engineering owner can provide or identify the governing List spec, settle FR8 against 'keep', and record the decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. Independently of that decision, these need changes:
- The diff carries the whole stack (17,607 authored lines); once the predecessor pull requests land it should contain only this pull request's two commits (998 lines).
- With
followingContent="moves", content below the list that layout did not move (for example a footer held bymargin-top: auto) is shifted by the list's height change and slides back (contentBelowinrowMotion.ts). - Row positions are recorded only on
Listrenders, so a layout change between renders (a rotation, a row growing on its own) replays as motion at the next render. - With
hasAnimatedChangesoff, everyListstill subscribes to(pointer: coarse)and reads every row's layout on each render.
[Automated review]
| // Below, not beside: a flex or grid neighbour does not move when the | ||
| // component changes height. | ||
| const top = | ||
| next.getBoundingClientRect().top - presentation(next, 'y', motionNow()); | ||
| if (top < bottom - 1) { | ||
| continue; | ||
| } | ||
| below.push(next); |
There was a problem hiding this comment.
This keeps any in-flow element whose top is below the list, but never checks that layout moved it. When an ancestor absorbs the list's height change (a footer held at the bottom by margin-top: auto, the block after a grid row kept open by a taller column, a fixed-height or overflow: hidden parent), useListUpdateMotion still shifts it by -grew and springs it back, so content that did not move jumps and slides. Recording each candidate's position at every commit, as the rows' are, and springing it by its own measured shift would avoid that.
| const before = placesRef.current; | ||
| placesRef.current = places; | ||
| const height = element.offsetHeight; | ||
| const grew = height - heightRef.current; | ||
| heightRef.current = height; |
There was a problem hiding this comment.
The baseline is only refreshed here, after a List render. A rotation that rewraps rows, a row that grows from its own state, or a late font moves rows without rendering List, and the next unrelated render replays that change as motion for the rows and the content below. Refreshing the places and height when layout changes without a commit (for example from a resize observation held only while hasAnimatedChanges is on) would stop the replay.
| enabled: boolean, | ||
| followingContent: 'moves' | 'stays' = 'moves', | ||
| ): void { | ||
| const animates = useMotionPresentation() === 'uikit' && enabled; |
There was a problem hiding this comment.
List calls this hook for every instance, so with hasAnimatedChanges off each List still subscribes to (pointer: coarse) here and, below, reads every row's offsetParent/offsetTop and its own offsetHeight on every render before the enabled check. Returning before the measurements when the prop is off (clearing placesRef), and reading the pointer inside the effect instead of subscribing, keeps the opt-in free for lists that do not use it.
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
6a3b76c to
5df8024
Compare
There was a problem hiding this comment.
This pull request needs an engineering owner's decision before it can be approved: no current spec governs List's new hasAnimatedChanges and followingContent props, and the draft AST-094 FR8 (a move fades through under Reduce Motion) conflicts with the code keeping the slide (REDUCED_MOTION = 'keep'). An engineering owner can provide or identify the governing List spec, settle FR8 against 'keep', and record the decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. Independently of that decision, these need changes:
- The diff carries the whole stack (18,305 authored lines); once the predecessors land, or after retargeting and rebasing onto main, it should contain only this pull request's two commits (1,191 lines).
- With
followingContent="moves", content below the list that layout did not move (for example a footer held bymargin-top: auto) is shifted by the list's height change and slides back. - The same transient
translatere-anchorsposition: fixedelements inside that content, so a fixed action bar jumps up the screen for about half a second. - Row positions are recorded only on
Listrenders, so a layout change between renders replays as motion at the next render. - With
hasAnimatedChangesoff, everyListstill subscribes to(pointer: coarse)and reads every row's layout on each render.
[Automated review]
| if ( | ||
| style.position === 'absolute' || | ||
| style.position === 'fixed' || | ||
| style.position === 'sticky' || | ||
| style.display === 'none' | ||
| ) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
contentBelow checks only the candidate's own position, but the translate it then writes makes the candidate the containing block of any position: fixed element inside it. In a fixture with an action bar fixed 16px above the bottom of a 720px viewport, inside a section after the list, removing a row moved the bar from 660px to 344px from the top, and it stayed re-anchored until releaseMotion cleared the translate about 500 ms later. Skipping candidates that contain fixed-position descendants, or defaulting followingContent to 'stays', would avoid it.
5df8024 to
6ffa28d
Compare
6ffa28d to
84c1f76
Compare
There was a problem hiding this comment.
This pull request needs an engineering owner's decision before it can be approved: no current spec governs List's new hasAnimatedChanges and followingContent props, and the draft AST-094 FR8 (a move fades through under Reduce Motion) conflicts with the code keeping the slide (REDUCED_MOTION = 'keep'). An engineering owner can provide or identify the governing List spec, settle FR8 against 'keep', and record the decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. Independently of that decision, these need changes:
- The diff carries the whole stack (18,321 authored lines); once the predecessors land, or after retargeting onto the stack's previous branch, it should contain only this pull request's two commits (1,191 lines).
- With
followingContent="moves", content below the list that layout did not move (for example a footer held bymargin-top: auto) is shifted by the list's height change and slides back. - The same transient
translatere-anchorsposition: fixedelements inside that content, so a fixed action bar jumps up the screen until the motion ends. - Row positions are recorded only on
Listrenders, so a layout change between renders replays as motion at the next render. - With
hasAnimatedChangesoff, everyListstill subscribes to(pointer: coarse)and reads every row's layout on each render.
[Automated review]
|
Thanks. This pull request merges after #8061, #8076, #8446, #8360, #8890, #8077, #8085 and #8088, in that order. The compare link at the top of the description shows only its 12 files, and the size line is base against head for this change. On the size of the List change itself (2,067 lines):
|
There was a problem hiding this comment.
Changes requested. The List change follows component:List FR12, the latest push's private doc entry for the touch band's divider width is correct, and your breakdown of the List change's 2,067 lines answers the earlier request to trim it, so no trim is needed once it stands alone. What remains is the stack: please land #8061, #8076, #8446, #8360, #8890, #8077, #8085 and #8088 first, so this diff is the List change alone (2,067 of its 35,031 authored lines) and its size line reports base against head for every component it changes. This branch also carries #8360 at a commit 18 commits behind that pull request, so merging it now would ship its superseded code.
[Automated review]
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: this head rewords component:TreeList FR8, which is still proposed, so that the rows after a collapsing group hold until its rows have slid under the parent. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or FR8 can be settled in #8088 first. The List change itself follows component:List FR12; what remains after the decision is the stack: please land #8061, #8076, #8446, #8360, #8890, #8077, #8085 and #8088 first, so this diff is the List change alone (2,067 of its 35,786 authored lines) and its size line reports base against head for every component it changes.
[Automated review]
| | FR5 | `TreeListItemData` MUST admit `className`, `style`, and `xstyle` as one per-item styling seam. Every supplied value MUST compose on the same Item row box; an implementation MUST NOT treat current support for only one mechanism as authority to exclude the other two. | Owner decision; `architecture:public-component-api` INV5–INV6; `spec:AST-002/DEC-1` | Settled intent; implementation may be staged | | ||
| | FR6 | The Item row box is the logical element that paints the row, owns its hover state, and contains Start content, Item label and description, and End content. The styling seam MUST NOT move to the semantic `<li>` or another element without a compatibility decision. | Owner decision and the caller-owned per-row interaction boundary | Settled intent | | ||
| | FR7 | A click on an Item row outside its Item label performs the row's action: an Item with `onClick` runs it; otherwise an Item with children toggles; otherwise an Item whose only action is its link follows that link as Enter does, with modified and middle clicks following the shared enlarged-surface rule in `family:navigation-destinations`. The Chevron and controls in Start content and End content keep their own activation, and a disabled Item does nothing. | Owner decision; `family:navigation-destinations` | Settled intent; implementation may be staged | | ||
| | FR8 | Expanding or collapsing an Item with children MUST play its group's own motion: under a coarse primary pointer its rows slide out from under the parent as one block, clipped to the group's box, and back on collapse, a collapsing group's rows leaving the accessibility tree as the slide starts, while the Chevron turns, by transform only (`spec:AST-094` FR4–FR5); under a fine pointer the group appears and leaves at once. With an `animatedLayout` handle, the group and the rows after it follow `spec:AST-105`'s shared motion under every pointer (FR4, FR5); without one, the group's own slide stays touch-only and the rows after the group take their places at once (`spec:AST-105` FR2): as an expanding group appears and, under a coarse primary pointer, as a collapsing group's rows finish sliding under the parent, until which they hold their places. | UIKit outline list (`UICollectionView`), iOS 26.2, whose rows after a closing group hold until its rows have slid under the parent (0.24–0.37 s measured); `spec:AST-105` FR2 | Proposed; awaiting an engineering owner's approval | |
There was a problem hiding this comment.
This rewords FR8, which is still "Proposed; awaiting an engineering owner's approval" on main: without a handle, the rows after a collapsing group now hold until its rows have slid under the parent, and the verification row at L279 fails rows that move earlier. It needs an engineering owner's decision (an exact-head /approve-spec), or it can be settled in #8088 first. It also leaves open where focus goes when a collapsing row holds it.
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: this head carries #8088's rewording of component:TreeList FR8, still proposed on main, so that under a finger the rows after a collapsing group hold until its rows have slid under the parent. An engineering owner records that decision by commenting /approve-spec 2aa3b3b332491c3a07850657b2bfd6e9d064d5e6 on this pull request (the author may do so as an engineering owner), or FR8 is settled through #8088 first. The List change itself follows component:List FR12; after that decision, two changes remain:
- Land the eight predecessor pull requests first, so this diff is the 2,067-line List change rather than 35,919 authored lines.
- Report the base-against-head size of every component the diff changes, or let the existing size line cover it once the predecessors land.
[Automated review]
| | FR5 | `TreeListItemData` MUST admit `className`, `style`, and `xstyle` as one per-item styling seam. Every supplied value MUST compose on the same Item row box; an implementation MUST NOT treat current support for only one mechanism as authority to exclude the other two. | Owner decision; `architecture:public-component-api` INV5–INV6; `spec:AST-002/DEC-1` | Settled intent; implementation may be staged | | ||
| | FR6 | The Item row box is the logical element that paints the row, owns its hover state, and contains Start content, Item label and description, and End content. The styling seam MUST NOT move to the semantic `<li>` or another element without a compatibility decision. | Owner decision and the caller-owned per-row interaction boundary | Settled intent | | ||
| | FR7 | A click on an Item row outside its Item label performs the row's action: an Item with `onClick` runs it; otherwise an Item with children toggles; otherwise an Item whose only action is its link follows that link as Enter does, with modified and middle clicks following the shared enlarged-surface rule in `family:navigation-destinations`. The Chevron and controls in Start content and End content keep their own activation, and a disabled Item does nothing. | Owner decision; `family:navigation-destinations` | Settled intent; implementation may be staged | | ||
| | FR8 | Expanding or collapsing an Item with children MUST play its group's own motion: under a coarse primary pointer its rows slide out from under the parent as one block, clipped to the group's box, and back on collapse, a collapsing group's rows leaving the accessibility tree as the slide starts, while the Chevron turns, by transform only (`spec:AST-094` FR4–FR5); under a fine pointer the group appears and leaves at once. With an `animatedLayout` handle, the group and the rows after it follow `spec:AST-105`'s shared motion under every pointer (FR4, FR5); without one, the group's own slide stays touch-only and the rows after the group take their places at once (`spec:AST-105` FR2): as an expanding group appears and, under a coarse primary pointer, as a collapsing group's rows finish sliding under the parent, until which they hold their places. | UIKit outline list (`UICollectionView`), iOS 26.2, whose rows after a closing group hold until its rows have slid under the parent (0.24–0.37 s measured); `spec:AST-105` FR2 | Proposed; awaiting an engineering owner's approval | |
There was a problem hiding this comment.
This rewords FR8, still "Proposed; awaiting an engineering owner's approval" on main: without a handle, the rows after a collapsing group now hold until its rows have slid under the parent, and the verification row (L279) fails rows that move earlier. An engineering owner records the decision by commenting /approve-spec with this head's full SHA on the pull request (the author may, as an engineering owner), or FR8 is settled through #8088 first.
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: this head carries #8088's rewording of component:TreeList FR8, still proposed on main, so that under a finger the rows after a collapsing group hold until its rows have slid under the parent. An engineering owner records that decision by commenting /approve-spec 8134a0f642fbb6979aefb81bf356a21d065714cb on this pull request (the author may do so as an engineering owner), or FR8 is settled through #8088 first. The List change itself follows component:List FR12; after that decision, two changes remain:
- Land the eight predecessor pull requests first, at their current heads (#8360 has moved past the commit this branch carries), so this diff is the 2,067-line List change rather than 36,093 authored lines.
- Report the base-against-head size of every component the diff changes, or let the existing size line cover it once the predecessors land.
[Automated review]
| | FR5 | `TreeListItemData` MUST admit `className`, `style`, and `xstyle` as one per-item styling seam. Every supplied value MUST compose on the same Item row box; an implementation MUST NOT treat current support for only one mechanism as authority to exclude the other two. | Owner decision; `architecture:public-component-api` INV5–INV6; `spec:AST-002/DEC-1` | Settled intent; implementation may be staged | | ||
| | FR6 | The Item row box is the logical element that paints the row, owns its hover state, and contains Start content, Item label and description, and End content. The styling seam MUST NOT move to the semantic `<li>` or another element without a compatibility decision. | Owner decision and the caller-owned per-row interaction boundary | Settled intent | | ||
| | FR7 | A click on an Item row outside its Item label performs the row's action: an Item with `onClick` runs it; otherwise an Item with children toggles; otherwise an Item whose only action is its link follows that link as Enter does, with modified and middle clicks following the shared enlarged-surface rule in `family:navigation-destinations`. The Chevron and controls in Start content and End content keep their own activation, and a disabled Item does nothing. | Owner decision; `family:navigation-destinations` | Settled intent; implementation may be staged | | ||
| | FR8 | Expanding or collapsing an Item with children MUST play its group's own motion: under a coarse primary pointer its rows slide out from under the parent as one block, clipped to the group's box, and back on collapse, a collapsing group's rows leaving the accessibility tree as the slide starts, while the Chevron turns, by transform only (`spec:AST-094` FR4–FR5); under a fine pointer the group appears and leaves at once. With an `animatedLayout` handle, the group and the rows after it follow `spec:AST-105`'s shared motion under every pointer (FR4, FR5); without one, the group's own slide stays touch-only and the rows after the group take their places at once (`spec:AST-105` FR2): as an expanding group appears and, under a coarse primary pointer, as a collapsing group's rows finish sliding under the parent, until which they hold their places. | UIKit outline list (`UICollectionView`), iOS 26.2, whose rows after a closing group hold until its rows have slid under the parent (0.24–0.37 s measured); `spec:AST-105` FR2 | Proposed; awaiting an engineering owner's approval | |
There was a problem hiding this comment.
This rewords FR8, still "Proposed; awaiting an engineering owner's approval" on main: without a handle, the rows after a collapsing group now hold under a coarse primary pointer until its rows have slid under the parent, and the verification row (L279) fails rows that move earlier. An engineering owner approves or rejects this wording by commenting /approve-spec with this head's full SHA on the pull request, or it is settled through #8088 first.
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: this head carries #8088's rewording of component:TreeList FR8, still proposed on main, so that under a finger the rows after a collapsing group hold until its rows have slid under the parent. An engineering owner records that decision by commenting /approve-spec 0535acf7004d0cbc538e836f2344030f1557d6bc on this pull request (the author may do so as an engineering owner), or FR8 is settled through #8088 first. The List change itself follows component:List FR12, and the latest push's animator and press-release fixes come with tests that fail before them; after that decision, two changes remain:
- Land the eight predecessor pull requests first, so this diff is the 2,067-line List change rather than 36,239 authored lines.
- Report the base-against-head size of every component the diff changes, or let the existing size line cover it once the predecessors land.
[Automated review]
| | FR5 | `TreeListItemData` MUST admit `className`, `style`, and `xstyle` as one per-item styling seam. Every supplied value MUST compose on the same Item row box; an implementation MUST NOT treat current support for only one mechanism as authority to exclude the other two. | Owner decision; `architecture:public-component-api` INV5–INV6; `spec:AST-002/DEC-1` | Settled intent; implementation may be staged | | ||
| | FR6 | The Item row box is the logical element that paints the row, owns its hover state, and contains Start content, Item label and description, and End content. The styling seam MUST NOT move to the semantic `<li>` or another element without a compatibility decision. | Owner decision and the caller-owned per-row interaction boundary | Settled intent | | ||
| | FR7 | A click on an Item row outside its Item label performs the row's action: an Item with `onClick` runs it; otherwise an Item with children toggles; otherwise an Item whose only action is its link follows that link as Enter does, with modified and middle clicks following the shared enlarged-surface rule in `family:navigation-destinations`. The Chevron and controls in Start content and End content keep their own activation, and a disabled Item does nothing. | Owner decision; `family:navigation-destinations` | Settled intent; implementation may be staged | | ||
| | FR8 | Expanding or collapsing an Item with children MUST play its group's own motion: under a coarse primary pointer its rows slide out from under the parent as one block, clipped to the group's box, and back on collapse, a collapsing group's rows leaving the accessibility tree as the slide starts, while the Chevron turns, by transform only (`spec:AST-094` FR4–FR5); under a fine pointer the group appears and leaves at once. With an `animatedLayout` handle, the group and the rows after it follow `spec:AST-105`'s shared motion under every pointer (FR4, FR5); without one, the group's own slide stays touch-only and the rows after the group take their places at once (`spec:AST-105` FR2): as an expanding group appears and, under a coarse primary pointer, as a collapsing group's rows finish sliding under the parent, until which they hold their places. | UIKit outline list (`UICollectionView`), iOS 26.2, whose rows after a closing group hold until its rows have slid under the parent (0.24–0.37 s measured); `spec:AST-105` FR2 | Proposed; awaiting an engineering owner's approval | |
There was a problem hiding this comment.
This rewords FR8, still "Proposed; awaiting an engineering owner's approval" on main: without a handle, the rows after a collapsing group now hold under a coarse primary pointer until its rows have slid under the parent, and the verification row (L279) fails rows that move earlier. It also ties the hold to the slide, while under Reduce Motion those rows fade instead and the code holds the rows after the group until the fade ends. An engineering owner can approve this wording with /approve-spec and this head's full SHA, or FR8 can be settled through #8088 first.
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: this head carries #8088's rewording of component:TreeList FR8, still proposed on main, so that under a finger the rows after a collapsing group hold until its rows have slid under the parent. An engineering owner records that decision by commenting /approve-spec 0c59447f97aade3afceb1291c6ed40f0b2a0cc9a on this pull request (the author may do so as an engineering owner), or FR8 is settled through #8088 first. The List change itself follows component:List FR12, and the latest push's animator and press-overlay fixes come with tests that fail before them; after that decision, two changes remain:
- Land the eight predecessor pull requests first, so this diff is the 2,067-line List change rather than 36,338 authored lines.
- Report the base-against-head size of every component the diff changes, or let the existing size line cover it once the predecessors land.
[Automated review]
|
/approve-spec 5787619 |
There was a problem hiding this comment.
Changes requested. With a handle, the List change covers removals, inserts and moves under component:List FR12, but three things need fixing first:
- Miss: adding the first row to a list with no keyed rows (an empty list, or one with only unkeyed rows) plays no entrance and the content after the list jumps, because
measurereturns early atpackages/core/src/List/listLayoutUpdates.tsL372-374, while FR12 asks an inserted row to enter and the content after it to glide. - Regression:
docs/specs/components/List/ListItem.spec.mdstill saysitemKeyis specified but not shipped (L39) and lists it as a known gap (L76-78), so update that record in this pull request now thatListItemacceptsitemKey. spec:AST-105FR5 asks each adopting pull request to show mouse behavior before and after, and the description shows touch recordings only, so please add mouse recordings of the Animated changes story without and with the handle.
Two optional comment cleanups are noted inline. CI has not run on this head yet.
[Automated review]
| if (rows.size === 0) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Miss: with no keyed rows this returns before anything is measured, so the first row added to an empty list (or the first keyed row among unkeyed ones) plays no entrance, and the content after the list jumps. That breaks component:List FR12. Report a target anyway (the list's last child, or the list itself when it is empty) and keep an empty measurement so the layout effect plays the entrance. A 0-to-1 case in listLayoutUpdates.test.tsx would cover it.
There was a problem hiding this comment.
Fixed in 09cd1f2. With no keyed rows, the list now reports its last child, or itself when empty, so the first keyed row enters and the content after the list glides. Two new unit tests fail without the fix.
| * `animatedLayout`. A row without one changes at once. It is separate from | ||
| * React's `key`, which stays React's own. | ||
| */ | ||
| itemKey?: string | number; |
There was a problem hiding this comment.
Regression: docs/specs/components/List/ListItem.spec.md still says itemKey is "specified but not shipped" (L39) and lists "ListItem has no itemKey yet" as a known gap (L76-78). Please update that record here: list the prop as shipped, drop the gap and add the new tests to verified_by.
There was a problem hiding this comment.
Updated in 09cd1f2: itemKey is in the public contract, the gap is gone, and verified_by lists listLayoutUpdates.test.tsx.
| // How it runs, step by step, through the one size-change module. | ||
| // - Before a commit that changes the rows, LayoutChangeSnapshot calls | ||
| // `measure`, before React changes the DOM. It reports each row in view, | ||
| // since any of them may leave and the module exits only what it measured. | ||
| // It also reports the last row, so the content after the list glides when | ||
| // no row is in view. The module's walk takes the siblings on both sides of | ||
| // each reported row, up to its band around the visible part. So the rows | ||
| // near the view and the content after the list glide from their old | ||
| // places. A long list's rows far from the view are never read. Rows sit in | ||
| // order down the list, so halving finds the rows in view. When only one row | ||
| // is in view and the last row is far from it, no reported row's walk | ||
| // reaches the last row. So an insert above the last row moves it at once. | ||
| // - After the commit, rows are matched by their `itemKey`. A row that left | ||
| // comes back where it was and exits in place. It gives up its space, is | ||
| // inert and hidden from assistive technology, and conceals upward | ||
| // (`holdLayoutExit`), then it goes. A row that appeared enters | ||
| // (`playLayoutChange`). The rows that stay glide. All of it plays on the | ||
| // shared motion role and starts in the same frame. | ||
| // - A row that slides up across rows moving down rides above them for its | ||
| // glide. The list's surface is behind it, as an opaque UIKit cell passes | ||
| // over its neighbors. | ||
| // - It runs for every pointer. Reduce Motion follows the module's policy. | ||
| // - Without a handle nothing is measured, and the list changes at once. A row | ||
| // without an `itemKey` has no identity to match. Its own insertion, removal | ||
| // or move changes at once, though it glides like any content when a keyed | ||
| // row's change moves it. |
There was a problem hiding this comment.
Non-blocking (comment style): this block narrates the code below it. Keep only the constraints, for example: "The module exits only rows it measured, so every row in view is reported. The last row is reported too, so content after the list glides when no row is in view."
| // The row's identity for the list's animated layout | ||
| // (List/listLayoutUpdates.ts). |
There was a problem hiding this comment.
Non-blocking (comment style): this restates itemKeyProps(itemKey); it can go.
|
/approve-spec 502832a |
There was a problem hiding this comment.
Changes requested: this head fixes the three earlier items (the first keyed row, the ListItem record and the mouse evidence), but two defects in the row updates remain.
- Miss: under React
<StrictMode>in development, ListItem's unmount Effect (packages/core/src/List/ListItem.tsxL419-436) replays while an inserted row's entrance plays, andkeepPresentationcommits the entrance's first frame inline, so the row stays atopacity: 0after the entrance ends, againstarchitecture:react-component-runtimeINV3's rule that an Effect is safe under StrictMode replay. - Miss:
markCrossings(packages/core/src/List/listLayoutUpdates.tsL326-331) raises only rows gliding up, so a row moved from first to last slides under the four rows it crosses and is hidden for most of its move, whilecomponent:ListFR12 says a row crossing rows moving the other way rides above them.
Please add a StrictMode insert case and a "move the first row last" case with the fixes. One optional comment-style suggestion is inline. CI has not run on this head yet, and the fix commit will need a fresh /approve-spec for the spec-owner check.
[Automated review]
| // present inline while it is still in the document. A parent's layout | ||
| // cleanup runs before React removes the row, so the removal's collapse | ||
| // shows the row as it was on screen. | ||
| useIsomorphicLayoutEffect(() => { |
There was a problem hiding this comment.
Miss: under <StrictMode> (development), React replays this Effect for a newly inserted row right after the commit, while its entrance plays. keepPresentation then commits the entrance's first frame inline (opacity: 0, clip-path: inset(0 0 100%)), and the row stays invisible after the entrance ends. Record what the animations show without writing it to the row (for example saveAnimations from motion/carryAnimations.ts, applied in putBack), or keep the presentation only of rows the current commit measured, and add a StrictMode insert case.
There was a problem hiding this comment.
Fixed in 6e95066. The cleanup now keeps the presentation only of a row its list reported for the commit in progress, the only rows that can exit. A StrictMode replay on a new row runs after the list's layout effect clears that set, so it writes nothing. The new StrictMode insert case fails without the fix.
| } | ||
| if (to > from) { | ||
| lowestDown = Math.max(lowestDown, to); | ||
| } else if (lowestDown > to) { |
There was a problem hiding this comment.
Miss: only a row gliding up is raised. Moving the first row last raises the four rows moving up (z-index: 2, opaque surface fill), and the moved row passes beneath them, hidden for most of its move, while component:List FR12 says a row crossing rows moving the other way rides above them. Raising the row with the longest glide among those it crosses covers both directions; a "move the first row last" case would pin it.
There was a problem hiding this comment.
Fixed in 6e95066. Of two rows that cross, the longer glide rides above (on a tie, the row gliding up). One pass each way finds the rows that cross any other, and only those are compared, so a removal still costs one pass. The new "moving the first row last" case fails without the fix.
| // How it runs, step by step, through the one size-change module. | ||
| // - Before a commit that changes the rows, LayoutChangeSnapshot calls | ||
| // `measure`, before React changes the DOM. It reports each row in view, | ||
| // since any of them may leave and the module exits only what it measured. | ||
| // It also reports the last row, so the content after the list glides when | ||
| // no row is in view. The module's walk takes the siblings on both sides of | ||
| // each reported row, up to its band around the visible part. So the rows | ||
| // near the view and the content after the list glide from their old | ||
| // places. A long list's rows far from the view are never read. Rows sit in | ||
| // order down the list, so halving finds the rows in view. When only one row | ||
| // is in view and the last row is far from it, no reported row's walk | ||
| // reaches the last row. So an insert above the last row moves it at once. | ||
| // - After the commit, rows are matched by their `itemKey`. A row that left | ||
| // comes back where it was and exits in place. It gives up its space, is | ||
| // inert and hidden from assistive technology, and conceals upward | ||
| // (`holdLayoutExit`), then it goes. A row that appeared enters | ||
| // (`playLayoutChange`). The rows that stay glide. All of it plays on the | ||
| // shared motion role and starts in the same frame. | ||
| // - A row that slides up across rows moving down rides above them for its | ||
| // glide. The list's surface is behind it, as an opaque UIKit cell passes | ||
| // over its neighbors. | ||
| // - It runs for every pointer. Reduce Motion follows the module's policy. | ||
| // - Without a handle nothing is measured, and the list changes at once. A row | ||
| // without an `itemKey` has no identity to match. Its own insertion, removal | ||
| // or move changes at once, though it glides like any content when a keyed | ||
| // row's change moves it. |
There was a problem hiding this comment.
Non-blocking (comment style): this block still narrates the code below it. Keep only the constraints, for example: "The module exits only rows it measured. A row without an itemKey has no identity, so its own change lands at once."
|
/approve-spec 6e95066 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, because the pull request is a draft from a fork; mark it ready for review and have a maintainer add the ci:run label so CI runs.
This head fixes both defects raised earlier: an inserted row now stays visible under React <StrictMode>, and a row moved down now rides above the rows it crosses, each with a unit case that fails before the fix. With a handle and an itemKey on each row, inserts, removals and moves animate as component:List FR12 describes, and the new browser spec passes in Chromium.
[Automated review]
|
/approve-spec fab2359 |
There was a problem hiding this comment.
Changes requested: one defect in the row updates needs fixing first.
- With a handle, a
ListItemwhoserefchanges during a commit (for example a callback ref written inline) has its running glide and entrance written inline bykeepPresentation(packages/core/src/List/ListItem.tsxL445), so re-rendering the list while rows glide leaves them 21 px off their places and the new row half-faded and clipped, againstcomponent:ListFR12; keep the presentation only for a row that actually leaves, and add a case where a row'srefchanges mid-glide.
Everything else follows component:List FR12, and the new unit and Chromium browser cases pass on this head. CI has not run on this head yet. One optional comment-wording suggestion is inline.
[Automated review]
| (onClick != null || href != null || interactiveRef != null) && !isDisabled; | ||
| const hitAreaRef = useTouchHitArea(isTouchTarget); | ||
| const mergedRef = useMergedRefs(ref, hitAreaRef); | ||
| const mergedRef = useMergedRefs(ref, hitAreaRef, keepPresentationOnDetach); |
There was a problem hiding this comment.
This cleanup also runs when the merged ref changes, not only when the row leaves: useMergedRefs returns a new ref whenever the caller's ref (for example ref={row => …} written inline) or the hit-area ref changes. React runs the old cleanup in the commit's mutation phase, while reportedRows still holds the row, so keepPresentation commits the row's running glide and entrance inline. In Chromium, an insert followed 120 ms later by a status update left Sent, Drafts and Archive at translate: 0px -21px and the new row at opacity: 0.43 for good. Recording what the animations show without writing it (and writing it in putBack), or an unmount-only cleanup as the previous revision had, keeps this to rows that leave.
There was a problem hiding this comment.
Fixed in 08652ac. The detach now only records what the row's animations show: it reads them through commitStyles, then restores each part's style. The exit writes the record, so a row that stays is left untouched. The new case, a re-render mid-glide with a new ref on each row, fails without the fix.
| * The list's `animatedLayout` handle, for its rows. Internal: a row's own | ||
| * layout changes, such as its start and end content, report to the same | ||
| * handle. |
There was a problem hiding this comment.
Non-blocking (comment style): nothing reads this field at this head, so the second sentence describes behavior that does not exist yet. "The list's animatedLayout handle, for its rows." is enough until the start and end content motion lands.
A List that takes the handle useAnimatedLayout returns animates its row changes on one shared motion, for every pointer. An inserted row enters, a removed row collapses in place, and a row that crosses rows moving the other way rides above them. The rows after a change, and the content after the list, glide to their new places. Rows match across renders by their itemKey; a row without one changes at once. The row marks the update module writes (item key, leaving, travelling) live in their own private-attribute owner modules. A list under the handle isolates for every pointer, so a sliding row stays below a page-level sticky header. The List and ListItem records now say the row updates and itemKey are implemented, and their verified_by lists the files that verify them.
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, because the pull request is a draft from a fork. Once it is marked ready, a maintainer adds the ci:run label so CI runs.
With an animatedLayout handle and an itemKey on each row, List animates inserts, removals and moves as component:List FR12 describes, and a row whose ref changes while rows glide now ends at its place; the new unit case covers it. The unit tests and the browser spec pass on this head in Chromium.
- Non-blocking:
component:ListFR12 does not say where focus goes when a removed row held it, so a keyboard user who removes a row from a control inside it lands on the page body; the record could state it.
[Automated review]
|
/approve-spec 08652ac |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, because the pull request is a draft from a fork. Once it is marked ready, a maintainer adds the ci:run label so CI runs.
With an animatedLayout handle and an itemKey on each row, List animates inserts, removals and moves as component:List FR12 describes. The merge of main moves those glides to the theme's move curve, and the unit tests and the browser spec pass against it in Chromium.
- Non-blocking: the recordings and test runs the description marks "at this head" predate that merge, when the glide was a spring; a fresh mouse recording would show the motion that now ships.
- Non-blocking:
component:ListFR12 does not say where focus goes when a removed row held it, so a keyboard user who removes a row from a control inside it lands on the page body; the record could state it.
[Automated review]
Owner decision, Nov 6 2026 (Cindy): the layout glide takes --duration-medium. The shared glide played over --duration-medium-min; it now plays over --duration-medium: 300 ms in the Neutral theme and 410 ms with no theme, instead of 225 ms and 310 ms, both scaled by tempo. Collapsible, TreeList, Table's row expansion, AppShell's content area and List share it. AST-105 FR4 names the token, and its DEC-3 records the decision.
|
/approve-spec fe1f020 |
There was a problem hiding this comment.
Changes requested: the List row updates follow component:List FR12, and the glide-duration change carried from #10261 has an engineering owner's approval at this head, but two things need fixing first.
- Newly live check:
measureinlistLayoutUpdates.ts(L421-429) reads each near row's computed transform and rect after writing the previous rows' settles, whicharchitecture:react-component-runtimeINV18 rules out; read every near row's transform and rect in one pass, then write the settles, and take a settled row's top from its rect minus its own translation. - The diff is 2,513 authored lines because it carries #10261's separable 84-line glide-duration change; land #10261 first and merge
main, so this pull request is the 2,429-line List change, which its decomposition already clears.
CI has not run on this head because the pull request is a draft from a fork; once it is marked ready, a maintainer adds the ci:run label so CI runs.
[Automated review]
| for (let index = near[0]; index <= near[1]; index++) { | ||
| const done = settleTranslation(elements[index]); | ||
| if (done != null) { | ||
| settled.push(done); | ||
| } | ||
| } | ||
| for (let index = near[0]; index <= near[1]; index++) { | ||
| tops.set(order[index], elements[index].getBoundingClientRect().top); | ||
| } |
There was a problem hiding this comment.
Newly live check: settleTranslation reads a row's and its children's computed transform and then writes them, one row at a time, so every row after a translated one is read after a write, and the tops loop reads every rect after those writes. Current architecture:react-component-runtime INV18 fails "a read interleaved with writes". Read every near row's transform and rect in one pass first, then write the settles, and take a settled row's top from that rect minus the row's own translation.
Settling a swiped row read the next row's transform after writing the row before it, and read every row's place after all the writes. One pass now reads each near row's translation and place, and then the settles are written. A settled row's place is its box less its own translation.
|
/approve-spec b79fde9 |
There was a problem hiding this comment.
Changes requested: the List row updates follow component:List FR12 and the reads between settles are fixed, but two things need fixing first.
- Miss:
measureinlistLayoutUpdates.tsstill reads layout after writing the settles, inrowsMeeting(elements, 0, height)(L468) and each target'smeasureBefore(L478-481), so a change with a swiped row near the view lays the page out once more before the commit, whicharchitecture:react-component-runtimeINV18 rules out; take the rows in view from the first pass, callmeasureBeforethere, then write the settles. - The diff is 2,633 authored lines because it carries #10261's separable 84-line glide-duration change; land #10261 first and merge
main, so this pull request is the List change alone.
CI has not run on this head because the pull request is a draft from a fork.
[Automated review]
| // The module exits only rows it measured, so every row in view is | ||
| // reported. The last row is reported too, so content after the list | ||
| // glides when no row is in view. | ||
| const shown = rowsMeeting(elements, 0, height); |
There was a problem hiding this comment.
Miss: this reads every row's rect after the settles above (L461-463) have written row and child transforms, and the measureBefore calls below (L478-481) read after them too. So a change with a swiped row near the view lays the page out once more before the commit: a Chromium trace of a move with two translated rows shows 2 Layouts in the click task instead of 1. Current architecture:react-component-runtime INV18 fails "a read interleaved with writes". Take the rows in view from the first pass's rects (top and bottom less the translation already read), call each target's measureBefore in that pass, then write the settles.
| * translation moves. `readTranslation` reads it, and `settleTranslation` | ||
| * moves it, so a list reads every row before it writes any. Returns the |
There was a problem hiding this comment.
Non-blocking (INV29): this comment still describes moving the translation, which settleTranslation does, and says "a list reads every row before it writes any", which rowsMeeting at L468 contradicts. Suggested: "Reads a row's own translation and its children's transforms. Returns null when the row has no translation of its own."
Stack: #8106 → #8139 → #8143 → #8163
Stacked on #10261 until it lands; review only this PR's change: vjeux/astryx@8982806...b79fde9
Batch: a behavior module, 2,429 authored lines: tests 1,473 (unit 661, browser 812), product code 767, stories 152, docs and records 37.
User need
On iOS, when a table's rows change, the others slide to their new places, so you see what changed. A List's rows jump in one frame.
Problem and solution fit
With an
animatedLayouthandle and anitemKeyon each row (component:ListFR12), an inserted row enters and a removed row collapses in place, inert and hidden from assistive technology. A row crossing others rides above them. The rows after a change and the content after the list glide, on one frame of the shared motion, for every pointer. Without anitemKeyor a handle, rows change at once. The first keyed row of a list that had none enters too.Current specification
component:ListFR12;spec:AST-105FR1–FR5;spec:AST-103DEC-4. List's readiness row andcomponent:ListItemnow list the row updates anditemKeyas shipped; the start and end content motion is not yet.Public delta and caller burden
<List animatedLayout={useAnimatedLayout({containerRef})}>with<ListItem itemKey={id}>, both optional.Design and alternatives
Evidence
iOS 26.2 simulator, touch, at an earlier head (0c59447):
A mouse in Chromium, without the handle (main) and with it:
Size (gzip, main → head): List +9,791 B JS and +143 B CSS; ListItem +254 B JS and +81 B CSS. Components on List add 8.4–9.7 KB JS, the shared animated layout module TreeList already ships.
Writing rubric results
R1 narration: none. R2 research: none. R3 evidence: both
verified_bylists add their new test files. R4 rejected alternatives: none. R5 change references: none. R6 open questions: none.Scope
Testing
What was run at this head: test:ui (885 files, 21,802 tests) with no unhandled errors, including four List cases that fail without this head's fixes. They cover the first keyed row of a list with none (it played no entrance), a row moved from first to last (it slid beneath the rows it crossed), and a row inserted under StrictMode (React's replay of its detach left it at opacity 0). A fifth removes a row under StrictMode: it keeps what it showed, and fails if the detach keeps nothing. A sixth re-renders while rows glide, with a new ref on each row: no row is left with its motion written on it. A seventh changes a list with two swiped rows apart: it reads every row's place before it settles either swipe. Also: List's browser specs in Chromium on a 2-core pin (45 passed) and in the CI WebKit profile (34 passed, its Chromium-only cases skip); the core type check, the Storybook build, review:pre and check:repo. At an earlier head of this change: strict lint (0 errors), both builds, Storybook's type check, test:node (914 files), the accessibility audit on both new stories (no new violations), Item's swipe accessibility contracts (8 passed), and the new spec at 3 CPUs and 16 workers in WebKit and ×10 in the CI WebKit profile (90 passed).