Skip to content

Navigation Menu

Sign in
Sign up

fix(Tokenizer): avoid blank trailing input row - #6057

Open
imdreamrunner wants to merge 13 commits into
main from
investigate/powersearch-empty-line
Open

fix(Tokenizer): avoid blank trailing input row #6057
imdreamrunner wants to merge 13 commits into
main from
investigate/powersearch-empty-line

Conversation

@imdreamrunner

@imdreamrunner imdreamrunner commented Sep 4, 2026
edited
Loading

Copy link
×ばつ 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](https://files.catbox.moe/5o5iqk.png) After — the identical fixture stays one row (32px field): ![After: identical PowerSearch fixture stays on one row](https://files.catbox.moe/yyqwn2.png) 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 typecheck` - `pnpm -F @astryxdesign/storybook typecheck` - `pnpm vitest run packages/core/src/Tokenizer/Tokenizer.test.tsx packages/core/src/PowerSearch/PowerSearch.test.tsx` — 106 passed - [Coarse-pointer reproduction on the unfixed commit](https://github.com/facebook/astryx/actions/runs/33941690649/job/101240905930) — fails with exactly 1.00px overlap in both LTR and RTL - [Coarse-pointer verification on the final clean head](https://github.com/facebook/astryx/actions/runs/33944223219/job/101247861651) — passes both LTR and RTL The 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](https://facebook.github.io/astryx/pr/6057/iframe.html?id=core-powersearch--near-full-token-row&viewMode=story) or [the RTL variant](https://facebook.github.io/astryx/pr/6057/iframe.html?id=core-powersearch--near-full-token-row-rtl&viewMode=story). " data-view-component="true"> Copy Markdown
Contributor

Summary

  • remove Tokenizer's empty trailing combobox from flex layout so it cannot create a blank final row
  • bound the empty search hit surface before the measured end-control lane, including the Clear all button's 2px coarse-pointer hit outset
  • keep pills and controls above the background input, while restoring the normal in-flow editing width as soon as a query is entered
  • add deterministic real-Chromium guards for LTR and RTL layout, hit testing, focus, and coarse-pointer behavior

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

The 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.

vercel Bot commented Sep 4, 2026
edited
Loading

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
astryx Ready Ready Preview Sep 5, 2026 4:22am UTC

Request Review

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Sep 4, 2026
@github-actions github-actions Bot added the needs:design-review Affects visuals — Design should review label Sep 4, 2026

@cixzhang cixzhang left a comment

Copy link
Copy Markdown
Contributor

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]

github-actions Bot added a commit that referenced this pull request Sep 4, 2026

github-actions Bot commented Sep 4, 2026
edited
Loading

Copy link
Copy Markdown
Contributor

PR Analysis Report

📚 Storybook Preview

View Storybook for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

🧪 Sandbox Preview

View Sandbox for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

Modified Components

Tokenizer (@astryxdesign/core) · View in Storybook
Metric Before After Delta
Bundle Size (ESM) N/A N/A N/A
Lines of Code N/A 845 -
Complexity N/A Very High (105) -

Bundle Size Summary

Package Size (ESM) Size (CJS) Gzipped
@astryxdesign/core N/A 4.8KB 1.2KB

Accessibility Audit

Status: 1 accessibility violation(s) found — 1 serious.

Tokenizer - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 2/24 stories · Learn more
    • WCAG: 1.4.3 (Level AA)

Visual Regression

Status: No visual change across 2 compared shot(s).


Generated by PR Enrichment workflow | Storybook | Sandbox | View full report

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Contributor

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]

github-actions Bot added a commit that referenced this pull request Sep 5, 2026

@cixzhang cixzhang left a comment

Copy link
Copy Markdown
Contributor

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]

github-actions Bot added a commit that referenced this pull request Sep 5, 2026

Copy link
Copy Markdown

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?

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Contributor

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]

Copy link
Copy Markdown

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.

github-actions Bot added a commit that referenced this pull request Sep 5, 2026

@cixzhang cixzhang left a comment

Copy link
Copy Markdown
Contributor

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]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cixzhang cixzhang cixzhang approved these changes
@josephfarina josephfarina Awaiting requested review from josephfarina josephfarina is a code owner
@cvkxx cvkxx Awaiting requested review from cvkxx
@ernestt ernestt Awaiting requested review from ernestt
@kentonquatman kentonquatman Awaiting requested review from kentonquatman
@rubyycheung rubyycheung Awaiting requested review from rubyycheung

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Meta Open Source bot. needs:design-review Affects visuals — Design should review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /