Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
The compositor release works as described in Chromium and keeps its clock and curve. Approval waits on one spec decision and three fixes.
Spec decision needed: no current record says Button may take its own ::before during a touch press, and that changes what a caller's ::before styles render. An engineering owner needs to provide or identify the governing clause (for example in component:Button) and records the decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner.
Required fixes:
- Nest
willChangeunder@media (pointer: coarse)incontrolPressLayerStyles.control(privateAttributes/press.stylex.ts:302), because a pen or finger press on a fine-pointer device now getswill-change: transform, againstspec:AST-017FR59 and the changeset's "nothing changes". - Drop
expect(state.press).toBeNull()from the fine-pointer mouse test (ButtonPressRelease.interaction.browser.spec.ts:376), becausecomponent:ButtonFR15 routes a mouse press through the press controller and the assertion pins that recorded gap as a rule. - Build Button's layer from one shared press-layer style with the rows instead of copying
rowPressLayerStyles.cellandcellOnImage, keeping only Button's own coarse-guardedwillChange(and later its clock) incontrol.
[Automated review]
| // for it and drops that layer at its end, which repaints the button. A | ||
| // layer kept from the touch to the end of the release keeps the release | ||
| // free of Paint. | ||
| willChange: {default: null, [PRESSED_ANY]: 'transform'}, |
There was a problem hiding this comment.
This is the only control arm outside @media (pointer: coarse). The controller writes the press state for every non-mouse pointer, so on a fine-pointer device a pen or finger press now gets will-change: transform (a stacking context and a compositing layer for the press). That goes against spec:AST-017 FR59 and the changeset's "nothing changes".
willChange: {default: null, [COARSE]: {default: null, [PRESSED_ANY]: 'transform'}},keepsPressOutsideCoarse could also reject will-change outside the coarse query, so the guard catches this.
| expect(['none', 'normal'], JSON.stringify(state)).toContain(state.layer); | ||
| expect(state.isolation).toBe('auto'); | ||
| // A mouse's press is `:active`'s: the controller writes nothing. | ||
| expect(state.press).toBeNull(); |
There was a problem hiding this comment.
component:Button FR15 says a press paints "through the press controller: at once under a mouse or a pen", and Button's Current-state impact lists the mouse path as not yet implemented. This assertion turns that gap into "the desktop rule", so whoever implements the mouse path gets a failing test. The layer and isolation checks above already prove this test's point, so this line can go.
| * rest, and with any other pointer, nothing here applies: no layer, no | ||
| * `isolation`, and the variant's own paint and release, as before. | ||
| */ | ||
| control: { |
There was a problem hiding this comment.
control repeats rowPressLayerStyles.cell and cellOnImage property for property: the image arms, the coarse animation-name: none, isolation, the ::before box, opacity, clock and fill mode. controlRelease duplicates cellRelease. Only the ENABLED nesting and willChange are new. StyleX merges ::before properties one at a time (as cellWithoutLayer relies on), so Button could compose one shared press-layer style with the rows and keep only its own coarse-guarded willChange, and later its clock, here.
Under a coarse primary pointer, Button (and IconButton and ToggleButton) paints its touch press on its own ::before layer, present only while a press is on or fading, with isolation only then. ClickableCard, SelectableCard and Thumbnail fade their ::after. Each release is an opacity animation, which engines run on the compositor, so it keeps fading while the page's script runs. Any other pointer is unchanged. Rebased onto main and #9765 (which supersedes #8117), on Specs' direction: the layers play the strength's own press clock and curve (main's tempo clock) for now, and a pen follows the touch presentation. Button FR15's release length waits on an owner decision. So this drops #8118's UIButton table, its mouse and key input marker, its #8350 CLI files and #8338's reduced-motion arms. The press layer styles live in the press attribute's owner module. Co-authored-by: Cindy Zhang <cindyxz@meta.com>
0f28e7c to
6e6dbb6
Compare
There was a problem hiding this comment.
The compositor release works as described in Chromium and keeps its clock and curve. Approval waits on one spec decision and three fixes.
Spec decision needed: no current record says Button may take its own ::before during a touch press, and that changes what a caller's ::before styles render during a press (the press fill can disappear and the caller's decoration moves). An engineering owner needs to settle whether Button's component record claims that pseudo-element (or whether the press must leave a caller's ::before intact), and records the decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner.
Required fixes:
- Nest
willChangeunder@media (pointer: coarse)incontrolPressLayerStyles.control(privateAttributes/press.stylex.ts:306), because a pen or finger press on a fine-pointer device now getswill-change: transform, againstspec:AST-017FR59 and the changeset's "nothing changes". - Drop
expect(state.press).toBeNull()from the fine-pointer mouse test (ButtonPressRelease.interaction.browser.spec.ts:376), becausespec:AST-094FR11 and DEC-21 route a mouse press through the press controller and the assertion pins Button's recorded gap as a rule. - Build Button's layer from the shared row press-layer styles (
rowPressLayerStyles.cellandcellOnImage) instead of copying them, keeping only Button's own coarse-guardedwillChange(and later its clock) incontrol.
[Automated review]
| // for it and drops that layer at its end, which repaints the button. A | ||
| // layer kept from the touch to the end of the release keeps the release | ||
| // free of Paint. | ||
| willChange: {default: null, [PRESSED_ANY]: 'transform'}, |
There was a problem hiding this comment.
This is the only control arm outside @media (pointer: coarse). The controller writes the press state for every non-mouse pointer, so on a fine-pointer device a pen or finger press now gets will-change: transform (a stacking context and a compositing layer), against spec:AST-017 FR59 and the changeset's "nothing changes". Suggest willChange: {default: null, [COARSE]: {default: null, [PRESSED_ANY]: 'transform'}}, and have keepsPressOutsideCoarse reject will-change outside the coarse query.
| expect(['none', 'normal'], JSON.stringify(state)).toContain(state.layer); | ||
| expect(state.isolation).toBe('auto'); | ||
| // A mouse's press is `:active`'s: the controller writes nothing. | ||
| expect(state.press).toBeNull(); |
There was a problem hiding this comment.
spec:AST-094 FR11 says a fine pointer's press paint starts "through the same press controller" and DEC-21 says "The press controller tracks every pointer"; Button's record lists the mouse path as not yet implemented. This assertion pins that gap as a rule, so closing it fails this test. Suggest dropping it and keeping the layer and isolation checks.
| * rest, and with any other pointer, nothing here applies: no layer, no | ||
| * `isolation`, and the variant's own paint and release, as before. | ||
| */ | ||
| control: { |
There was a problem hiding this comment.
control repeats rowPressLayerStyles.cell and cellOnImage property for property (image arms, coarse animation-name: none, isolation, the ::before box, opacity, clock and fill mode), and controlRelease duplicates cellRelease. Composing Button from those styles, with only Button's coarse-guarded willChange (and later a clock override) here, keeps one path for both.
Supersedes #8118 by @vjeux, rebased onto
mainand stacked on #9765 (#8117's replacement). The change is vjeux's: his commit authorship is kept, and the changeset credits him. This pull request targets #9765's branch so only its own change shows.Draft until an owner decision.
component:ButtonFR15's release length waits on Cindy's decision (Specs & Decisions' batch card 47). Until then, on Specs' direction, the compositor layers play the strength's own press clock and curve,main's tempo-scaled 200 ms deselect fade. Card 47's answer is then a change to one constant,CONTROL_RELEASE. Specs will give the FR15 text and stamp after that decision.User impact
This affects people who tap a button or a card on a touch-first device: Button, IconButton, ToggleButton, ClickableCard, SelectableCard and Thumbnail.
isolation, the strength's own release.Problem and solution fit
Under a coarse primary pointer, Button paints the press on its own
::before(controlPressLayerStyles.control). The layer,isolationand awill-change: transformexist only while a press is on or fading. The cards and Thumbnail fade their own::after(controlOver). #9758's controller ends the press when that layer's release animation has played.Changes since #8118
mainand Specs & Decisions' rulings changed what this pull request carries:::beforewhile pressed (callers don't style it). The release followsspec:AST-094's control profile (250 ms) on touch, and a mouse keeps 200 ms") conflicts with her 11/02 rule that non-gesture interactions use the theme's motion tokens on every pointer. Specs carded it (card 47) and asked for a duration-agnostic mechanism meanwhile, with every overlay control kept atmain's clock. So this drops:motion/uikit/button.ts) and its mirror (utils/controlPressLayer.stylex.ts);data-astryx-press-input) that split 200 from 250 ms;interactionOverlay.stylex.ts.motion/uikit/button.ts,astryx component buttonhas nothing new to resolve to, so fix(cli): a component's name in another case never resolves through a module that shares its spelling #8350's six CLI files are not here (fix(cli): a component's name in another case never resolves through a module that shares its spelling #8350 lands on its own).press-paint-reduced-motion.md,pressPaintReducedMotion.test.ts) are dropped. Onmainthe theme tempo already caps the release under Reduce Motion, and the layers use that same capped clock.privateAttributes/press.stylex.tsbeside the row layer (architecture:public-component-apiINV23).Expected behavior and authority
Cindy's 10-25 ruling allows Button's
::beforeon touch.spec:AST-094FR5 requires the compositor release.design:user-statesDR1 and DR5. A fine pointer is unchanged (spec:AST-017FR59).component:ButtonFR15's text waits on card 47.Verification
isolationat rest;::after.keepsPressOutsideCoarse), and an Item row keeps its layer to a press.check:changesetsandcheck:knowledge.review:prereports no findings (1,779 authored lines).ButtonPress,ButtonPressRelease,ButtonPressTiming,ListRowPress,ListRowPressPaths) pass 39, with 1 skipped. On fix(core): under a finger a pressable highlights at touch-down, and waits only inside a scroller #9765's head, 8ButtonPressReleasecases fail:PressModality,PressedState,CardPress): 16 of 17 pass.PressModality's mouse-drag-across-menu-rows case fails intermittently in my rig on fix(core): under a finger a pressable highlights at touch-down, and waits only inside a scroller #9765's head too, and passed 4 of 4 runs earlier.Scope