Skip to content

[fields] Avoid focusing the field on blank space clicks#22285

Merged
LukasTy merged 55 commits into
mui:masterfrom
LukasTy:claude/musing-albattani-b80c2f
Jun 23, 2026
Merged

[fields] Avoid focusing the field on blank space clicks#22285
LukasTy merged 55 commits into
mui:masterfrom
LukasTy:claude/musing-albattani-b80c2f

Conversation

@LukasTy

@LukasTy LukasTy commented May 1, 2026

Copy link
Copy Markdown
Member

Problem

Screen.Recording.2026-05-04.at.13.28.28.mov
Screen.Recording.2026-05-13.at.17.16.34.mov

Summary

  • Fix a Chromium focus-delegation quirk where clicking on a non-contenteditable ancestor of the field (for example, a flex wrapper with extra horizontal space) was focusing the nearest contenteditable section span. The fix marks section content as -webkit-user-modify: read-only while the field root is not :focus-within, so Chromium has nothing to delegate focus to. The rule relaxes once any section receives focus.
  • Inside the field root, a new mousedown handler intercepts clicks on padding, separator gaps, or the area past the last section. It calls preventDefault and 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, since openPicker clicks on the field's container rather than on a section span.
  • handleClick is simplified: the unconditional setFocused(true) at the top is removed (only the 'all' / Ctrl+A branch needs it), and the legacy hasClickedOnASection branch is dropped. That branch checked domGetters.getRoot().contains(target), which is the section-list root — a descendant of the field root that owns the handler — so it would have been false for clicks on adornments (calendar / clear buttons) and re-focused the first section. In practice the regression was masked because those buttons call preventDefault() and useField.handleRootClick early-returns on event.isDefaultPrevented(), but the branch was a latent bug for any custom adornment that didn't preventDefault. The 'all' cursor-positioning branch is preserved.
  • The -webkit-user-modify rule 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's locator.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 to true). It was rejected for two reasons:

  1. It would break locator.fill() in every browser, not just WebKit. Playwright's actionability check on a contenteditable element looks at the contenteditable attribute itself; false fails the check before the implicit focus step ever runs. The current -webkit-user-modify rule fails the same check in WebKit, but we can gate it to Chromium via @supports and leave WebKit untouched. There is no equivalent way to gate the contenteditable attribute itself per-browser.
  2. It caused mid-click target shifts. If contenteditable flipped on focus / mousedown, React re-rendered the section between mousedown and mouseup. Chromium then synthesized the click event on the common ancestor of the original mousedown target and the current mouseup target (typically the FormControl root, outside the field's onClick handler), which broke picker-opening for multi-input range fields. We hit this regression on 38dfa0b02d5 before 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

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]>
@LukasTy LukasTy added type: bug It doesn't behave as expected. scope: pickers Changes related to the date/time pickers. feature: Keyboard editing Related to the pickers keyboard edition needs cherry-pick The PR should be cherry-picked to master after merge. v8.x labels May 1, 2026
@LukasTy LukasTy self-assigned this May 1, 2026
@code-infra-dashboard

code-infra-dashboard Bot commented May 1, 2026

Copy link
Copy Markdown

Deploy preview

https://deploy-preview-22285--material-ui-x.netlify.app/

Bundle size

Bundle Parsed size Gzip size
@mui/x-data-grid 0B(0.00%) 0B(0.00%)
@mui/x-data-grid-pro 0B(0.00%) 0B(0.00%)
@mui/x-data-grid-premium 0B(0.00%) 0B(0.00%)
@mui/x-charts 0B(0.00%) 0B(0.00%)
@mui/x-charts-pro 0B(0.00%) 0B(0.00%)
@mui/x-charts-premium 0B(0.00%) 0B(0.00%)
@mui/x-date-pickers 🔺+895B(+0.45%) 🔺+293B(+0.52%)
@mui/x-date-pickers-pro 🔺+889B(+0.33%) 🔺+331B(+0.43%)
@mui/x-tree-view 0B(0.00%) 0B(0.00%)
@mui/x-tree-view-pro 0B(0.00%) 0B(0.00%)
@mui/x-license 0B(0.00%) 0B(0.00%)

Details of bundle changes


Check out the code infra dashboard for more information about this PR.

LukasTy and others added 22 commits May 1, 2026 17:51
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]>
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]>
@github-actions github-actions Bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jun 3, 2026
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]>
@LukasTy

LukasTy commented Jun 3, 2026

Copy link
Copy Markdown
Member Author

Thanks @flaviendelangle, addressed in 3d541257132. Took most of the suggestions, pushed back on a few. Walking through each:

Important issues

#1handleRootMouseDown's event.isDefaultPrevented() gate

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 preventDefault is the DOM-level "this event is handled" signal that the clear/open buttons rely on (their handlers preventDefault, then propagation lets handleRootMouseDown see isDefaultPrevented() and bail). Inline userland preventDefault on onMouseDown is the everyday "browser, don't do native default things" call — text-selection suppression, etc. — and consumers do it without meaning to disable the field's section selection. Honoring it would silently break the field for anyone who calls preventDefault for unrelated reasons. The previous defaultMuiPrevented discussion already settled this — adding it back would re-open the same trap. The new test (should not select any section when a capture-phase parent calls preventDefault on mousedown) locks the contract so a future refactor can't drift.

Also tightened the comment to match (#9 below).

#2handleClick's !focused branch dropping setSelectedSections(startIndex)

Restored the defensive fallback when parsedSelectedSections == null && !focused. Real for the path you flagged: programmatic element.click(), AT activations, synthetic test setups. Kept the comment honest about when the branch actually matters.

#3 — Closest-section / separator tests not discriminating

Updated the separator test to click the / after Day (data-sectionindex=1) instead of after Month (data-sectionindex=0). Under JSDOM's zero-sized rects, the closest-by-distance fallback always picks 0, so the new test target is the only one that meaningfully distinguishes the [data-sectionindex] lookup (Day) from the fallback (Month). Dropping the lookup now fails the assertion.

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.

#4event.button !== 0 untested

Added.

#5 — Userland onMouseDown forwarding untested

Added.

#6 — CSS gate coverage one section-layout deep

Added selection.DateTimeField.test.tsx mirroring the DateField/TimeField outer-wrapper flex test against the longer MM/DD/YYYY hh:mm aa layout. Range fields share the same PickersInputBase and the same gate, so adding coverage for SingleInputDateRangeField would be redundant — the structural difference is in the field wrapper, not in the gated section content.

#7Number(sectionElement.dataset.sectionindex) → NaN not guarded

Added Number.isInteger guard. Defense in depth; matches your reasoning.

Comment issues

#8 — handleClick !focused comment — rewritten to explain when the branch matters (programmatic element.click() without preceding mousedown) instead of the vague "driven by focus".

#9 — handleRootMouseDown comment — rewritten to be explicit about the capture-phase / userland-inline asymmetry and why userland inline preventDefault is intentionally not honored.

#10findClosestSectionIndexToPoint JSDoc — restated as "no role="spinbutton" descendants" to match the actual query.

#11 — Playwright fill() claim in the CSS-gate comment

Pushing back on this one. The comment already explains why the gate exists (Chromium-only quirk + WebKit fill() editability check break) in terms of the observable behavior, without naming a Playwright issue number. If Playwright fixes their behavior upstream, the gate gracefully degrades to a no-op — there's no broken state to clean up later. Linking an upstream issue creates a stale-link maintenance burden without removing actual work. I'd rather leave the rationale anchored to the actual constraint (the CSS property semantics) than to a tracking issue that may close without changing our code at all.

Suggestions

  • Reorder cheap checks last: marginal, didn't touch. Readability stays clearer with the "is this even applicable" disabled/ready/all checks grouped together.
  • Redundant onSelectedSectionsChange: added the if (parsedIndex !== parsedSelectedSections) guard. Same fix you suggested.
  • Tie-breaking & RTL: real concern, deferred to a follow-up — needs a <div dir="rtl"> browser test setup that the suite doesn't currently have, and the RTL layout discussion is broader than just this function. Not blocking this PR.
  • 'all' + getSelection() failure fallback: pushing back. The 0-tick containerClickTimeout runs after the user has clicked somewhere — if rangeCount === 0 at that moment, the user has lost their selection (focus moved out, layout shifted, whatever). Preserving "all" mode there would leave the field in a state the user didn't choose; falling back to startIndex is the closest thing to "no selection state is recoverable, restart from a known position". A console.warn would be noisy in dev for an edge case that's already a silent recovery — diagnostic value isn't worth the constant warning.
  • 'all'-preservation test omits the click: added.
  • e2e input.focus() workaround / it.todo for the original variant: not adding. The hidden-input click→fill pattern is tracked in [fields] Re-evaluate hidden-input focus delegation (v10 breaking change) #22592; once that lands we'll restore the no-prior-action variant. it.todo would just duplicate the issue reference. The current comment on the e2e test already links [fields] Re-evaluate hidden-input focus delegation (v10 breaking change) #22592.
  • Left/right/equidistant boundary tests: deferred. The current browser test exercises the math meaningfully (Day-closer-than-Month-or-Year). Boundary cases would be nice-to-have but aren't a known regression path; would file follow-up if it ever surfaces in practice.

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

@github-actions github-actions Bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jun 11, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions Bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jun 15, 2026
@LukasTy
LukasTy requested a review from flaviendelangle June 15, 2026 14:19

@flaviendelangle flaviendelangle left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@flaviendelangle

Copy link
Copy Markdown
Member

Looks like we just have the onSelectedSectionsChange that does not satisfy Claude 😆

LukasTy and others added 4 commits June 19, 2026 20:56
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]>
@LukasTy
LukasTy force-pushed the claude/musing-albattani-b80c2f branch from 6ae02a8 to 39f5996 Compare June 22, 2026 11:01
@LukasTy

LukasTy commented Jun 22, 2026

Copy link
Copy Markdown
Member Author

Thanks @flaviendelangle - both valid. Fixed in 6581554 (dedup) and 39f5996 (stabilization).

Finding 1 - extra onSelectedSectionsChange call. Reproduced the 3x vs master's 2x. Took your onClick guard, but keeping the existing mousedown guard too dropped the already-selected click to 0 (master fires 1), so I added the onClick guard and removed the redundant mousedown one - exact master parity:

gesture master PR before PR now
new section 2 3 2
already-selected 1 1 1

Regression test added; non-pointer path preserved. Perf follow-up (39f5996): one stable useEventCallback handler reading the index from data-sectionindex, so nothing sits in a dep array.

Finding 2 - test comment. Right, the 'all' branch's setTimeout(0) isn't flushed by fireEvent, so the assertion is synchronous-only. Softened the comment; left the transient-selection fallback for its own test.

@LukasTy
LukasTy requested a review from flaviendelangle June 22, 2026 12:19
@alexfauquette
alexfauquette removed their request for review June 22, 2026 12:38

@flaviendelangle flaviendelangle left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (useFieldRootPropsuseFieldPickersInputBasesyncSelectionToDOM + 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]>
@LukasTy
LukasTy enabled auto-merge (squash) June 23, 2026 08:05
@LukasTy
LukasTy merged commit 3737ff0 into mui:master Jun 23, 2026
21 checks passed
@LukasTy
LukasTy deleted the claude/musing-albattani-b80c2f branch June 23, 2026 10:14
mbrookes pushed a commit to mbrookes/mui-x that referenced this pull request Jun 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: Keyboard editing Related to the pickers keyboard edition scope: pickers Changes related to the date/time pickers. type: bug It doesn't behave as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants