Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
|
/approve-spec 3d0c0b7 |
There was a problem hiding this comment.
Requesting changes: the resize retarget and the split end behave as described in Chromium, and an engineering owner approved both record changes for this head, but three fixes remain.
- After items are added or removed without the row resizing, the item whose stop the end page took keeps
scroll-snap-align: noneeven once its stop sits more than half a step from the end, so keyboard and wheel scrolling skip a stop that a flick counts as a page (SnapPaging.ts:458-463); re-measure the paging when the row's children change. - The new
spec:AST-094FR6 sentence says a resize "retargets" the settle, butmeasure()ends it at once on the new page (a 218 px jump in Chromium), which FR6's own "A new target MUST keep the current velocity" rules out; reword the sentence to say the settle ends at once, or keep the velocity in code. component:CarouselFR5 ("native snap's per-item stops") and FR7 ("each direct child's start edge") still say every item is a stop, which contradicts the new "Split end" state; amend them to name that exception.
Non-blocking suggestions are inline.
[Automated review]
| // The same page, renumbered: the owner hears it only through a | ||
| // release's report still waiting. | ||
| if (pendingReport == null) { | ||
| report('controlled'); |
There was a problem hiding this comment.
Non-blocking: with a controlled index, this silent sync leaves the owner's index naming another page, and usePager's controlled sync never re-runs because no onIndexChange fires; a later index equal to the new number then moves nothing. No caller passes a controlled index with child pages today, but the module doc's "A controlled index wins" no longer holds here.
There was a problem hiding this comment.
Left as is: no caller passes a controlled index with child pages. A caller that does would need its own sync, in a separate change.
3d0c0b7 to
6f396fd
Compare
|
/approve-spec 6f396fd |
There was a problem hiding this comment.
Requesting changes. The Carousel fix works, and an engineering owner approved the spec:AST-094 FR6 change for this head, but one blocking issue remains.
retargetSettle()inusePager.tsrestarts a uniform pager's settle from the old layout's pixel offset, so a Lightbox gallery resized or rotated mid-settle first shows an earlier photo (when the stage widens) or empty space past the last photo (when it narrows near the end) and then slides, where the base shows the target photo at once; keep the shown page fraction for uniform pages by scaling the offset and velocity by the new stride over the old one (or keep the immediate landing there), and add a widen and a narrow uniform-pager test.
Two smaller notes are non-blocking: the comment above the renumbering block restates the branches below it, and the silent renumber leaves a controlled index naming the old number.
[Automated review]
6f396fd to
95aa8c8
Compare
There was a problem hiding this comment.
Requesting changes. Last round's two findings are fixed (uniform pages now keep their fraction of a page through a resize, and a release's report keeps its two-frame wait), and the spec:AST-094 FR6 change carries spec-owner approval at this head, but one blocking issue remains.
- Miss:
measure()remaps a renumbered settle to the page nearest its old pixel offset and replays child pages from the old pixels (usePager.ts:996-1011,:1036-1037), so a Carousel snap-paging row whose items follow the row or viewport width (cqi,vw) rests on the item the person left when it widens mid-swipe, and skips an item when it narrows, where the base rests on the swiped-to item; map the target and the shown position through the item they sit on (for example, keep the index and scale by the page fraction when the page count is unchanged, as the uniform branch does) and add a widen and a narrow case with viewport-relative items.
The silent renumber under a controlled index and the repeated animated-scroll solve stay non-blocking.
[Automated review]
95aa8c8 to
c4319b4
Compare
There was a problem hiding this comment.
Approval waits on an engineering-owner decision on the spec:AST-094 FR6 change, and two fixes are needed either way.
Spec decision: this head adds a new FR6 requirement, "A resize during a settle retargets it. With as many pages as before, the settle keeps its page and its fraction of a page. With another count, a settle toward the last page ends on the last page, and any other on the page nearest its target's offset", with its "FR6 resize" verification row. The earlier /approve-spec comments name earlier heads, and the same-count sentence is new here. An engineering owner records the decision by commenting /approve-spec c4319b445d09484836f1d5f737952e75ef16ff61; the author may do so as an engineering owner.
Blocking:
- In
usePager.ts(lines 1013-1026), a resize that keeps the page count maps the shown position by its page fraction, so a fixed-width view-paging Carousel whose views regroup (the three-per-view story widened from 300 to 400 px) jumps 94 px in one frame mid-settle, which FR6's retarget rule and its "a jump … fails" verification row forbid; decide "same pages" from the items (or from offsets that scaled with the row), keep the pixels otherwise, and add a fixed-width unit case. - The changeset says the row "no longer jumps", but the fixed-width same-count case above still jumps, so fix the jump or narrow the sentence to the cases the tests cover.
The twelve new unit cases and the two new browser cases fail on main and pass on this head in Chromium.
[Automated review]
|
/approve-spec c4319b4 |
There was a problem hiding this comment.
Requesting changes for two fixes. An engineering owner approved the spec:AST-094 FR6 resize change for this head, and in Chromium the pager does what that sentence says; the record change can stay as it is.
Blocking:
- Miss: a still finger that lands on a release's settle no longer stops it when the row resizes inside the 85 ms catch window, because
retargetSettle()inusePager.tscallsstopSettle(), which clears the armed catch, so the settle slides on under the finger against FR6's "A finger MUST be able to catch any animation mid-flight"; keep the pending catch across the retarget and add a unit case for it. - The changeset (
.changeset/pager-resize-settle-retarget.md, line 5) says a resized row "no longer jumps" and plays on "from where it shows", but a fixed-width three-per-view row widened from 300 to 400 px mid-settle keeps its page count and still steps 94 px in one frame, so narrow it, for example to "no longer stops at once or runs back to an earlier page. The settle plays on at its speed, to the last page when it was heading there, and otherwise to the page nearest where it was heading."
The earlier comment on the same-count step in usePager.ts is settled by the approved FR6 sentence. The new unit and browser tests fail on main and pass on this head in Chromium.
[Automated review]
c4319b4 to
c081881
Compare
|
/approve-spec c081881 |
There was a problem hiding this comment.
Requesting changes for three fixes. An engineering owner approved the spec:AST-094 FR6 resize change for this head, so the record change can stay as it is, and rows whose pages renumber, and Lightbox's uniform pages, now play on through a resize as it says.
Blocking:
- A snap-paging Carousel whose items follow the row's width (for example
width: 100%), widened mid-swipe, turns back and rests on the item the person left, becausescaledPages()inusePager.tsneeds every page offset within 0.5 px of one ratio while Carousel measures its stops on the moving row, where each stop carries the distance the row moved between the frame and the read; make those stops agree with the row's rects (or let the fit allow one shared shift) and add a browser case with row-sized items. - Miss: a still finger that lands on a release's settle no longer stops it when the row resizes inside the 85 ms catch window, because
retargetSettle()callsstopSettle(), which disarms the armed catch, against FR6's "A finger MUST be able to catch any animation mid-flight"; keep the armed catch across the retarget, as the waiting report is kept, and add a unit case. - The changeset (
.changeset/pager-resize-settle-retarget.md, line 5) says a resized row no longer runs back to an earlier page, which the first finding contradicts; fixing that finding makes it true, otherwise narrow the sentence to rows whose pages renumber.
Required CI has not run on this head yet.
[Automated review]
c081881 to
78edbda
Compare
There was a problem hiding this comment.
Approval waits on an engineering-owner decision on the spec:AST-094 FR6 record change, and two fixes are needed either way.
The FR6 resize sentence and its "FR6 resize" verification row add a requirement the current record does not state, and the last spec approval named an earlier head; this head adds two cases to the row and changes how "scale by one ratio" is judged. An engineering owner records the decision by commenting /approve-spec 78edbda58c9fd5b6714b981baaeec162f2a9cec0 on this pull request; the author may do so as an engineering owner.
Blocking:
- A snap-paging Carousel whose items follow the row's width, flicked from item 2 toward item 3 and widened from 300 to 450 px about 100 ms after the lift, still turns back and rests on item 2 in Chromium (7 of 7 runs; the base rests on item 3), because
scaledPages()inusePager.tsjudges stops measured on the moving row, which an instrumented run read as[0, 640, 1090, 1350, 1800]for items at[0, 450, 900, 1350, 1800], and a stop clamped to the row's end never carries a shared shift (a variant of the shared-shift unit case with 0.7 px instead of 0.56 px rests on page 1); make the held row's stops agree with what it draws, or decide "the same pages" from layout that does not move, and add a browser case with row-sized items widened as the settle starts. - The changeset (
.changeset/pager-resize-settle-retarget.md, line 5) says a resized row no longer runs back to an earlier page, which the first finding contradicts; fixing that finding makes it true, otherwise narrow the sentence to rows whose pages renumber.
A still finger now catches a release's settle through a resize, as FR6 requires. Required CI has not run on this head yet.
[Automated review]
78edbda to
f4dde6e
Compare
A resize that renumbered a children pager's pages mid-settle left the settle on its old index, which then named another page: a row of three per view narrowed mid-settle ran back to the page it had left. A resize mid-settle also ended the settle at once, with a jump to the new page. When every page offset scales by one ratio, as when items or uniform pages follow the row's width, each page keeps its number and the settle keeps its fraction of a page. Otherwise the settle retargets by offset and plays on from where the track shows, at its velocity. A settle toward the last page ends on the last page, a target halfway between two pages goes on the way the settle moves, and the page last at rest keeps its place too. The retarget reads the track and starts the new curve at one time. A release's waiting report still goes out two frames after the new settle starts, and a finger's waiting catch still catches it. The held row's stops and span came from the children's rects plus the animator's position now, which runs ahead of the frame the row draws: a row whose items follow its width turned back after a widening, and a retargeted settle ended past its page. Each child's own translate now comes off its rect, read right after it.
f4dde6e to
35eba30
Compare
|
/approve-spec 35eba30 |
There was a problem hiding this comment.
Approval waits on an engineering-owner decision on the spec:AST-094 FR6 record change, and one split is needed either way.
The FR6 resize sentence and its "FR6 resize" verification row add a rule the current record does not state, including whether "scale by one ratio" means the page offsets, as written, or the spacings between pages, as scaledPages() checks (pages at 16/316/616/916 widened to 16/466/916/1366 keep page 2 in the code and get page 1 by the sentence). An engineering owner records the decision by commenting /approve-spec 35eba30a0c1c647f1aca6bf7d0a1ebc217066d14 on this pull request, and the author may do so as an engineering owner.
Blocking:
- The change is 2,034 authored lines, over the 2,000-line limit, and its held-row stop and span fix (
SnapPaging.ts,snapPagingValue.ts,SnapPaging.test.tsxand its two browser cases, about 500 lines) works on its own: open it as its own pull request, land it first, and rebase this one onto it.
The row-sized run-back from the last review is fixed in Chromium, and required CI has not run on this head yet.
[Automated review]
| on a pager's settle after a release never reaches the pager's content: no | ||
| press feedback and no click. UIKit's content took none of 30 such touches | ||
| change during a drag starts from rest (DEC-25). A resize during a settle | ||
| retargets it. When the page offsets all scale by one ratio, the settle |
There was a problem hiding this comment.
Part of the spec decision: scaledPages() in usePager.ts compares the spacings between pages, not the offsets. So a row whose first page is not at 0 behaves differently from this sentence. Pages at 16/316/616/916 widened to 16/466/916/1366 during a settle toward page 2 rest on page 2 in the code, but the "Otherwise" branch here gives page 1. If the code's rule is the intended one, this could read: "When the spacings between pages all scale by one ratio, the settle keeps its page and its fraction of a page."
User impact
A pager resized mid-settle jumps or rests off its page. Narrowed 100 px, a three-per-view Carousel row ran back to the view it left. Widened, a row of row-wide items turned back too. A partial last page rested 60 px short; others jumped 60–190 px in one frame.
Problem and solution fit
The settle now retargets with its velocity, on the stops the row draws.
Expected behavior and authority
spec:AST-094FR6: "A new target MUST keep the current velocity". Amended here: "A resize during a settle retargets it. When the page offsets all scale by one ratio, the settle keeps its page and its fraction of a page. Otherwise a settle toward the last page ends on the last page. Any other ends on the page nearest its target's offset, a tie going the way it moves", with a resize row.component:CarouselFR7: "the last page rests where the row ends".UIKit decays on toward the old target (iOS 26.2 simulator); FR6 and FR7 require the new pages.
Smallest restoration
measure()maps a settle's target and the page last at rest by offset. The last page stays last; a tie goes the way the settle moves.translateoff its rect, read right after it.Evidence
Scope
Testing
tsc0; strict ESLint 0 and Prettier clean on the changed files; comment sentence lengths 0 over on the changed lines;check:changesetspasses;check:knowledgereports the records aligned.usePager.test.tspasses 155 of 155 andSnapPaging.test.tsx20 of 20. On main 15 of the 17 newusePagercases fail; the other two guard this change's earlier heads. The unit suites of Carousel, SnapPaging, Lightbox, hooks, gestures and motion: pending on this head.flex: 0 0 100%) widens by half about 100 ms into a flick's settle. It must end on the item it was heading to. Without the stop fix (78edbda's build) it rests on the item it left, 450 px, in 2 of 2 Chromium runs at 1 worker; with it, 2 of 2 end on 900 px. Full runs pending.itemsSpanlands on the layout's span to 6 decimals; the rects alone land 1.8 px short. Under a scaled row it takes the scale off each rect before the translate. Through the component, a held row narrowed by 30 px reports a travel of 441, the layout's; without the fix it reports 439.usePager.tsand its tests are this head's, and their browser runs are quick feedback; 5 browser runs per engine where named:LightboxDismiss's busy-frame dismissal check failed, as on main'susePager.LightboxDismiss's reopening case missed its 5 s close once, a timeout logged for its own fix; it then passed 10 of 10 here and on main'susePager.Follow-ups
indexwhose page a resize renumbersWriting rubric results
R1 narration: none; the FR6 sentence and row state the system in the present tense.
R2 research: none in the record.
R3 evidence: the verification row names the tests and their cases, not a change.
R4 rejected alternatives: none added.
R5 change references: none.
R6 open questions: none added.