[scheduler] Fix keyboard and focus issues around the "+N more" popover - #23312
[scheduler] Fix keyboard and focus issues around the "+N more" popover#23312rita-codes wants to merge 18 commits into
Conversation
The same occurrence is rendered by two triggers whenever the "+N more" popover is open: one in the month cell, one in the popover. Both anchor the editing surface on mount, so when the first one unmounts its cleanup cleared an anchor the other one already owned. The surface then rendered null while the store was still editing, and the dialog vanished right after opening. Release the anchor only when the unmounting trigger still owns it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Take over a vacant anchor: when the trigger owning it unmounts, no sibling re-anchored and the editing surface stopped rendering while the store was still editing. - Return focus to the "+N more" button when the popover closes with the editing surface. It was handing focus back to an event that unmounts with it, leaving focus on the document so the next Tab left the page. - Paint the focus ring inside the chevron shape. `clip-path` clips the outline away, so an event continuing past the day edge took focus without showing anything. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Drop the chevron while focused instead of painting a ring inside it, so the focus outline reads like every other event. This is what the day grid event already does; `clip-path` clips an outline away either way. Move `cursor: pointer` to the card root as well. It sat on the compact and regular variants only, so all-day events, which render `filled` inside the "+N more" popover, showed the default cursor while the same event in the grid or the week view showed a pointer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Matches how programmatic focus is done elsewhere in the repo, so handing focus back cannot scroll the grid. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rita-codes
added
type: bug
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
The key press could land before the dialog handed focus to the title field, so nothing submitted and the dialog stayed open. React 19 won the race locally; React 18 lost it on CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Anchor ownership is now a registry of the mounted triggers for the edited occurrence. Registering only claims a vacant or detached anchor, so the trigger the user activated keeps it instead of the last one in tree order — an occurrence is routinely rendered several times (a multi-day event across agenda days or month week rows, the "+N more" popover repeating the cell), so the previous "last mount wins" opened the dialog next to the wrong instance. Unregistering hands over to a live sibling in the same update: the intermediate `null` unmounted the editing surface and the form came back reseeded, discarding the user's draft. Popover focus restore takes the paper node the transition already passes, reads `activeElement` from its `ownerDocument`, and falls back to the day cell when the opener itself unmounted. The chevron focus reset moves to `arrowClips` so it is not stated twice. Tests now pin what they claim: the anchor never passing through `null`, ownership staying with the activated trigger, the submit actually firing, and the `filled` variant being present for the cursor assertion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The dialog and its focus trap settle on different elements across React versions, so waiting for the title to hold focus timed out on React 18. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UPPER_CASE is for the primitive constants in these suites; the object fixtures around them are camelCase. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rebuilding it by hand restated the id, title and span, so the two could drift apart. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The guard reads as a positive condition: focus is restored from the two places it is about to be lost from. There is no reachable case of another element holding focus at that point, since the popover is modal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Tabbing out of the popover before its exit transition ends is reachable, so the guard is load-bearing: without it the restore drags focus back to the "+N more" button from wherever the user had got to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
flaviendelangle
left a comment
flaviendelangle
left a comment
There was a problem hiding this comment.
PR review
One merge-blocking shadow-DOM focus regression remains. The focused jsdom and Chromium suites passed 62 tests, and PR CI is green, but the new coverage misses several important focus paths.
Bugs (1)
1. 🔴 Shadow-root portals skip focus restoration
Location: packages/x-scheduler/src/internals/components/more-events-popover/MoreEventsPopover.tsx:116
const activeElement = ownerDocument.activeElement;
// ...
paper.contains(activeElement);When the popover is portaled into a shadow root, ownerDocument.activeElement returns the shadow host rather than the focused event inside the popover. The guard therefore incorrectly concludes focus moved elsewhere and skips restoration. A focused DOM reproduction confirmed this behavior.
Failure scenario: Embed the scheduler in a shadow root and configure Material UI portals to use that root. After submitting an event from “+N more,” the focused popover event unmounts and focus remains on the shadow host instead of returning to the trigger or day cell.
Fix: Use the shadow-aware getActiveElement helper from @mui/utils/getActiveElement, and add a regression test with the popover container inside a shadow root.
Tests (3)
1. 🟠 The transition-race test runs after the transition has finished
Location: packages/x-scheduler/src/month-view/tests/MonthView.test.tsx:250
await user.keyboard('{Escape}');
await user.keyboard('{Tab}{Tab}{Tab}{Tab}{Tab}{Tab}{Tab}{Tab}{Tab}{Tab}');test/setupVitest.ts globally sets react-transition-group’s config.disabled = true. Consequently, onExited runs before the Tab presses, so this does not exercise focus moving while the exit transition is active.
Failure scenario: An unconditional onExited restoration would still pass because the test moves focus only after restoration already occurred.
Fix: Enable transitions for this test, move focus to a known external element while the popover remains connected, then await exit and assert focus remains external.
2. 🟠 The disconnected-trigger fallback is untested
Location: packages/x-scheduler/src/month-view/tests/MonthView.test.tsx:217
const onEventsChange = spy();
const { user, popover } = await renderAndOpenPopover({ onEventsChange });The controlled event list is never updated, so the “+N more” button remains connected. Only the anchor.isConnected branch is covered; the newly added grid-cell fallback is not.
Failure scenario: Moving or deleting enough events during submission removes the trigger, but a broken fallback could leave focus on <body> while all tests pass.
Fix: Update the controlled events during submission so the button unmounts, then assert focus moves to the originating grid cell.
3. 🟡 The focus-ring test does not isolate each edge selector
Location: packages/x-scheduler/src/internals/components/event/event-item/EventItem.test.tsx:52
const clipped = popover.querySelector<HTMLElement>('[data-starting-before-edge]');The fixture spans both sides of the displayed day, so it has both edge attributes. Removing either half of the comma-separated selector would still leave the element matching the other half.
Failure scenario: Start-only or end-only events could lose their focus outline without failing this test.
Fix: Parameterize separate start-only and end-only occurrences.
Simplifications (0)
No findings.
Docs (2)
1. 🟠 The focus-restoration guarantee is too broad
Location: docs/data/scheduler/accessibility/accessibility.md:161
When the popover closes, focus returns to the **"X more"** button that opened it...The implementation intentionally preserves focus when it already moved elsewhere and falls back to the day cell when the trigger unmounts.
Failure scenario: Consumers relying on the documented guarantee observe focus on another element or the day cell in supported flows.
Fix: Document that focus returns to the opener only when it remains mounted and focus has not already moved; otherwise it falls back to the day cell.
2. 🟡 The dialog anchoring comment describes the old behavior
Location: packages/x-scheduler/src/internals/components/event-dialog/EventDialog.tsx:162
* Mounts the desktop dialog during the `'edit'` stage, anchored to the element editing started from.The new registry re-anchors the dialog to a surviving trigger when the original unmounts.
Failure scenario: Maintainers infer that the initial trigger remains the anchor throughout editing, contradicting the new ownership behavior.
Fix: Say that the initial trigger is preferred while mounted, with handoff to another registered trigger when necessary.
Verdict
Request changes - shadow-root portal usage can still lose the intended keyboard focus position.
🤖 Review generated with Codex
Make the focus tests exercise what they claim: the transition-race test ran after the exit had already finished, and the day-cell fallback and the per-edge focus ring selectors had no coverage at all. Read the active element past the shadow host, matching what Material UI's own FocusTrap does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011pvJymj8hcaW3yVoCiBkpb
|
The three test findings were right — each one passed with its fix removed. Fixed and re-verified in jsdom and Chromium:
Docs and the Shadow root: applied, but I don't think it's reproducible. I built the scenario and it passed with the broken read, so I instrumented |
2 participants
Closes #23307.
Keyboard and focus problems around the month view's "+N more" popover, found while chasing an intermittent failure of
MonthView.test.tsx > should allow Enter key to activate events in the popoverthat turned out to be a real bug rather than a flaky test. Every fix has a regression test that was checked to fail without it.Anchor ownership
An occurrence is routinely rendered by several
EventEditingTriggers at once, and it takes no popover to get there:getOccurrenceKeyis the event id alone, so a multi-day event has one trigger per day it spans in the agenda, one per week row it crosses in the month view, and the "+N more" popover repeats what the cell already shows. All of them anchored the shared editing surface from a layout effect, which caused two distinct bugs.The dialog closed itself right after opening. The effect cleanup released the anchor unconditionally, so a trigger unmounting after a sibling had re-anchored cleared an anchor it no longer owned.
EventDialogSurfacebails onanchor == null, so the dialog vanished a moment after opening while the store still said editing.The dialog anchored to the wrong instance.
startEditinganchors to the trigger the user activated, but every trigger's effect then overwrote it in tree order, so the last one mounted won. Clicking a three-day event on Wednesday in the agenda opened the dialog next to the Friday instance.Ownership is now a registry of the mounted triggers for the edited occurrence. Registering only claims an anchor that is vacant or detached, so the activated trigger keeps it while it is mounted; unregistering hands over to a live sibling in the same state update. That last part matters on its own: releasing to
nullfirst made React commit an intermediate render with no anchor, which unmounted the editing surface and brought the form back reseeded from the event — silently discarding whatever the user had typed. The trigger is back to a single effect.Focus lost when the dialog is submitted
Closing the dialog with Enter submits the form, which ends editing and closes the popover with it. The dialog hands focus back to the event it was opened from, that event lives in the popover, and the popover then unmounts it — leaving focus on
<body>, so the next Tab lands in the browser chrome instead of the calendar.The popover now restores focus as it exits. It has to happen on the transition's
onExited: any earlier and the dialog'sModalis still mounted, and its focus trap pulls focus straight back. It readsactiveElementfrom the paper'sownerDocumentso a calendar inside a shadow root behaves the same, and falls back to the day cell when editing emptied the day and unmounted the "+N more" button itself.Focus is only taken back from where it was about to be lost — the document, or an element inside the popover that is going away with it. Tabbing out of the popover before its exit transition ends is reachable, and there the restore leaves the user where they got to.
Focus ring and cursor on popover events
An event continuing past the day edge gets a chevron via
clip-path, which clips theoutlineaway with everything else outside the shape, so those events took focus showing no ring at all.DayGridEventalready solved this by dropping the chevron while focused; that rule moves toarrowClipsand both components use it, instead of the fix existing twice with each copy restating its own border radius.EventItemalso setcursor: pointeron thecompactandregularvariants only, so all-day and multi-day events, which renderfilledinside the popover, showed the default cursor while the same event in the day grid or the week view showed a pointer. The pointer moved to the card root, matchingDayGridEvent, which sets it in its base styles for every variant.Changelog
<EventCalendar />no longer closes the editing dialog immediately when an event is opened from the "+N more" popover.The editing dialog now anchors to the event the user activated, instead of another instance of the same event elsewhere in the view.
Edits in progress are no longer discarded when the event the dialog was opened from unmounts.
Submitting the editing dialog from the "+N more" popover returns focus to the "+N more" button instead of dropping it on the document.
Events continuing past the day edge now show a focus ring, and all-day events in the "+N more" popover show a pointer cursor.
I have followed (at least) the PR section of the contributing guide.