Repository navigation
fix(AlertDialog): a released action fades on the theme's tempo, as other presses do - #8638
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested. The tracker's move to paintPress is the right direction, but as opened this pull request also carries all of #8346 and #8076 (about 17,000 changed lines against its own 91), and dispose can now end a fade the tracker did not start. Please land the dependencies or retarget onto them, rebase so the diff shows only the three-file tracker change, and make dispose finish only the tracker's own fades:
actionGroupTracker.ts:430-436never forgets an action when its fade ends, sodisposealso removes a fade that another painter starts on that action later, which it did not do before this commit.
[Automated review]
| dispose() { | ||
| for (const element of fading) { | ||
| // Only a fade still running: a later paint is its painter's. | ||
| paintPress(element, 'none', {onlyFrom: ['fading']}); | ||
| } | ||
| fading.clear(); | ||
| reset(); |
There was a problem hiding this comment.
fading is never pruned when the writer's timer ends a fade, so this loop also ends a fading written later by another painter on an action the tracker faded earlier. Fade, wait 200 ms, paintPress(button, 'fading'), then dispose(): the attribute is gone. The same case passes against the previous tracker, whose timers deleted their own entries. Could dispose end only fades the tracker started, for example through a handle paintPress returns for the fade it starts, or by dropping the action once the writer ends that fade? It would be worth adding that case.
| // The document's press controller faded this button a moment ago (its | ||
| // fade's clock runs in the one writer). |
There was a problem hiding this comment.
This comment names the document's press controller, but the case fades with paintPress. The controller fades through writePress on its own timer in pressFeedback.ts, so this case covers another paintPress painter rather than the controller. Worth rewording, or adding a controller case if that interplay is meant to be covered.
meta-cla
Bot
added
the
CLA Signed
There was a problem hiding this comment.
Changes requested. Routing the tracker through paintPress is the right direction, but against main this pull request still carries all of #8076 (about 15,600 changed lines against its own 91), and dispose can end a fade the tracker did not start. Please land #8076 and merge main so the diff shows only the three-file tracker change, and make dispose finish only the tracker's own running fades, with a test for that case.
actionGroupTracker.ts:430-437never forgets an action once the writer ends its fade, sodisposealso removes a fade another painter starts on that action later.
[Automated review]
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC |
|---|---|---|
| SegmentedControl | 82.6 kB (+116 B) · 7.8 kB CSS (0) | — |
| AlertDialog | 84.6 kB (+10 B) · 13.1 kB CSS (0) | — |
| Carousel | 51.3 kB (+4 B) · 10.6 kB CSS (0) | — |
102 more components
| Component | Size | LoC |
|---|---|---|
| lab/CommentThread | 254.3 kB (+2 B) · 2.4 kB CSS (0) | — |
| Button | 45.8 kB (+1 B) · 9.8 kB CSS (0) | — |
| DropdownMenuItem | 17.5 kB (+1 B) · 5.5 kB CSS (0) | — |
| LayerProvider | 63.7 kB (+1 B) · 10.5 kB CSS (0) | — |
| AppShell | 70.9 kB (0) · 12.2 kB CSS (0) | — |
| AvatarGroupOverflow | 39.3 kB (0) · 6.6 kB CSS (0) | — |
| Banner | 47.8 kB (0) · 10.3 kB CSS (0) | — |
| BaseTypeahead | 55.8 kB (0) · 10.3 kB CSS (0) | — |
| BreadcrumbItem | 68.4 kB (0) · 11.5 kB CSS (0) | — |
| Calendar | 54.8 kB (0) · 10.4 kB CSS (0) | — |
| ChatComposer | 61.9 kB (0) · 11.2 kB CSS (0) | — |
| ChatComposerInput | 59.9 kB (0) · 10.7 kB CSS (0) | — |
| ChatComposerTokenElement | 59.9 kB (0) · 10.7 kB CSS (0) | — |
| ChatDictationButton | 46.4 kB (0) · 9.9 kB CSS (0) | — |
| ChatLayout | 49.0 kB (0) · 10.3 kB CSS (0) | — |
| ChatLayoutScrollButton | 46.4 kB (0) · 10.0 kB CSS (0) | — |
| ChatSendButton | 46.3 kB (0) · 9.8 kB CSS (0) | — |
| CheckboxInput | 49.2 kB (0) · 9.8 kB CSS (0) | — |
| CheckboxListItem | 53.9 kB (0) · 11.1 kB CSS (0) | — |
| ClickableCard | 14.6 kB (0) · 7.1 kB CSS (0) | — |
| CodeBlock | 59.8 kB (0) · 11.1 kB CSS (0) | — |
| Collapsible | 22.7 kB (0) · 4.5 kB CSS (0) | — |
| CommandPalette | 50.8 kB (0) · 12.3 kB CSS (0) | — |
| CommandPaletteItem | 10.1 kB (0) · 3.6 kB CSS (0) | — |
| ComplexSelector | 56.3 kB (0) · 11.1 kB CSS (0) | — |
| ContextMenu | 75.9 kB (0) · 15.4 kB CSS (0) | — |
| ContextMenuItem | 26.2 kB (0) · 5.0 kB CSS (0) | — |
| DateInput | 147.9 kB (0) · 15.4 kB CSS (0) | — |
| DateRangeInput | 72.2 kB (0) · 11.9 kB CSS (0) | — |
| DialogHeader | 48.9 kB (0) · 10.2 kB CSS (0) | — |
| DrawerHeader | 49.0 kB (0) · 11.6 kB CSS (0) | — |
| DropdownMenu | 74.1 kB (0) · 15.5 kB CSS (0) | — |
| DropdownMenuCheckboxItem | 18.3 kB (0) · 5.9 kB CSS (0) | — |
| DropdownMenuRadioItem | 18.3 kB (0) · 5.9 kB CSS (0) | — |
| DropdownMenuSubMenu | 56.5 kB (0) · 10.0 kB CSS (0) | — |
| FileInput | 54.3 kB (0) · 10.8 kB CSS (0) | — |
| IconButton | 45.9 kB (0) · 9.8 kB CSS (0) | — |
| Item | 13.3 kB (0) · 4.9 kB CSS (0) | — |
| Lightbox | 52.8 kB (0) · 10.2 kB CSS (0) | — |
| Link | 43.3 kB (0) · 8.0 kB CSS (0) | — |
| ListItem | 53.2 kB (0) · 11.1 kB CSS (0) | — |
| Markdown | 146.3 kB (0) · 18.4 kB CSS (0) | — |
| MobileNav | 52.5 kB (0) · 10.4 kB CSS (0) | — |
| MobileNavToggle | 46.2 kB (0) · 9.8 kB CSS (0) | — |
| MoreMenu | 74.4 kB (0) · 15.5 kB CSS (0) | — |
| MultiSelector | 73.7 kB (0) · 15.3 kB CSS (0) | — |
| NumberInput | 55.7 kB (0) · 11.3 kB CSS (0) | — |
| Outline | 21.9 kB (0) · 4.2 kB CSS (0) | — |
| Pagination | 87.3 kB (0) · 16.1 kB CSS (0) | — |
| Popover | 53.3 kB (0) · 12.7 kB CSS (0) | — |
| PowerSearch | 236.1 kB (0) · 21.7 kB CSS (0) | — |
| RadioListItem | 51.7 kB (0) · 10.1 kB CSS (0) | — |
| SegmentedControlItem | 10.4 kB (0) · 4.0 kB CSS (0) | — |
| SelectableCard | 14.8 kB (0) · 7.3 kB CSS (0) | — |
| Selector | 78.4 kB (0) · 15.7 kB CSS (0) | — |
| SelectorOption | 16.6 kB (0) · 5.4 kB CSS (0) | — |
| SideNavCollapseButton | 46.7 kB (0) · 9.9 kB CSS (0) | — |
| SideNavHeading | 59.7 kB (0) · 10.6 kB CSS (0) | — |
| SideNavItem | 55.3 kB (0) · 10.5 kB CSS (0) | — |
| Step | 27.9 kB (0) · 5.6 kB CSS (0) | — |
| Stepper | 47.6 kB (0) · 10.0 kB CSS (0) | — |
| Switch | 87.7 kB (0) · 10.6 kB CSS (0) | — |
| Tab | 11.4 kB (0) · 4.2 kB CSS (0) | — |
| Table | 85.3 kB (0) · 16.1 kB CSS (0) | — |
| TableCell | 77.1 kB (0) · 15.9 kB CSS (0) | — |
| TableHeaderCell | 76.7 kB (0) · 15.6 kB CSS (0) | — |
| TableSelectionToolbar | 55.7 kB (0) · 11.8 kB CSS (0) | — |
| TabMenu | 56.3 kB (0) · 10.5 kB CSS (0) | — |
| TextInput | 53.5 kB (0) · 11.0 kB CSS (0) | — |
| Thumbnail | 50.1 kB (0) · 11.4 kB CSS (0) | — |
| TimeInput | 132.7 kB (0) · 14.4 kB CSS (0) | — |
| Timestamp | 21.3 kB (0) · 10.0 kB CSS (0) | — |
| Toast | 59.7 kB (0) · 10.1 kB CSS (0) | — |
| ToggleButton | 46.7 kB (0) · 9.9 kB CSS (0) | — |
| Token | 25.3 kB (0) · 4.8 kB CSS (0) | — |
| Tokenizer | 71.4 kB (0) · 17.1 kB CSS (0) | — |
| TopNav | 54.1 kB (0) · 10.7 kB CSS (0) | — |
| TopNavHeading | 59.1 kB (0) · 10.4 kB CSS (0) | — |
| TopNavItem | 10.7 kB (0) · 4.0 kB CSS (0) | — |
| TopNavMegaMenu | 54.8 kB (0) · 11.2 kB CSS (0) | — |
| TopNavMegaMenuItem | 10.8 kB (0) · 4.2 kB CSS (0) | — |
| TopNavMenu | 58.9 kB (0) · 10.5 kB CSS (0) | — |
| TreeList | 38.2 kB (0) · 5.0 kB CSS (0) | — |
| Typeahead | 63.6 kB (0) · 11.8 kB CSS (0) | — |
| lab/ChatEmojiPicker | 66.9 kB (0) · 773 B CSS (0) | — |
| lab/ChatReactionBar | 67.5 kB (0) · 1.1 kB CSS (0) | — |
| lab/CommentComposer | 105.5 kB (0) · 1.9 kB CSS (0) | — |
| lab/CommentPin | 47.5 kB (0) · 1.2 kB CSS (0) | — |
| lab/DiffViewer | 71.8 kB (0) · 2.1 kB CSS (0) | — |
| lab/EmojiPicker | 71.2 kB (0) · 722 B CSS (0) | — |
| lab/ListInput | 83.7 kB (0) · 1.6 kB CSS (0) | — |
| lab/Schedule | 62.0 kB (0) · 3.9 kB CSS (0) | — |
| lab/TourStep | 58.6 kB (0) · 590 B CSS (0) | — |
| lab/TransferList | 76.7 kB (0) · 1.7 kB CSS (0) | — |
| lab/TransferListSelector | 83.2 kB (0) · 1.9 kB CSS (0) | — |
| richtext/RichTextEditor | 278.7 kB (0) · 2.6 kB CSS (0) | — |
| richtext/RichTextEditorToolbar | 225.6 kB (0) · 454 B CSS (0) | — |
| richtext/RichTextView | 259.2 kB (0) · 2.3 kB CSS (0) | — |
| lab/Comments | 259.4 kB (−1 B) · 2.5 kB CSS (0) | — |
| lab/SelectableCardGroup | 50.9 kB (−1 B) · 162 B CSS (0) | — |
| SideNav | 70.2 kB (−2 B) · 11.2 kB CSS (0) | — |
| DateTimeInput | 168.6 kB (−24 B) · 17.1 kB CSS (0) | — |
Package size: @astryxdesign/core 6.30 MB packed (+212 B); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: core 1 row; run pnpm api:report.
Not run on this PR: add ci:run-visual, ci:run-a11y or ci:run-rtl to run them.
Generated by PR Enrichment workflow | View full report
There was a problem hiding this comment.
Changes requested. Routing the tracker through paintPress is the right direction, but against main this pull request still carries all of #8076 (about 15,800 changed lines against its own 91), and dispose can end a fade the tracker did not start. Please land #8076 and merge main so the diff shows only the three-file tracker change, and make dispose finish only the tracker's own running fades, with a test for that sequence.
actionGroupTracker.ts:430-437never forgets an action once the writer ends its fade, sodisposealso removes a fade another painter starts on that action later, which the tracker did not do before this commit.
[Automated review]
30e0d10 to
6471d4c
Compare
There was a problem hiding this comment.
Changes requested. Routing the tracker through one press writer is a reasonable direction, but this branch was built on copies of #8076 and #8360 that have since landed on main in a different form, so it cannot merge as it stands.
- The branch conflicts with
mainin 87 files, includingactionGroupTracker.ts,actionGroupTracker.test.tsandpressFeedback.ts. paintPress, which the tracker now imports (actionGroupTracker.ts:65), does not exist onmain, because #8076 and #8360 landed without it,writePressor theheldpaint.- Resolving the merge toward this branch would re-add 27 changesets already released in v0.7.5 and v0.7.6 and modules
maindropped or moved, such asgestures/useAttachment.ts. - The changeset's "No component uses the tracker yet" no longer holds on
main, where AlertDialog drives the tracker with its paint on. - Against
mainthe diff is 27,382 authored lines, of which the tracker change is 164.
Next: merge main keeping main's version of every carried file, rebuild the tracker change on main's press controller, and update the changeset and description. A few new comments could also be shorter; see the inline notes.
[Automated review]
|
|
||
| import {PRESSED_ATTRIBUTE} from '../utils/pressFeedback'; | ||
| import {PRESS_FADE_MS} from '../utils/pressGesture'; | ||
| import {paintPress} from '../utils/pressFeedback'; |
There was a problem hiding this comment.
main has no paintPress to import: #8076 and #8360 landed without it, writePress or the held paint, and main's press controller writes the attribute through a private paintElement. After merging main, this change needs rebuilding on that controller, or the one-writer design has to come to main with every painter it covers.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [chore] The internal action-group tracker paints its highlight through the one press writer in `utils/pressFeedback.ts`, as the document's press controller does, so neither cuts the other's fade short, a `held` paint is left alone, and its unmount ends only the fades it started that are still running. No component uses the tracker yet, and no package entry point exports it. |
There was a problem hiding this comment.
On main, AlertDialog drives the tracker with its paint on (hooks/useActionGroupTracking.ts), so "No component uses the tracker yet" would be false in the release that ships this. Once the change is rebuilt on main, could this say what changes for AlertDialog's actions?
| * The highlight is painted through `paintPress`, the one writer of the press | ||
| * attribute (utils/pressFeedback.ts), as the document's press controller | ||
| * paints: a write ends any fade another painter left running on that action, | ||
| * so neither cuts the other's fade short, and a paint the tracker did not | ||
| * write (a `held` row) is left alone. |
There was a problem hiding this comment.
Non-blocking: this is one sentence of about 50 words with several topics. A shorter version: "The tracker paints through paintPress, the press attribute's one writer. A write ends a fade that another painter left running. A held paint stays with its owner."
| element.removeAttribute(PRESSED_ATTRIBUTE); | ||
| } | ||
| for (const element of fading) { | ||
| // Each is the tracker's own fade, still running. |
There was a problem hiding this comment.
Non-blocking: this repeats what the comment on the fading set (line 242) already says, so it can go.
| const PAINTS: ReadonlySet<string> = new Set(['on', 'held', 'fading']); | ||
| /** A `fading` paint's own release clock, per element (paintPress). */ | ||
| const fadeTimers = new WeakMap<Element, ReturnType<typeof setTimeout>>(); | ||
| /** A `fading` paint's end callback, per element (paintPress's onFadeEnd). */ |
There was a problem hiding this comment.
Non-blocking: this restates the name and type of fadeEnds, so it can go.
6471d4c to
abde23b
Compare
vjeux
changed the title
github-actions
Bot
removed
the
needs:design-review
meta#8638's own change. The tracker paints its groups' presses through paintPress, the one writer, which now takes an `onFadeEnd` for a `fading` paint: the writer calls it once, when that fade stops being the element's paint, on its release's end (meta#8890's endAfterRelease) or when a later write replaces it. The tracker forgets an action there, so `dispose` ends only its own running fades. Absorbs meta#8638.
meta#8638's own change. The tracker paints its groups' presses through paintPress, the one writer, which now takes an `onFadeEnd` for a `fading` paint: the writer calls it once, when that fade stops being the element's paint, on its release's end (meta#8890's endAfterRelease) or when a later write replaces it. The tracker forgets an action there, so `dispose` ends only its own running fades. Absorbs meta#8638.
meta#8638's own change. The tracker paints its groups' presses through paintPress, the one writer, which now takes an `onFadeEnd` for a `fading` paint: the writer calls it once, when that fade stops being the element's paint, on its release's end (meta#8890's endAfterRelease) or when a later write replaces it. The tracker forgets an action there, so `dispose` ends only its own running fades. Absorbs meta#8638.
meta#8638's own change. The tracker paints its groups' presses through paintPress, the one writer, which now takes an `onFadeEnd` for a `fading` paint: the writer calls it once, when that fade stops being the element's paint, on its release's end (meta#8890's endAfterRelease) or when a later write replaces it. The tracker forgets an action there, so `dispose` ends only its own running fades. Absorbs meta#8638. The gestures README says so: the tracker paints its groups' presses with paintPress.
There was a problem hiding this comment.
Changes requested: the timer change is correct and brings the tracker's release fade onto the theme's tempo, but the release note claims more than the change does.
- The changeset (and the User impact section) says every action a finger or pen lifts on now fades for the theme's full release time, but a lift that closes the alert (Cancel, or an action that closes it at once) still ends its fade at the close and a loading action paints no overlay, so please reword it to cover an action lifted while the alert stays open and enabled.
Optional: the new sentence in pressFadeMs's doc comment repeats the reason the tracker's own comment gives, and the pressFeedback.ts header's @output list does not name pressFadeMs yet.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] AlertDialog: under the touch presentation, the action a finger or pen lifts on now fades for the theme's full release time, as every other press does, instead of cutting off at 200 ms under a slower motion tempo. |
There was a problem hiding this comment.
This promises the longer fade for every lifted action, but a lift that closes the alert (Cancel, or an action that closes it at once) still ends its fade at the close, and a loading action paints no overlay. Suggested wording:
[fix] AlertDialog: under the touch presentation, an action lifted while the alert stays open now ends its release fade on the theme's tempo-scaled clock, as the stylesheet's fade does, instead of at a fixed 200 ms.
| * The release fade's clock on `element`, read once, at the release, from the | ||
| * property its stylesheet reads: the theme's `--_astryx-motion-tempo`. Under | ||
| * Reduce Motion a tempo may shorten the fade, never lengthen it past 200 ms. | ||
| * A group's tracker times its own release fades with it too. |
There was a problem hiding this comment.
Non-blocking: this sentence names a caller, which the import already shows, and the tracker's own comment gives the reason. Consider dropping it, and adding pressFadeMs to the header's @output list instead.
…her presses do Under the touch presentation, AlertDialog's action-group tracker took the press off an action lifted while the alert stays open on a fixed 200 ms timer. The stylesheet's release runs 200 ms times the theme's tempo (up to 1.5), and the press controller's own timer follows it, so under a slower tempo the fade snapped off before it ended. The tracker now times its release fades with the press controller's clock (pressFadeMs, exported from pressFeedback.ts), which also keeps the Reduce Motion cap.
abde23b to
ea5342d
Compare
There was a problem hiding this comment.
Recommending approval: the action-group tracker now ends a lifted action's release fade on the press controller's tempo-scaled clock, so under a non-default theme tempo AlertDialog's action keeps its press paint for the whole release, as component:Button FR15 and spec:AST-108 FR1 and FR4 require.
The new tempo test fails against the previous tracker and passes here, the Reduce Motion cap is covered, and the reworded changeset now matches what changes. CI's test jobs were skipped while the pull request is a draft, so they still need to run once it is marked ready.
[Automated review]
cixzhang
added
the
ci:run
Main's #8638 renamed the release clock helper fadeMs to an exported pressFadeMs. The release call here now uses pressFadeMs; nothing else in this pull request changes.
…9758) * fix(core): a touch press's release lasts until its fade has played Under a finger, the touch press controller ended a press's release on a clock that starts at the lift. When script delays the first frame of the release animation, the animation ends later, but the clock does not, so the highlight was cut off mid-fade. When the clock runs out, the controller now waits for a release animation that still runs or still waits for its first frame: on the surface, its ::before or ::after, or on a part inside it whose nearest press owner is the surface. With none, the release ends at the clock, as before. Rebased onto main after #8360 landed: main's theme tempo already caps the release under Reduce Motion, so only the release-ends-with-its-animation part remains. Co-authored-by: Cindy Zhang <cindyxz@meta.com> * docs(core): the press clock's comments say the release waits for its animation CodeBunny's notes on the first round: pressGesture.ts still described PRESS_FADE_MS as the timer that removes the attribute, and two new comments in pressFeedback.ts had sentences over 25 words. * Merge main into the press release change Main's #8638 renamed the release clock helper fadeMs to an exported pressFadeMs. The release call here now uses pressFadeMs; nothing else in this pull request changes. Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com>
User impact
Under AlertDialog's touch presentation, an action a finger or pen lifts on while the alert stays open fades its press out, but that fade ended on a fixed 200 ms timer, so under a slower motion tempo it snapped off early. A lift that closes the alert, or a loading action, is unchanged. Touch presentation only: a mouse, a keyboard and the desktop presentation are unchanged.
Before, at tempo 1.5: the release runs 300 ms, but the press came off at 200 ms. After: it stays until 300 ms. At the default tempo nothing changes.
Problem and solution fit
PRESS_FADE_MS), while the stylesheet scales the release by the theme's--_astryx-motion-tempo(up to 1.5). The press controller's timer already reads that tempo; the tracker's never did.spec:AST-108FR4), and this one press stayed on the old clock.The tracker now times its release fades with the press controller's own clock, so the two can no longer drift apart.
Expected behavior and authority
spec:AST-108FR4: the press release follows the theme's tempo, and under Reduce Motion a tempo may shorten it but never lengthens it past 200 ms. The press controller already meets it; this brings the tracker in line.Smallest restoration
pressFeedback.tsexports its existing fade clock aspressFadeMs, unchanged. The tracker's release timer calls it instead of the constant. Nothing else changes: the tracker still paints and cuts as before, and no new API ships.This replaces an earlier version of this pull request that rerouted every press paint through one writer;
mainhas since taken the press controller in another form, so the change is rebuilt on it.Evidence
--_astryx-motion-tempo: 1.5loses itsfadingpaint at 200 ms.Scope
Testing
Local gates at ea5342d, which differs from abde23b only in the changeset and two comment lines: the tracker and press controller tests (99 of 99), the core type check, strict lint,
pnpm lintandreview:pre. At abde23b: the hooks, utils, AlertDialog, Button and Tokenizer units (2,089 of 2,089), including the two new tracker cases; the core type check, strict lint,pnpm lint, the knowledge check andreview:pre(no findings); and AlertDialog's Chromium interaction spec on a fresh Storybook build (11 of 11). With the tracker and press controller as on the base under these tests (NOT-FOR-LANDING arm 22f83308af), the tempo case fails: the paint is gone at 200 ms. The Reduce Motion case passes on both.