[fields] Avoid focusing the field on blank space clicks#22285
Conversation
The root click handler unconditionally selected the first section on the very first click anywhere on the field, due to a misnamed `hasClickedOnASection` check that was always true. Replace it with the existing `getSectionIndexFromDOMElement` getter so clicks on padding, separators, or any non-section target become a no-op. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
The previous commit only fixed the click handler. In real browsers the sections container is `tabindex=0` and the browser auto-focuses it on blank-space mousedown, which then runs `handleFocus` and selects the first section -- so the bug still reproduces. Track whether the most recent `mousedown` landed on blank space and skip both `setFocused(true)` and the first-section fallback in `handleFocus` when that focus is the synthetic browser auto-focus on the container. Plumbed `onMouseDown` through `useField`, `useField.types`, and `PickersTextField` so the handler reaches the input root. Updated the test to simulate the native focus event and assert the focused class is not added. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Clicking the floating label was focusing the hidden input via the standard `<label htmlFor>` behavior. The hidden input's `onFocus` forwards to `useField`'s `handleFocus`, which set `focused=true` and selected the first section -- the user-visible bug: a click outside the field's editable area highlighted the first section. Block the label's `mousedown` and `click` defaults so it no longer focuses the hidden input. Screen-reader association via `htmlFor` / `aria-labelledby` is preserved; user-supplied handlers from `slotProps.inputLabel` are still invoked. Reverted the speculative `onMouseDown` plumbing through `useField` (it didn't address this bug and added churn). The previous commit's click-handler cleanup in `handleClick` is kept. Updated tests to cover label clicks (focused and unfocused fields) and to keep the regression guards for direct section clicks. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
The label-mousedown/click preventDefault from the previous commit was a guess based on a misread of the user's bug. The user clarified that the actual symptom is clicks "outside the field outline" focusing the field with a section selected based on click position (left -> first section, right -> last section), and the label fix did not change that. Reverting to keep only the original `handleClick` cleanup. I'll leave the diagnosis open until I have a real reproduction of the click flow the user is observing. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Chromium has a quirk where clicking on a non-editable element that visually surrounds a `contenteditable` descendant can still delegate focus onto that descendant. In the picker fields this manifested as clicking outside the visible field outline (e.g. on a parent flex container that centers the field) focusing the nearest section's `contenteditable` span -- the user-reported bug. Track the most recent `mousedown` target across the page via a document-level capture listener (cleared in the next macrotask so keyboard/programmatic focus isn't gated on stale state). When a section's `onFocus` fires and the recent mousedown landed outside the field root entirely, blur the section to undo Chromium's delegation. Mousedowns inside the field root (including the sections container's padding, which is how some test helpers open pickers) are still treated as legitimate field interactions. Firefox and Safari do not exhibit the delegation behavior, so this purely fixes the Chromium-only quirk. See https://stackoverflow.com/questions/34354085/clicking-outside-a-contenteditable-div-stills-give-focus-to-it Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Replace the previous mousedown-tracking + blur-after-focus workaround with a one-liner: a section's content span is `contenteditable` only while it is the currently selected section. This removes the Chromium focus-delegation surface entirely. Chromium delegates clicks on a non-editable ancestor (e.g. a wrapping flex container that visually surrounds the field) to the nearest `contenteditable` descendant; with no editable section in the idle state, there is nothing to delegate to, so the field stays unfocused. No flicker (focus is never moved in the first place), no global listeners, no timeout dance. Verified that direct section clicks, Tab navigation, keyboard arrow navigation, typing, and `Ctrl+A` still work -- the active section is made editable on selection, before `useEnhancedEffect` programmatically focuses it. See https://stackoverflow.com/questions/34354085/clicking-outside-a-contenteditable-div-stills-give-focus-to-it Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Several e2e tests opened the field by clicking on the \`PickersSectionList\` root and relied on Chromium's focus delegation to forward the focus onto the nearest section's \`contenteditable\`. The previous commit removed that delegation surface (sections are now \`contenteditable\` only while selected), so a click on the empty container no longer focuses anything -- just like a real user clicking the field's padding rather than its date text. Update each impacted test to click on the first \`role="spinbutton"\` inside the relevant container. This better reflects the user gesture the test is meant to model (clicking on a section to focus it / open the picker) and is not Chromium-specific. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Replace the previous \`contenteditable\`-toggling fix with the same pattern React Spectrum uses for its DateField, which is friendlier to screen readers and lets Playwright's \`fill()\` work without an extra click. Sections stay \`contenteditable=true\` whenever the field is editable, preserving the screen-reader announcement state and keeping the existing e2e tests working as-is. To prevent the browser from delegating focus from a click on a non-section ancestor onto the nearest section's contenteditable: - The field root attaches an \`onMouseDown\` handler. For primary-button clicks landing inside the sections container but NOT on a section span, it calls \`event.preventDefault()\` (suppressing the native delegation) and manually focuses the section whose horizontal center is closest to the click point. This mirrors the native "click anywhere on the input focuses the closest editable bit" behavior. - Each section's \`onMouseDown\` and \`onPointerDown\` call \`event.stopPropagation()\` so the parent handler does not fire for direct section clicks; native focus on the section is the right behavior in that case. - Adornments (open/clear buttons) live outside the sections container, so the parent handler skips them and their own focus/click semantics are unchanged. This is the approach \`useDateSegment\` / \`useDatePickerGroup\` use in \`@react-aria/datepicker\` -- see https://github.com/adobe/react-spectrum/blob/main/packages/react-aria/src/datepicker/useDatePickerGroup.ts Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Earlier the section content hooked its own \`onMouseDown\` to \`stopPropagation\` so that the field root's \`mousedown\` handler did not run for clicks landing directly on a section. That broke event propagation that downstream consumers (e.g. multi-input range pickers opening on click) rely on. Move the check up to the field root: instead of stopping propagation on the section, the root's \`onMouseDown\` looks at \`event.target\` and exits early when the click lands on (or inside) a \`[data-sectionindex]\` section span. Native focus on the section runs as before, the click event still bubbles to all the original handlers, and Chromium's focus delegation is still suppressed for clicks on padding / separator gaps inside the sections container. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Plain \`contenteditable=true\` on every section left a Chromium delegation surface: clicks on a non-section ancestor of the field (e.g. a wrapping flex container in the docs demo) focused the nearest section. The mousedown handler on the field root only fires for clicks within the field, so it could not stop those outside-field clicks. Make a section \`contenteditable\` only while the field is focused (or this specific section is the selected one). In the idle state no descendant is editable, so Chromium has no target to delegate to and clicks anywhere outside the field are inert. Once the field is focused -- via Tab, a click on a section, or a click on the field's padding handled by the root mousedown -- every section becomes editable, so keyboard editing, programmatic focus, and tools like Playwright's \`fill()\` keep working uniformly. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…editable The previous commit gated section-content \`contenteditable\` on the field's focused state (true when focused, false when idle). That closed Chromium's outside-click delegation, but it had a real downside: when React re-rendered between \`mousedown\` and \`mouseup\` to flip the attribute, Chromium recomputed the click event's target as the common ancestor of the two phases (the \`FormControl\` wrapper) instead of the section span. The click event then never bubbled through the \`PickersInputBase\` root, breaking handlers that depend on it -- most visibly the multi-input range picker's open-on-click flow, which made several e2e tests time out waiting for the tooltip. Replace the attribute flip with a pure CSS rule on the section content: \`-webkit-user-modify: read-only; user-select: none\` while the field input root has no \`:focus-within\`. \`user-modify: read-only\` blocks Chromium's delegation just like \`contenteditable=false\` does, but the attribute itself stays \`true\` throughout, so the click target stays on the spinbutton and the rest of the click chain runs normally. As soon as anything inside the input root receives focus, \`:focus-within\` flips, the rule is no longer matched, and the spans behave like normal \`contenteditable\` elements -- keyboard editing, programmatic focus, and \`fill()\` all keep working uniformly. Verified with a Playwright reproduction: - click 100px outside the field outline: no focus moves (idle case) - click directly on a section: focuses that section - type into the focused section: characters are inserted Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
This reverts commit 23ec403.
…ui-x into claude/musing-albattani-b80c2f
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…ui-x into claude/musing-albattani-b80c2f
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
The CSS rule against Chromium's focus delegation needs trusted pointer events to be meaningfully verified. Switch the test to drive the click through `@vitest/browser/context`'s CDP-backed userEvent so the assertion is about the observable end result (no section is focused after clicking on an ancestor outside the field root), not the implementation detail. Adds `@vitest/browser` as an explicit devDependency so vite can resolve the dynamic import in browser-mode tests (`pnpm test:browser`). The import stays inside the `skipIf(isJSDOM)` test so it never runs in `pnpm test:unit`. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…mport" This reverts commit cf362ad.
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…attani-b80c2f # Conflicts: # pnpm-lock.yaml
Code changes: - `handleClick`'s `!focused` branch restores the defensive `setSelectedSections(sectionOrder.startIndex)` when no section is active, so programmatic `element.click()` (no preceding mousedown) doesn't leave the field in a "focused but no active section" state. - `handleMouseDown` guards `Number(sectionElement.dataset.sectionindex)` against `NaN` with `Number.isInteger`. Defense in depth; the dataset is always set in practice. - `handleMouseDown` skips the redundant `setSelectedSections` call when the target section is already selected, so userland `onSelectedSectionsChange` doesn't fire twice for clicks on the already-active section. Comment tightenings: - `useFieldRootProps.ts:204` (handleClick !focused): replaces "Section selection is driven by focus, not click" with a more concrete explanation of when the branch matters. - `useField.ts:204-208` (handleRootMouseDown): clarifies that the `isDefaultPrevented` check catches capture-phase parents and the clear/open buttons, but does NOT honor inline userland `preventDefault` as an opt-out. - `useFieldRootProps.ts:368` (findClosestSectionIndexToPoint JSDoc): restates the null-return as "no `role=spinbutton` descendants" instead of "zero sections", since the implementation literally queries for `[role="spinbutton"]`. Tests: - Separator-click regression test now clicks the `/` after Day (data-sectionindex=1) instead of after Month (=0). Under JSDOM's zero-sized rects, the closest-section fallback always picks 0, so the new target discriminates between the `[data-sectionindex]` lookup (Day) and the fallback (Month). Dropping the lookup would now fail the test. - `'all'`-preservation test now also fires `click` so the 0-tick `containerClickTimeout` in `handleClick` is exercised. - New tests: non-primary mousedown (`button: 2` right-click) is a no-op; capture-phase parent `preventDefault` suppresses section selection (pins the contract); userland `onMouseDown` consumer receives the event. - New `selection.DateTimeField.test.tsx` browser test mirrors the DateField/TimeField outer-wrapper flex test against DateTimeField's longer (MM/DD/YYYY hh:mm aa) layout. Keeps the Chromium delegation gate covered for the three field variants. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
|
Thanks @flaviendelangle, addressed in Important issues#1 — Kept the current asymmetric behavior (capture-phase parent suppresses, inline userland does not), and added a test to pin the contract. The asymmetry is intentional: capture-phase Also tightened the comment to match (#9 below). #2 — Restored the defensive fallback when #3 — Closest-section / separator tests not discriminating Updated the separator test to click the The JSDOM "non-section descendant" test stays — yes, with 0×0 rects it only confirms the path runs, but the comment already calls that out, and the actual closest-section math is covered by the browser-only test below. #4 — Added. #5 — Userland Added. #6 — CSS gate coverage one section-layout deep Added #7 — Added Comment issues#8 — handleClick #9 — handleRootMouseDown comment — rewritten to be explicit about the capture-phase / userland-inline asymmetry and why userland inline #10 — #11 — Playwright Pushing back on this one. The comment already explains why the gate exists (Chromium-only quirk + WebKit Suggestions
Full suites locally: 3366 pickers browser, ~3340 pickers jsdom, ~1009 pickers-pro (the random failures in DesktopDateTimePicker / DateTimeRangePicker describeValue runs are pre-existing parallel-load timeouts — they pass individually and aren't related to this PR). |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
…attani-b80c2f # Conflicts: # pnpm-lock.yaml
flaviendelangle
left a comment
There was a problem hiding this comment.
PR review
This is a carefully-built, well-commented fix for the Chromium blank-space focus-delegation quirk, with good cross-variant test coverage. Nothing is merge-blocking. One confirmed, minor behavior change: clicking a section now fires the public onSelectedSectionsChange callback one extra time (an empirically-measured 2→3 invocations with the identical value). I verified the core logic in jsdom and reproduced the callback count against both the PR and master.
Bugs (1)
1. 🟡 Clicking a section fires onSelectedSectionsChange an extra (redundant) time
Location: packages/x-date-pickers/src/internals/hooks/useField/useFieldRootProps.ts:266-272
// Skip the redundant state update if the selected section is already the
// target, so userland `onSelectedSectionsChange` doesn't fire twice ...
if (parsedIndex !== parsedSelectedSections) {
setSelectedSections(parsedIndex);
}setSelectedSections calls onSelectedSectionsChange unconditionally (no value dedup — useFieldState.ts). The new mousedown path calls setSelectedSections(parsedIndex), which drives .focus() on the target section; that focus fires the section's own onFocus → createFocusHandler → setSelectedSections (useFieldSectionContentProps.ts:148-156), and then the click bubble fires the section container's onClick → setSelectedSections (useFieldSectionContainerProps.ts:21-32). The dedup guard quoted above only suppresses the already-selected-section case; for a click on a different section all three fire.
I confirmed this empirically by spying on onSelectedSectionsChange and clicking an unselected section:
master: 2 invocations →[2, 2]- this PR: 3 invocations →
[2, 2, 2]
The value is always identical, so end state is correct and controlled consumers (React bails on equal state) are unaffected. The visible impact is limited to side-effecting listeners (analytics/logging/counters), which now observe one extra call per section click.
Failure scenario: A consumer with <DateField onSelectedSectionsChange={logSelectionChange} /> that records each invocation sees 3 events for a single click on a new section instead of 2.
Fix: Mirror the existing mousedown dedup in the section container's click handler — since mousedown is now authoritative for pointer input, guard createHandleClick in useFieldSectionContainerProps.ts to bail when the section is already selected (if (parsedSelectedSections === sectionIndex) return;, threading parsedSelectedSections into that hook). That removes the redundant click-bubble fire while preserving the no-mousedown (programmatic/AT) path, where parsedSelectedSections won't yet equal the index. Alternatively, accept it and drop the misleading "doesn't fire twice" claim from the comment, since the guard only covers one of the two cases.
Tests (1)
1. ℹ️ "preserve the all-sections selection" test doesn't actually exercise the 0-tick timeout
Location: packages/x-date-pickers/src/DateField/tests/selection.DateField.test.tsx:193-200
// ... Fire both mousedown and click so the 0-tick
// containerClickTimeout in `handleClick` is also exercised.
fireEvent.mouseDown(sectionsContainer);
fireEvent.click(sectionsContainer);
expect(getCleanedSelectedContent()).to.equal('MM/DD/YYYY');handleClick's 'all' branch schedules its work in containerClickTimeout.start(0, …) (a real setTimeout(0) macrotask). fireEvent is synchronous and act() does not flush macrotasks, so the assertion runs before the timeout body executes. The test verifies the synchronous path keeps 'all' intact, but the new if (!selection || selection.rangeCount === 0) fallback inside the timeout (useFieldRootProps.ts:183-187) is never reached during the assertion window — contrary to the comment's claim that it is "also exercised."
Failure scenario: The new transient-selection fallback could regress (e.g., wrong index, or a stray setSelectedSections after focus moves) and every selection test would still pass green.
Fix: Either advance timers / await a tick before asserting to actually observe the timeout result, or soften the comment to reflect that only the synchronous 'all'-preservation path is asserted.
Simplifications (0)
No findings.
Docs (0)
No findings. The inline comments are unusually thorough and accurate; the @supports (-webkit-app-region: drag) Chromium gate, the mousedown/isDefaultPrevented contract, and the findClosestSectionIndexToPoint JSDoc all match the code. No docs/ API-JSON regeneration is needed — onMouseDown follows the same pattern as onKeyDown/onClick (in propTypes, not in the API-docs JSON), and propTypes were updated.
Verdict
Approve after nits — no blocking issues; the only behavior change is a confirmed-but-harmless extra onSelectedSectionsChange invocation per section click, plus a test whose comment overstates its coverage.
🤖 Review generated with Claude Code
|
Looks like we just have the |
The new `mousedown` section selector added a third `setSelectedSections` call per pointer click on a section (mousedown + the section's own `onFocus` + the section container's `onClick`), so the public `onSelectedSectionsChange` fired 3x instead of master's 2x for a click on a new section. `setSelectedSections` invokes the callback unconditionally (no value dedup), so the extra call reached side-effecting consumer listeners. Move the dedup to the authoritative spot: - `useFieldSectionContainerProps`: bail out of the section container's `onClick` when `parsedSelectedSections === sectionIndex`. For pointer input the `mousedown` handler already selected the section before the click bubbles, so this absorbs the redundant call. The non-pointer paths (programmatic `element.click()`, some AT activations) have no preceding `mousedown`, so `parsedSelectedSections` doesn't match yet and selection proceeds normally. - `useFieldRootProps`: drop the now-redundant `mousedown` dedup guard and always `setSelectedSections(parsedIndex)`. Keeping both guards dropped the already-selected-section click to 0 callbacks (master fires 1); with only the `onClick` guard the counts match master exactly -- 2 for a new section, 1 for the already-selected one. Add a regression test locking the exact call args ([[2],[2]] then [[2]]). Also soften the "all-sections preservation" test comment: `handleClick`'s `'all'` branch schedules its cursor-positioning work in a 0-tick `setTimeout` that `fireEvent` does not flush, so the test only asserts the synchronous outcome -- the prior comment overstated that the timeout body was exercised. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
The merge of master bumped vitest to 4.1.8, but the `@vitest/browser` catalog entry this branch added still pinned ^4.1.7, leaving the catalogs config out of sync with the lockfile. CI installs with `--frozen-lockfile`, which fails on `ERR_PNPM_LOCKFILE_CONFIG_MISMATCH`. Regenerate so `@vitest/browser` resolves to 4.1.8, matching the sibling `@vitest/browser-playwright` / `@vitest/coverage-v8` entries. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
The previous dedup added `parsedSelectedSections` to `createHandleClick`'s dependency array. That dep was not the real cost -- `setSelectedSections` (an unmemoized plain function in `useFieldState`) was already in the deps, so the per-section onClick factory was recreated on every render regardless. Replace the per-index factory with a single `useEventCallback` handler that reads the section index from the container's `data-sectionindex` at click time and reads `disabled` / `parsedSelectedSections` / `setSelectedSections` through the event callback's latest closure. None of them sit in a dependency array now, so the `onClick` identity -- and the returned container-props factory -- stay stable across renders instead of being rebuilt every time. Behavior is unchanged: the dedup regression test still asserts the exact `onSelectedSectionsChange` call args ([[2],[2]] then [[2]]). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
6ae02a8 to
39f5996
Compare
|
Thanks @flaviendelangle - both valid. Fixed in 6581554 (dedup) and 39f5996 (stabilization). Finding 1 - extra
Regression test added; non-pointer path preserved. Perf follow-up (39f5996): one stable Finding 2 - test comment. Right, the |
flaviendelangle
left a comment
There was a problem hiding this comment.
PR review
This PR replaces Chromium's contenteditable focus-delegation with an explicit mousedown → "select the nearest section" mechanism, gated by a Chromium-only @supports CSS rule, plus a handleClick/section-container cleanup. I verified the logic by reading the full handler chain (useFieldRootProps → useField → PickersInputBase → syncSelectionToDOM + the dedicated focus effect at useField.ts:277) and ran the jsdom field suites: DateField selection (25 pass / 2 browser-skipped), DateField/TimeField/DateTimeField editing+keyboard (743 pass), pro single-input range selection+editing (87 pass), and DesktopDatePicker field/keep-open + DateField describeValue (25 pass). Nothing is merge-blocking — no 🔴. The design's two load-bearing assumptions (programmatic .focus() works through the WebkitUserModify: read-only rule, and WebKit never matches the @supports gate) are only validatable in real browsers and rest on the PR's browser/e2e suite, which I can't run here.
Bugs (1)
1. ℹ️ the WebKit exclusion of the @supports gate is load-bearing and only verified by browser CI
Location: packages/x-date-pickers/src/PickersTextField/PickersInputBase/PickersInputBase.tsx:373
'@supports (-webkit-app-region: drag)': {
[`.${pickersInputBaseClasses.root}:not(:focus-within) &`]: {
WebkitUserModify: 'read-only',
userSelect: 'none',
},
},The whole "leave WebKit untouched so locator.fill() keeps working" argument depends on WebKit returning false for @supports (-webkit-app-region: drag). -webkit-app-region has WebKit heritage (Dashboard widgets) before Blink/Electron adopted it, so this exclusion is not self-evidently safe across WebKit versions — it's an empirical claim, not a guaranteed one.
Failure scenario: A WebKit build that does match the @supports query would apply WebkitUserModify: read-only to idle sections and break Playwright's fill() actionability check in Safari e2e (the exact regression the gate exists to avoid). Blast radius is contained to CI (the JS mousedown path still focuses sections, so real Safari users are unaffected — focus lifts the rule), but the guard is doing real work with no in-repo assertion that it actually excludes WebKit.
Fix: No code change required; consider a one-line note in the CSS comment that the WebKit exclusion is asserted only by the browser e2e suite (the WebKit fill() cases), so a future reader knows those tests are the regression guard for this assumption.
Tests (1)
1. ℹ️ e2e "should handle change event on the input" was relaxed to dodge a tracked selection race
Location: test/e2e/index.test.ts:696
// ... the subsequent `fill()` then sees `focused === true` in React state
// and skips the re-entrant section-selection that would otherwise
// race with the value setter. Tracked for refactor in
// https://github.com/mui/mui-x/issues/22592.
await input.focus();
await input.fill('02/12/2020');The previous version clicked the section-list root before fill(). The new version focuses the hidden input directly specifically to avoid a re-entrant section-selection vs. value-setter race. This is properly tracked (#22592), so it's not blocking, but it does mean this e2e no longer exercises the click→fill path on the hidden input, and it encodes a known fragility in the new focus-delegation flow.
Failure scenario: The race the comment describes could resurface for real users who click into the field (rather than focusing it programmatically) and immediately type/paste; this test would no longer catch it. Worth confirming #22592 carries a regression test for the click-then-type ordering.
Simplifications (0)
No findings.
Docs (0)
No findings. The inline comments in handleMouseDown, handleClick, and useFieldSectionContainerProps accurately describe the new behavior; the getSelection/rangeCount === 0 guard added in the 'all' branch is a genuine robustness fix over the prior non-null-asserted getRangeAt(0). The new public onMouseDown prop is not surfaced in date-field.json API docs — but neither are onKeyDown/onPaste, so this is consistent, not a gap.
Verdict
Approve — no correctness regressions survived verification; the jsdom field suites pass and the residual risks are browser-only assumptions already covered by the PR's e2e suite. The two ℹ️ notes are heads-ups, not blockers.
🤖 Review generated with Claude Code
The `@supports (-webkit-app-region: drag)` gate relies on WebKit returning false for the query so the `WebkitUserModify: read-only` rule stays Chromium-only (WebKit would otherwise fail Playwright's `fill()` editability check). That exclusion is empirical, not guaranteed across WebKit versions. Add a comment pointing the next reader at the WebKit `fill()` cases in the browser e2e suite as the regression guard, so the assumption isn't silently load-bearing. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
Problem
Screen.Recording.2026-05-04.at.13.28.28.mov
Screen.Recording.2026-05-13.at.17.16.34.mov
Summary
-webkit-user-modify: read-onlywhile the field root is not:focus-within, so Chromium has nothing to delegate focus to. The rule relaxes once any section receives focus.mousedownhandler intercepts clicks on padding, separator gaps, or the area past the last section. It callspreventDefaultand explicitly focuses the section whose horizontal center is closest to the click point (matching native input behavior). This also keeps the multi-input range picker focus-switching tests working, sinceopenPickerclicks on the field's container rather than on a section span.handleClickis simplified: the unconditionalsetFocused(true)at the top is removed (only the'all'/ Ctrl+A branch needs it), and the legacyhasClickedOnASectionbranch is dropped. That branch checkeddomGetters.getRoot().contains(target), which is the section-list root — a descendant of the field root that owns the handler — so it would have beenfalsefor clicks on adornments (calendar / clear buttons) and re-focused the first section. In practice the regression was masked because those buttons callpreventDefault()anduseField.handleRootClickearly-returns onevent.isDefaultPrevented(), but the branch was a latent bug for any custom adornment that didn't preventDefault. The'all'cursor-positioning branch is preserved.-webkit-user-modifyrule is gated to Chromium only via@supports (-webkit-app-region: drag). That property is a Blink/Electron extension WebKit never adopted, so the gate cleanly excludes WebKit (where the bug doesn't exist anyway), keeping Playwright'slocator.fill()working on section spinbuttons without any test-side workaround.Why not toggle
contenteditable?An earlier iteration kept the section spans
contenteditable="false"while idle (or flipped only the active section totrue). It was rejected for two reasons:locator.fill()in every browser, not just WebKit. Playwright's actionability check on a contenteditable element looks at thecontenteditableattribute itself;falsefails the check before the implicit focus step ever runs. The current-webkit-user-modifyrule fails the same check in WebKit, but we can gate it to Chromium via@supportsand leave WebKit untouched. There is no equivalent way to gate thecontenteditableattribute itself per-browser.contenteditableflipped onfocus/mousedown, React re-rendered the section betweenmousedownandmouseup. Chromium then synthesized theclickevent on the common ancestor of the original mousedown target and the current mouseup target (typically theFormControlroot, outside the field'sonClickhandler), which broke picker-opening for multi-input range fields. We hit this regression on38dfa0b02d5before reverting.Side fix
This seems to address the extremely flaky current Safari section selection behavior
Screen.Recording.2026-05-21.at.17.04.46.mov