[pickers] Avoid stealing focus on click away #13434

LukasTy · 2024-06-10T10:42:17Z

Fixes #13332
Fixes #14801

Fix a related issue with Range Pickers: the stopPropagation calls were preventing click away listener from working properly
Fix a related issue with Pickers using Digital Clocks: The .focus() calls in effects were stealing the focus before closing the popper

Preview: https://deploy-preview-13434--material-ui-x.netlify.app/x/react-date-pickers/

mui-bot · 2024-06-10T10:45:27Z

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

Generated by 🚫 dangerJS against c393b79

…r-behavior

LukasTy · 2024-06-10T12:35:38Z

packages/x-date-pickers-pro/src/internals/hooks/useEnrichedRangePickerFieldProps.ts

@@ -195,15 +195,15 @@ const useMultiInputFieldSlotProps = <
  }, [rangePosition, open, currentView, startFieldRef, endFieldRef]);

  const openRangeStartSelection: React.UIEventHandler = (event) => {
-    event.stopPropagation();
+    event.preventDefault();


These fixes prevent the following issue:

Screen.Recording.2024-06-10.at.13.43.10.mov

The regular Picker is not closed when opening a range picker

Could we add a comment on top of this preventDefault to explain why it's here? In general, I think we should aim to justify each, they (preventDefault and especially stopPropagation) tend to be quickly food guns.

LukasTy · 2024-06-10T12:38:11Z

packages/x-date-pickers/src/DigitalClock/DigitalClock.tsx

@@ -219,7 +220,8 @@ export const DigitalClock = React.forwardRef(function DigitalClock<TDate extends
      return;
    }
    const offsetTop = activeItem.offsetTop;
-    if (autoFocus || !!focusedView) {
+    if ((autoFocus || !!focusedView) && activeItem !== lastActiveRef.current) {


Fix the issue of stealing the focus when closing a picker with DigitalClock:

Screen.Recording.2024-06-10.at.15.37.51.mov

LukasTy · 2024-06-10T12:39:05Z

packages/x-date-pickers/src/MultiSectionDigitalClock/MultiSectionDigitalClockSection.tsx

@@ -172,7 +173,7 @@ export const MultiSectionDigitalClockSection = React.forwardRef(
      const activeItem = containerRef.current.querySelector<HTMLElement>(
        '[role="option"][tabindex="0"], [role="option"][aria-selected="true"]',
      );
-      if (active && autoFocus && activeItem) {
+      if (activeItem && active && autoFocus && activeItem !== previousActive.current) {


Fix the issue of stealing the focus when closing a picker with MultiSectionDigitalClock:

Screen.Recording.2024-06-10.at.15.38.47.mov

LukasTy · 2024-06-10T12:41:00Z

packages/x-date-pickers/src/internals/components/PickersPopper.tsx

@@ -360,6 +360,16 @@ export function PickersPopper(inProps: PickerPopperProps) {
  }, [onDismiss, open]);

  const lastFocusedElementRef = React.useRef<Element | null>(null);
+
+  const handleClickAway: OnClickAway = useEventCallback(() => {
+    lastFocusedElementRef.current = null;


Fix the root cause why pickers where returning the focus back to the open button after being clicked away:

Screen.Recording.2024-06-10.at.15.39.53.mov

michelengelen

LGTM! 🚢

flaviendelangle

Looks solid!

oliviertassinari

In HEAD, we restore the focus back to the trigger. e.g. https://mui.com/material-ui/react-menu/

Screen.Recording.2024-06-10.at.18.37.04.mov

It doesn't do it anymore:

Screen.Recording.2024-06-10.at.18.34.53.mov

A bug?

oliviertassinari · 2024-06-10T16:31:02Z

packages/x-date-pickers-pro/src/internals/hooks/useEnrichedRangePickerFieldProps.ts

@@ -195,15 +195,15 @@ const useMultiInputFieldSlotProps = <
  }, [rangePosition, open, currentView, startFieldRef, endFieldRef]);

  const openRangeStartSelection: React.UIEventHandler = (event) => {
-    event.stopPropagation();
+    event.preventDefault();


Could we add a comment on top of this preventDefault to explain why it's here? In general, I think we should aim to justify each, they (preventDefault and especially stopPropagation) tend to be quickly food guns.

oliviertassinari · 2024-06-10T16:33:10Z

test/e2e/index.test.ts

@@ -777,6 +777,76 @@ async function initializeEnvironment(
          // check that selecting a day doesn't change the day text
          expect(await day11.textContent()).to.equal('11');
        });
+
+        describe('Focus behavior', () => {


I suspect a regular karma test would be enough here (so better since faster to run)? We are doing relatively simple events.

If I'm not mistaken, I tried writing unit tests initially, but failed to make them work.
I didn't try skipping them in jest though. I'll look into that.

I've tried karma tests and they have the same problem...
Maybe it's some sort of a race condition with timeouts, but on those tests, the document.activeElement is resolved/set on the button element. 🙈

LukasTy · 2024-06-11T07:56:55Z

In HEAD, we restore the focus back to the trigger. e.g. https://mui.com/material-ui/react-menu/
It doesn't do it anymore:
A bug?

Thank you for flagging this @oliviertassinari! 🙏
I've used the behavior in here as an example.
There is a clear issue that closing the picker with an Esc key no longer returns the focus to the button as in the referred example.
Given the W3C-ARIA Menu example you could think that our Menu implementation is a bit too intrusive with moving the focus back to the button even on click away.
I'll look into possible options a bit more in-depth. 😉

LukasTy · 2024-06-11T13:28:09Z

@oliviertassinari I've explored other solutions and here is the rundown:

Angular Material and React Spectrum behave like we currently do (click away returns focus back to the opening button), but they both are using the dialog approach, where all the rest of the page becomes non-interactive—under a fullscreen backdrop, in our case—we don't do this.
Native date input element behaves like in this PR implementation in both Chrome and Firefox—Esc returns the focus to the opening button, but clicking away doesn't. That seems to be inline with W3C-ARIA example guidelines.
Next UI seems to have the same behavior like we currently do, no backdrop and proper click-away handling. If that's a role model we want to follow, when I need to "get back to the drawing board".

Given these arguments, I think that the behavioral solution in this PR is valid and more correct than the current one.
Unless we want to have a full-screen backdrop visually "disabling" page interactiveness, in which case we need more changes.

WDYT @flaviendelangle @arthurbalduini @joserodolfofreitas?

Do you think that adding a fullscreen backdrop makes sense in this case?
Or do you feel our approach closer resembling a native date input behavior makes sense to you?
Or do we want to strive for making no behavioral changes and just fixing the underlying problem—Next UI approach?

oliviertassinari · 2024-06-11T14:05:52Z

@LukasTy Thanks for the dive. In my view, the behavior before feels better (Angular Material and React Spectrum ). At least, the things that are important properties we want seem to be:

Tabs go to the next form input after closing a date picker, on all platforms
the screen readers announce the right focus element, it's not lost

On the solution, I don't think that 1. (using a backdrop) is that much correlated to the root of the problem, this feels like taking a different path (which works, true). Now, it's not the end of the world, maybe it can work ok like this.

At the root, there might be nothing to change in MUI X (I assume what we do here is to trigger ). The problem might really be with the FocusTrap logic. Maybe the logic that restores the focus should give up if that focus is already set outside of its boundary. I mean, this would make a lot of sense to me. It makes me think of mui/material-ui#35307. For this, we should involve the FocusTrap's ower? https://www.notion.so/mui-org/0c8156b0b1634ea5bfd903dd005ec1cf?v=89ea8d7b7e104bf8b3bb68a9edca5e16&p=fbe1aed295f748c19576d024e9a48c62&pm=s

jbblanchet · 2024-09-30T17:37:03Z

Hi! Is this fix still being considered? I just spent the better part of last week migrating from react-dates to this date picker, but that bug is a deal breaker for us. I haven't see any comments since June on this issue, I was wondering if maybe it had been forgotten?

Thanks

github-actions · 2024-10-10T11:45:06Z

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

LukasTy · 2024-10-10T12:34:48Z

Hi! Is this fix still being considered? I just spent the better part of last week migrating from react-dates to this date picker, but that bug is a deal breaker for us. I haven't see any comments since June on this issue, I was wondering if maybe it had been forgotten?

Hi, thank you for reminding us about this problem. 🙏
It's been parked, because of higher-priority topics for now, but we plan to finalize this PR in the very near future. 👌

BrunMartins · 2025-01-31T07:07:23Z

Hello! Was there any progress on this issue in the meanwhile? If not, can the community help in anyway?
Thanks!

LukasTy added 4 commits June 4, 2024 18:26

[pickers] Fix to avoid refocusing previous item on click away

23f53da

Avoid stopPropagation to fix clickAway behavior on range picker

da055ed

Add all Picker components focus assertion tests

e8e1b9a

Avoid stealing focus when blurring digital clocks

bcc471d

LukasTy added bug 🐛 Something doesn't work component: pickers This is the name of the generic UI component, not the React module! needs cherry-pick The PR should be cherry-picked to master after merge labels Jun 10, 2024

LukasTy self-assigned this Jun 10, 2024

Merge remote-tracking branch 'upstream/master' into fix-pickers-poppe…

176c30d

…r-behavior

LukasTy commented Jun 10, 2024

View reviewed changes

Use nodeName to make jest happy

56fc153

LukasTy requested review from oliviertassinari and a team June 10, 2024 13:26

michelengelen approved these changes Jun 10, 2024

View reviewed changes

flaviendelangle approved these changes Jun 10, 2024

View reviewed changes

oliviertassinari reviewed Jun 10, 2024

View reviewed changes

Remove preventDefault calls as they do not seem required

c393b79

michelengelen mentioned this pull request Jul 23, 2024

[pickers] Inconsistent design of DatePicker and DateRangePicker #13941

Closed

michelengelen mentioned this pull request Oct 2, 2024

[pickers] Dropdown Anchor Movement Issue in Date/Time Pickers #14801

Open

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pickers] Avoid stealing focus on click away #13434

[pickers] Avoid stealing focus on click away #13434

LukasTy commented Jun 10, 2024 •

edited by michelengelen

Loading

mui-bot commented Jun 10, 2024 •

edited

Loading

LukasTy Jun 10, 2024

oliviertassinari Jun 10, 2024

LukasTy Jun 10, 2024

LukasTy Jun 10, 2024

LukasTy Jun 10, 2024

michelengelen left a comment

flaviendelangle left a comment

oliviertassinari left a comment

oliviertassinari Jun 10, 2024

oliviertassinari Jun 10, 2024

LukasTy Jun 11, 2024

LukasTy Jun 13, 2024

LukasTy commented Jun 11, 2024

LukasTy commented Jun 11, 2024 •

edited

Loading

oliviertassinari commented Jun 11, 2024 •

edited

Loading

jbblanchet commented Sep 30, 2024

github-actions bot commented Oct 10, 2024

LukasTy commented Oct 10, 2024

BrunMartins commented Jan 31, 2025

[pickers] Avoid stealing focus on click away #13434

Are you sure you want to change the base?

[pickers] Avoid stealing focus on click away #13434

Conversation

LukasTy commented Jun 10, 2024 • edited by michelengelen Loading

mui-bot commented Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michelengelen left a comment

Choose a reason for hiding this comment

flaviendelangle left a comment

Choose a reason for hiding this comment

oliviertassinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasTy commented Jun 11, 2024

LukasTy commented Jun 11, 2024 • edited Loading

oliviertassinari commented Jun 11, 2024 • edited Loading

jbblanchet commented Sep 30, 2024

github-actions bot commented Oct 10, 2024

LukasTy commented Oct 10, 2024

BrunMartins commented Jan 31, 2025

LukasTy commented Jun 10, 2024 •

edited by michelengelen

Loading

mui-bot commented Jun 10, 2024 •

edited

Loading

LukasTy commented Jun 11, 2024 •

edited

Loading

oliviertassinari commented Jun 11, 2024 •

edited

Loading