Repository navigation
fix(core): a control in an opening layer keeps its settled touch insets - #9957
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
needs:design-review
|
AI review status for this pull request.
|
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC |
|---|---|---|
| InfoTip | 37.2 kB (+321 B) · 5.0 kB CSS (0) | — |
| Switch | 87.3 kB (+293 B) · 9.5 kB CSS (0) | — |
Package size: @astryxdesign/core 6.39 MB packed (+1.8 kB); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 8 rows, core 2 rows; 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
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.
Changes requested: the layout-px measurement looks right, but the release note describes a defect that main does not have.
- The changeset's "Before, a popover growing out of its trigger left the insets its first, scaled frame gave." does not hold on
main, where the Popover's switches already settle at10px 2px 10px 2px(as the pull request's own evidence notes), so reword it to what the tests prove, for example "Before, a control measured while an ancestor was scaled kept that frame's insets until the next resize."
The new unit case and browser spec fail on main and pass here, and the Switch and InfoTip suites stay green. One non-blocking inline suggestion splits a source comment into one topic per sentence.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] Under a finger, a Switch or InfoTip in a layer that opens from a smaller scale keeps the touch insets of the size the layer settles at. Before, a popover growing out of its trigger left the insets its first, scaled frame gave. |
There was a problem hiding this comment.
On main the Popover's switches already settle at 10px 2px 10px 2px (the pull request's own evidence says so too), so "Before, a popover growing out of its trigger…" describes a defect released versions do not have. Suggested wording, matching what the tests prove:
[fix] Under a finger, a Switch or InfoTip measured while an ancestor is scaled keeps the touch insets of its unscaled layout. Before, that measurement kept the scaled frame's insets until the next resize.
There was a problem hiding this comment.
Done in 47ec35b: the release note now uses this wording.
| // The insets are in the control's layout px; the rects stay on screen for | ||
| // a touch's distances. Neighbours map by the control's scale, as if in its | ||
| // layer. One outside an opening layer then reads farther away; a touch | ||
| // re-measures at once and counts it. |
There was a problem hiding this comment.
Non-blocking: the first and third sentences each join two topics with a semicolon. A rewrite with one topic per sentence:
// The insets are in the control's layout px. The returned rects stay in
// screen px for a touch's distance checks. A neighbour maps by the
// control's scale, as if it is in the same layer. A neighbour outside that
// layer then reads farther away. A touch compares screen distances, so it
// still counts that neighbour.There was a problem hiding this comment.
Done in 47ec35b: the comment now has one topic per sentence, as suggested.
utils/hitArea measured a control's hit insets from client rects. A layer that opens from a smaller scale scales those rects, so a pass during the opening left the insets of a scaled frame. A popover growing out of its trigger left a switch's left inset at 21.8 px instead of 2 px, and nothing measured it again. The insets are lengths in the control's own box, so a pass now measures in its layout px: each rect's offset from the control is divided by the control's scale on screen, its client size over its layout size.
9f1bb9b to
47ec35b
Compare
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet.
The release note now describes what the tests prove, and the layout-px measurement holds up: the new unit case and the Chromium spec fail on main and pass here, and the existing Switch and InfoTip hit-area suites stay green.
Next step: mark the pull request ready for review, and a maintainer adds the ci:run label so CI runs.
[Automated review]
cixzhang
added
the
ci:run
User impact
Under a finger, a Switch or InfoTip measured while an ancestor is scaled keeps the scaled frame's touch insets until the next resize. On the open touch presentation, a popover growing out of its trigger leaves a switch a 21.8 px left inset and a 0.44 px right one, not the settled 2 px.
Problem and solution fit
watchHitAreameasures a control from client rects once it is on screen, and during the opening the layer's scale is in them.Expected behavior and authority
spec:AST-094FR42: under a coarse pointer, touch targets meet 44 px through an invisible hit area, clamped to half the gap to a neighbour.Smallest restoration
measureOnecomputes insets from layout-px rects; a touch's distance checks keep screen rects. No new observer, listener or timer; a mouse still measures nothing.Evidence
2.23 0.44 0.05 21.8,0.07 0.44 0.05 21.8and0.07 0.44 21.95 21.8px (top, right, bottom, left) in Chromium on the phone profile, 3 of 3 runs.10 2 10 2px for all three, 3 of 3 runs, as measured at rest.main, the Popover's CSS entry animation never reachesgetBoundingClientRectin Chromium (rect 280 px while the transform readsmatrix(0.95…)), somainis right there:10 2 10 2px either way. The defect shows wherever the scale reaches the rects.watchHitAreacase at half size hears{top: 16, right: 2, …}onmainand the full-size{top: 10, right: 4, …}here. A browser case holds the Switch story at scale 0.5 for the first pass: onmainits settled insets stay16px 12px 16px 12pxagainst10px 2px 10px 2pxat rest; here they match, 3 of 3 runs. A mouse context writes none.Scope
Testing
At 47ec35b on
mainfbb81c6:vitest run --project uionhitArea.test.ts,Switch,InfoTip,SideNav.test.tsxandResizeHandle.test.tsx: 10 files, 530 tests passed.playwright test packages/core/src/utils/__tests__/hitArea.interaction.browser.spec.ts --config playwright.interaction.config.ts --project=chromium, on a Storybook built at this commit: 2 passed in 3 runs. Onmain's build the first case fails.Switch.interaction.browser.spec.tsin Chromium: 44 passed onmain's build and on this one.setProperty, with a positive control on its filter. It is not part of this change.tsc --noEmit,pnpm lint,eslint(CI=true), Prettier andcheck:changesetspass.pnpm review:pre: 0 findings.Overlap with open pull requests
clampedHitInsetscall inmeasureOne: they addwatch.growth?.(target). Whichever lands second rebases. The merged call passes the layout-px target to both, aswatch.growth?.(inLayout(target, target, scale)). Expected order: this one first.hitArea.tsand its test.