-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(Tokenizer): avoid blank trailing input row - #6057
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@cixzhang
cixzhang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, the row now stays compact. One blocker: the empty search target sits under Clear all, so clicking it removes filters instead of focusing search in LTR and RTL. Please preserve a non-overlapping input hit target while keeping the compact row.
[Reviewed by Robohands]
PR Analysis Report📚 Storybook PreviewView Storybook for this PR 🧪 Sandbox PreviewView Sandbox for this PR Modified ComponentsTokenizer (@astryxdesign/core) · View in Storybook
Bundle Size Summary
Accessibility AuditStatus: 1 accessibility violation(s) found — 1 serious. Tokenizer - 1 issue(s)
Visual RegressionStatus: No visual change across 2 compared shot(s). Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
534b814 to
dc1e1d0
Compare
xinzi-bot
commented
Sep 5, 2026
Addressed in dc1e1d0. The empty compact input now keeps the measured end-lane reserve as logical outer spacing, so its flex base remains compact while its hit rectangle stops before Clear all. I added real-browser geometry guards for both LTR and RTL that require the input to stay on the token row, retain positive width, and not overlap the clear button. Both new guards pass in CI: https://github.com/facebook/astryx/actions/runs/33931922038/job/101212993760
Could you take another look?
@cixzhang
cixzhang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this fixes the Clear all overlap, but the exact-head story now fails its original compact-row assertion in both LTR and RTL: input top 47px, final token bottom 44px. The field is 55px tall again, so users still see the blank second row this PR is meant to remove. Please keep a positive, non-overlapping input hit target without restoring the extra row.
[Reviewed by Robohands]
@cixzhang
cixzhang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People clicking the empty search area can still remove a filter instead of focusing search. This exact head restores the compact row, but real Chromium renders the 4px input overlapping Clear all by 3.7px in both LTR and RTL; its center hit-tests as Clear all. The added story throws that same error locally while CI passes, so the regression fixture is not portable enough to gate this. Please keep the compact row while preserving a non-overlapping search target in both directions, and make the browser guard deterministic across supported environments.
[Reviewed by Robohands]
xinzi-bot
commented
Sep 5, 2026
You were right—the previous guard raced the ResizeObserver and could measure before the end-lane reserve was applied. Addressed in 3705ac0:
- the empty input is now a minimal, non-growing flex item; the wrapper remains the larger click-to-focus surface
- the guard waits for the measured lane before asserting
- LTR and RTL each require one-row alignment, positive input width, no Clear all overlap, a successful center-point browser hit test, and focus after click
Both hardened guards pass in Chromium: https://github.com/facebook/astryx/actions/runs/33933639943/job/101217896127
I also replaced the PR evidence with apples-to-apples captures using the same three filters, 600 CSS-pixel fixture width, and 2x output as the original report. Could you take another look?
9f41204 to
a0f64a2
Compare
xinzi-bot
commented
Sep 5, 2026
Agreed—the 4px in-flow target still had an impossible boundary case: at some widths it had to overlap the clear lane or wrap. I replaced that approach in a0f64a2.
While empty, the combobox is now an absolutely positioned first-row background hit surface bounded one pixel before the measured end lane. It consumes no flex width, and pills plus end controls are layered above it. Typing restores the normal in-flow input.
The guard now waits for ResizeObserver before measuring, then verifies in both LTR and RTL: one-row alignment, positive input width, no Clear all overlap, correct hit-testing at the final pill and clear-button centers, an exposed point that hit-tests to the input, and focus after clicking that point. Both variants pass in Chromium: https://github.com/facebook/astryx/actions/runs/33935679227/job/101223840998
I also replaced the PR explanation and confirmed the before/after captures are apples-to-apples: identical filters, 600 CSS-pixel fixture width, 2x output, and the after capture was taken only after the served iframe hash matched a0f64a2. Please take another look.
@cixzhang
cixzhang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People tapping the exposed search area can still remove a filter instead of focusing search. In coarse-pointer Chromium, Clear all’s 24px hit area extends 2px beyond its visible box while the input leaves only a 1px gap; 1–1.5px inside the input resolves to Clear all in both LTR and RTL, and clicking there removes a filter. The new browser stories pass because they run with a fine pointer and compare only the visible button rectangle. Please reserve the full effective Clear all hit area and run this guard with pointer: coarse in both directions.
[Reviewed by Robohands]
xinzi-bot
commented
Sep 5, 2026
Addressed on 00ebfea84.
I first tightened the guard and reproduced the reported failure in Linux Chromium with pointer: coarse: the empty combobox overlapped the effective Clear all hit target by exactly 1.00px in both LTR and RTL (failing run).
The empty input now reserves the clear button's full 2px coarse-pointer outset plus the existing 1px separation. The durable browser guard runs both directions with touch enabled, reads the ::after hit-area geometry, verifies all pills stay on row one, verifies the input stays on that row, checks non-overlap and hit-testing, and clicks an exposed input point to confirm focus. The final clean-head run passes.
I also replaced the mismatched screenshots in the PR description with an A/B captured in the same Chromium run using identical data, theme, viewport, scale, and fonts.
@cixzhang
cixzhang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this now keeps the row compact without letting the search or Clear all targets steal each other’s clicks in LTR/RTL and fine/coarse modes.
[Reviewed by Robohands]
Uh oh!
There was an error while loading. Please reload this page.
Summary
Why
In a nearly full PowerSearch row, the mounted combobox still participated in flex layout. Its minimum editing width could wrap by itself and make the field 55px tall even though no content appeared on the second row.
The empty input is now an absolutely positioned first-row background hit surface. It does not consume flex space, so it cannot create a row, and the pills and controls remain the foreground hit targets. Its inline end stops 3px before the measured end lane: 2px for InputClearButton's invisible 24px coarse-pointer target plus 1px separation. Typing restores the ordinary in-flow input.
Apples-to-apples visual verification
Both images below were captured in the same Linux Chromium run with identical data, neutral/light theme, 600 CSS-pixel fixture, 629 ×ばつ 100 viewport, ×ばつ device scale, and settled fonts. The before capture restores only the PR base's input-layout rules; the after capture uses this branch.
Before — the tokens fit row one, but the empty combobox creates a blank second row (55px field):
Before: empty combobox creates a blank second row
After — the identical fixture stays one row (32px field):
After: identical PowerSearch fixture stays on one row
The earlier screenshots were removed because they mixed the original deployed story with the PR-preview story and therefore had different token styling; they were not a valid visual comparison.
Verification
pnpm -F @astryxdesign/core typecheckpnpm -F @astryxdesign/storybook typecheckpnpm vitest run packages/core/src/Tokenizer/Tokenizer.test.tsx packages/core/src/PowerSearch/PowerSearch.test.tsx— 106 passedThe browser guard requires all pills to remain on row one, the empty combobox to share that row, a positive exposed input hit target, no overlap with the full Clear all touch target, correct token/Clear all hit testing, and focus after clicking the input.
Open the LTR regression story or the RTL variant.