Repository navigation
test(core): one internal definition pins core's touch style keys - #8331
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
meta-cla
Bot
added
the
CLA Signed
18631e2 to
a01fc4f
Compare
a01fc4f to
f81230b
Compare
4701ef7 to
03d15cf
Compare
vjeux
changed the title
![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 internal touchKeys reference and the press-paint cascade guard fit the current touch specs, but the diff against main now carries all of #8360 (about 27,200 lines) beside this pull request's own 289. Please rebase once #8360 lands so the diff is your six files, and make the drift test fail when any literal '@media (pointer: coarse)' key drifts (today only compact-touch keys and the keys around iOS gates are compared, so a one-character change in Switch/Switch.tsx passes), or narrow the header, the touchPresentation.ts note and the changeset to what the test pins.
[Automated review]
| * These are the spellings to match. A style that changes with these | ||
| * conditions writes the same literal key (`'@media (pointer: coarse)'`, the | ||
| * iOS gate as that media key outside `'@supports (-webkit-touch-callout: | ||
| * none)'`), and a test pins every literal in core to these values. Do not use |
There was a problem hiding this comment.
This says a test pins every literal in core, but only compact-touch keys and the '@media (pointer: coarse)' keys around an iOS gate are compared with the resolver (touchPresentation.test.ts:230–330). A one-character drift in any of the seven plain touch keys in Switch/Switch.tsx, or in COARSE in interactionOverlay.stylex.ts, fails no test. Either add a membership scan, as motion/reducedMotion.test.ts does for the Reduce Motion keys, or narrow this sentence and lines 10–11 to what is pinned.
| ]); | ||
| }); | ||
|
|
||
| it('keeps every touch and Reduce Motion key literal in core: no computed StyleX key', () => { |
There was a problem hiding this comment.
This scan finds computed keys only, and the equality test above compares touchKeys with the constants it repeats. To pin the literals themselves, scan every '@media …' key that names pointer or a compact width and require it to be touchKeys.touch, touchKeys.compact, touchKeys.compactTouch or an explicitly allowed query ((any-pointer: coarse), (pointer: fine), the Reduce Motion keys).
There was a problem hiding this comment.
Changes requested: the internal touchKeys reference, the new membership scan and the press-paint cascade guard fit the current touch specs, but the diff against main carries all of #8360 (about 27,200 lines) beside this pull request's own 346. Please rebase once #8360 lands so the diff is your six files, and widen the membership scan's pointer test in touchPresentation.test.ts (for example to /pointer|coarse|fine/): today a one-character drift inside the feature name, such as '@media (pointr: coarse)' in Switch/Switch.tsx, passes every case, although the header and the changeset say a test pins every literal.
[Automated review]
| for (const match of source.matchAll(/'(@media [^']*)'/g)) { | ||
| const key = match[1]; | ||
| const where = `${path.relative(root, full)}: ${key}`; | ||
| if (key.includes('pointer')) { |
There was a problem hiding this comment.
This classes a key by the substring pointer alone, so a typo inside that word drops the key out of the scan: '@media (pointr: coarse)' at Switch/Switch.tsx:192, or in COARSE at utils/interactionOverlay.stylex.ts:124, passes every case in this file, and StyleX compiles it into a query no browser matches. Testing /pointer|coarse|fine/.test(key) here keeps the head passing (every @media literal in core that mentions coarse or fine already names pointer) and fails that drift.
There was a problem hiding this comment.
Changes requested: the wider pointer test fixes the '@media (pointr: coarse)' blind spot, and the internal touchKeys reference and press-paint guard still fit the current touch specs, but the diff against main carries #8360's stack (about 27,200 lines) beside this pull request's own 347. Please rebase once #8360 lands so the diff is your six files, and either close the two drifts the scans still pass or narrow the header, the touchPresentation.ts note and the changeset to what they catch:
- A drift to
'@meda (pointer: coarse)'inSwitch/Switch.tsxis never scanned, because the membership case reads only literals that begin'@media. - A drift to
'@supports (-webkit-touch-calout: none)'inChat/ChatComposerInput.tsxstops counting as an iOS gate, so no case checks it.
[Automated review]
| !/\.(test|spec|stories)\./.test(entry.name) | ||
| ) { | ||
| const source = fs.readFileSync(full, 'utf8'); | ||
| for (const match of source.matchAll(/'(@media [^']*)'/g)) { |
There was a problem hiding this comment.
Only literals that begin '@media reach this scan, and the iOS-gate case below counts a @supports key only while it contains touch-callout, so two one-character drifts still fail no case: Switch/Switch.tsx:192 changed to '@meda (pointer: coarse)', and Chat/ChatComposerInput.tsx:270 changed to '@supports (-webkit-touch-calout: none)'. StyleX 0.19.0 compiles both into rules no browser applies. Matching every quoted at-rule key on one line, /'(@[a-z-]+ [^'\n]*)'/g, classing it by /pointer|coarse|fine|touch|callout/ and adding touchKeys.iosSupports to the allowed keys keeps this head passing and fails both; otherwise the header and the changeset can say they pin drifts that keep the at-rule and feature names.
There was a problem hiding this comment.
Changes requested: the drift test now fails both one-character drifts from the last review, and the internal touchKeys reference and press-paint guard fit the current touch specs, but the diff against main still carries #8360's stack (about 27,200 lines) beside this pull request's own 351. Please rebase once #8360 lands so the diff is your six files, and let the membership scan read every quoted at-rule key at touchPresentation.test.ts:266 (for example /'(@[^'\n]*)'/g): a capital or a digit in the at-rule name, such as '@Media (pointer: coarse)' in Switch/Switch.tsx, still passes every case, and StyleX compiles that key at a different priority than the literal.
[Automated review]
| !/\.(test|spec|stories)\./.test(entry.name) | ||
| ) { | ||
| const source = fs.readFileSync(full, 'utf8'); | ||
| for (const match of source.matchAll(/'(@[a-z-]+ [^'\n]*)'/g)) { |
There was a problem hiding this comment.
[a-z-]+ takes only lower-case letters and hyphens, so a capital or a digit in the at-rule name skips this scan: Switch/Switch.tsx:192 changed to '@Media (pointer: coarse)' fails no case in this file, and StyleX 0.19.0 compiles it at priority 6000 against the literal's 3200 (browsers still apply it, since at-rule names are case-insensitive), the reorder the touchPresentation.stylex.ts header warns about. Matching /'(@[^'\n]*)'/g keeps this head passing and fails it, and '@m3dia (pointer: coarse)' too; otherwise the comment above can say the at-rule is spelled in lower case.
|
Fixed in this push: a scan now requires every quoted at-rule key in core that names the pointer, the touch callout or a compact size to be a reference spelling or a query the presentation does not decide, so a one-character drift in the at-rule or in any word of the query fails: 5a7ae38, 6f42316, 9d19f94. The rest of the diff is #8360's, which lands first. |
|
Fixed in this push: the scan reads every quoted '@' string; one that names the pointer, the touch callout or a compact size, in any case, or that is within three characters of a reference spelling, case aside, must be that spelling, so a capital, a digit or a typo anywhere in the key fails: 439da85. |
There was a problem hiding this comment.
Changes requested: the drift test now fails every one-character drift that keeps a touch key's leading @, including the capital and digit cases from the last review, and the internal touchKeys reference and press-paint guard fit the current touch specs. The diff against main still carries #8360's stack (about 27,200 lines) beside this pull request's own 381, so please rebase once #8360 lands so the diff is your six files. One thing to know: the scan now also reads @astryx.… message ids and package specifiers, so a future id that contains fine or touch, such as '@astryx.filter.refine', would fail it as a drifted touch key.
[Automated review]
The touch keys (the coarse-pointer, touch-callout and compact-size media keys core's styles read) get one internal definition, and a scan pins every quoted at-rule key in core to its reference spelling: a key that names the pointer, the callout or a compact size, or that is a few characters off a reference, must be that spelling exactly. A browser proof shows the press paint outranks the coarse-pointer :active reset. meta#8331's change, rebased onto main as one commit. Core declares the interaction harness as a dev dependency, which its browser proof imports (the harness is on main; this is its first core user).
439da85 to
2bf77af
Compare
There was a problem hiding this comment.
Changes requested: the internal touchKeys reference, the drift and computed-key cases and the press-paint priority guard fit the current touch specs, and every spelling and import mutation tried fails one of them. The new Tab press spec, though, passes with the behaviour it checks removed: after mouse.down() the fine case only asserts a non-transparent layer, which the hover paint already gives, and headless Chromium does not enter :active while the DevTools finger is held, so the coarse case reads transparent with or without the coarse reset. Please make the fine case expect the pressed colour to differ from the hovered one, and in the coarse case assert the finger's press paint (data-astryx-press="on" and the layer's background image) and check the reset with the tab in :active under the phone profile (a held mouse button does it), or drop the spec and the harness devDependency.
[Automated review]
| await page.mouse.down(); | ||
| await expect | ||
| .poll(async () => (await tabState(page)).colour) | ||
| .not.toBe(TRANSPARENT); |
There was a problem hiding this comment.
This passes without the pressed overlay: the hover paint (black at 5%) stays on the layer while the pointer rests on the tab, so with Tab's :active arm removed the held press stays at 5% and not.toBe(TRANSPARENT) still holds. Recording the hovered colour and expecting the pressed colour to differ from it (10% in the default theme) makes the case fail when the press paint goes.
| const lift = await holdFinger(page); | ||
| const pressed = await tabState(page); | ||
| expect(pressed.coarse).toBe(true); | ||
| // Past any colour transition, it is still transparent. | ||
| await page.waitForTimeout(400); | ||
| expect((await tabState(page)).colour).toBe(TRANSPARENT); |
There was a problem hiding this comment.
Headless Chromium does not put the tab in :active during this DevTools touch hold (checked at once and at 400 ms), so the layer reads transparent with or without Tab's coarse :active reset (Tab.tsx:158). Asserting what the finger does paint (data-astryx-press="on" and a non-none background-image on the layer), and checking the reset with the tab in :active under the phone profile (a held mouse button, or CSS.forcePseudoState), makes the case fail when either behaviour goes.
The touch keys (the coarse-pointer, touch-callout and compact-size media keys core's styles read) get one internal definition, and a scan pins every quoted at-rule key in core to its reference spelling: a key that names the pointer, the callout or a compact size, or that is a few characters off a reference, must be that spelling exactly. A compiled-CSS check keeps the press paint above the coarse-pointer :active reset, and a browser proof shows a Tab's pressed overlay differing from its hover under a fine pointer and, under a coarse pointer, a finger's press painting as an image while the colour layer stays transparent in :active. meta#8331's change, rebased onto main as one commit. Core declares the interaction harness as a dev dependency, which its browser proof imports (the harness is on main; this is its first core user).
2bf77af to
ea9fbf5
Compare
|
Fixed in ea9fbf5: the fine case now expects the pressed colour to differ from the settled hover colour, and the coarse case asserts the finger's press paint ( |
There was a problem hiding this comment.
No changes requested: the internal touchKeys reference and its drift, computed-key and press-paint priority tests fit the current touch specs, and the Tab press spec now fails when the mouse pressed overlay, the coarse :active reset or the finger's press image is removed. The full CI lanes still need to run once the pull request leaves draft.
[Automated review]
cixzhang
added
the
ci:run
Small batch of the merge plan: one commit on main holding only this change.
Maintainer impact
Core spells the touch presentation's media queries as literal StyleX keys across its style files. If one spelling drifts from the resolver's query, that style silently stops applying on phones, and nothing fails.
Intended invariant
Every literal touch, compact and iOS-gate key in Core is the resolver's query (
spec:AST-094FR13,spec:AST-012FR9), and the press paint keeps outranking the coarse-pointer:activereset. No style file changes.Change
utils/touchPresentation.stylex.ts:touchKeys, the reference spellings, internal to Core (spec:AST-017FR21, DEC-27). Style files keep their literal keys: an imported key compiles to a different StyleX priority than the same literal (6000 against 3200) and would reorder the cascade. The iOS gate is only for the one style iOS WebKit itself requires, the 16 px input floor.utils/touchPresentation.test.ts:touchKeysequals the resolver's queries; no touch or Reduce Motion key in Core is a computed key; every quoted at-rule key in Core that names the pointer, the touch callout or a compact size, or that is within three characters of a reference spelling, is that spelling or a query the presentation does not decide; every iOS gate and compact-touch key is spelled and nested as the resolver.utils/pressPaintPriority.test.ts: in the compiled CSS, the press paint ranks above the coarse-pointer:activereset.TabList/__tests__/TabPressPointer.interaction.browser.spec.ts: under a fine pointer, a Tab's pressed overlay differs from its settled hover paint; under a coarse pointer, a finger's press marks the tab (data-astryx-press="on") and paints the layer's background image, and with the tab forced into:activethe color layer stays transparent. Chromium only: it holds a finger and forces:activethrough the DevTools protocol. Core declares the interaction harness as a dev dependency for it, its first Core user.utils/touchPresentation.ts: its header says why the keys are literal and namestouchKeys.Evidence
defineConstskey moved the coarse-pointer:activereset above the press paint, and WebKit lost the touch-down paint; on that headpressPaintPriority.test.tsfailed, and the computed-key test failed on 102 sites. Changing one literal key by one character fails the drift test, which names the file. Removing Tab's:activeoverlay and its press image fails the browser proof's fine case (the press stays at the 5% hover paint) and its finger case (no image); removing only the coarse:activereset fails its reset case (10% instead of transparent).Scope
Testing
Local gates at ea9fbf5, one commit on main d73908f holding only this change (8 files, +442/−4):
pnpm test:ui(core UI): 654 files, 16,450 tests passed.pnpm exec vitest run --project node: 804 files passed; the only failures are main's own story-tree and release-cache reds, tracked in test(storybook): run the story-tree check where Storybook is built, and bring the tree back to its shape #8509 and fix(cli): a crashed run's release install is recognized by the Node it ran on, whatever its name #8520.TabPressPointer.interaction.browser.spec.tson a built Storybook (playwright.interaction.config.ts): Chromium 2 passed, and each case failed on a Storybook built with its behavior removed; WebKit skips it, since only Chromium holds a finger and forces:activethrough the DevTools protocol.pnpm install --frozen-lockfileagrees with the lockfile;tsc --project packages/core/tsconfig.json --noEmit, strict ESLint and Prettier on every changed file: clean.pnpm check:repo: clean.