Repository navigation
fix(gestures): a selection a claimed lift's own handlers make stays - #9848
Conversation
When a recognizer claims a mouse or pen press, its drag's selection hold puts the press's selection back after the lift. A selection that the lift's own handlers made, such as the click an action runs, was put back over too. Now the hold keeps a selection the lift's handlers change, or a render they queue changes. When they change nothing, the press's selection comes back as before.
|
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.
|
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| SegmentedControl | 81.7 kB (+114 B) · 6.7 kB CSS (0) | — | — · — · — |
| PowerSearch | 236.1 kB (+110 B) · 21.8 kB CSS (0) | — | — · — · — |
| Slider | 88.6 kB (+102 B) · 8.3 kB CSS (0) | — | — · — · — |
7 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| lab/Comments | 258.3 kB (+100 B) · 2.5 kB CSS (0) | — | — · — · — |
| AlertDialog · Storybook | 83.1 kB (+99 B) · 13.1 kB CSS (0) | 215 (0) | ✓ · ✓ · ✓ |
| DateTimeInput | 167.8 kB (+99 B) · 17.2 kB CSS (0) | — | — · — · — |
| lab/CommentThread | 253.0 kB (+99 B) · 2.4 kB CSS (0) | — | — · — · — |
| Switch | 86.8 kB (+90 B) · 9.5 kB CSS (0) | — | — · — · — |
| TimeInput | 131.5 kB (+90 B) · 14.5 kB CSS (0) | — | — · — · — |
| DateInput | 147.1 kB (+83 B) · 15.5 kB CSS (0) | — | — · — · — |
Package size: @astryxdesign/core 6.30 MB packed (+705 B); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: theme-butter 1 row, theme-chocolate 1 row, theme-gothic 1 row, theme-matcha 1 row, theme-neutral 1 row, theme-stone 1 row, theme-y2k 1 row; run pnpm api:report.
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | View full report
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Recommendation: approve. No blocking findings: keeping a text selection that a claimed mouse or pen lift's own handlers make matches spec:AST-094 FR41, and the new AlertDialog pen test fails before the change and passes after it.
- Optional: in
packages/core/src/utils/selectionLock.ts, renamingatLifttoselectionAtReleasewould let you drop the comment above it.
CI for this head stopped at the step that waits for a maintainer's ci:run label, before any lint or test ran, so it still needs to run before this lands.
[Automated review]
| // The selection when the lift released the hold, until the lift's | ||
| // handlers ran. | ||
| let atLift: ReadonlyArray<AbstractRange> | null = null; |
There was a problem hiding this comment.
Optional naming suggestion: selectionAtRelease says what this holds, and atLift = null in keepLiftSelection already shows when it ends, so the comment could go.
cixzhang
added
the
ci:run
There was a problem hiding this comment.
Recommendation: request changes. The code change is correct: keeping a selection that a claimed mouse or pen lift's own handlers make matches spec:AST-094 FR41. The new AlertDialog pen test fails before the change and passes after it, and Chromium shows the same for both a mouse and a pen. Only the description needs a fix.
- Miss: the User impact example "for example a field's contents for copying" describes a bug that does not exist, because a lift handler that selects a text field's contents already keeps that selection before this change (the hold skips restoring while a text field has focus,
packages/core/src/utils/selectionLock.tslines 162-165), so please reword it to the page-text case your tests reproduce.
This pull request was reviewed on its own, not as a stack: #8657 is built on an earlier commit of this branch, not on its current head, so the declared stack does not hold until #8657 is rebased onto it.
[Automated review]
There was a problem hiding this comment.
Recommendation: approve. Keeping a text selection that a claimed mouse or pen lift's own handlers make restores spec:AST-094 FR41 ("never clears an existing selection"). The new AlertDialog pen test and the three selection-hold cases fail before the change and pass after it, and CI passed on this head. The description's field example that held up the last review is gone, and the new wording matches what the tests reproduce.
- Optional: renaming
atLifttoselectionAtReleaseinpackages/core/src/utils/selectionLock.tswould let you drop the comment above it.
[Automated review]
Stack: #9848 → #8657 → #8660
User impact
Part of the UIKit-parity work. With a pen on a touch screen, an AlertDialog action runs when the pen lifts. If that action selects text on the page, the selection vanishes right after the click: the drag's selection hold puts the press's selection back over it. A text field already keeps its own selection, since the hold leaves a focused field alone. The same happens for every recognizer that claims a mouse or pen press and runs a handler at its lift.
Problem and solution fit
spec:AST-094FR41). At the lift the hold puts back the selection the press began with.The fix: after the lift's handlers run, the hold keeps a selection they changed, or one a render they queue changes in the next microtask. When they change nothing, the press's selection comes back, as before.
Expected behavior and authority
spec:AST-094FR41: "A mouse or pen drag that a recognizer claims takes no new text selection for that drag and never clears an existing selection." The selection an action's own click makes belongs to the click, as it does for a native button.Smallest restoration
utils/selectionLock.ts:lockDragSelectionreturns a hold whosekeepLiftSelection()keeps a selection changed since the release.gestures/GestureHost.ts: at a claimed contact's lift, the host calls it after the press ends, which runs the lift's handlers.Evidence
AlertDialog.test.tsx, "keeps the selection that the action's own click makes when a pen press lifts on it" fails on main. The selection reads "Text the page selected", not "Text the action selects".selectionLock.test.tscases: a selection made after the release stays; one made by a render queued before the keep stays; the press's selection comes back when nothing changed.GestureHost.tsandselectionLock.tsat main.Scope
Testing
Local gates at this head (on main bb3838b), after a fresh
pnpm install --frozen-lockfile:GestureHost.tsandselectionLock.tsat main, the four new cases fail. The AlertDialog case keeps "Text the page selected", and the three unit cases have nokeepLiftSelection.test:uipasses (19,445 tests).gestures.interaction(66 passed in Chromium, 58 in WebKit; the rest are that file's engine gates).review:preandcheck:knowledgepass.