mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.07k stars 32.05k forks source link

[Select] moving focus with keyboard ArrowUp or ArrowDown is not working when [disablePortal] is set #34218

Open teetotum opened 1 year ago

teetotum commented 1 year ago

Duplicates

Latest version

Current behavior 😯

When I click into a <Select> (with disablePortal) the dropdown opens, but ArrowUp and ArrowDown will not move the keyboard focus to any of the items. Addendum: I verified that the autoFocus property does not influence the behavior. It does not matter whether autoFocus is set or not.

Expected behavior 🤔

Without disablePortal the behavior is as expected: keyboard ArrowUp and ArrowDown keys will move the focus to the items and between the items.

Steps to reproduce 🕹

I prepared a codesandbox that exhibits the behavior https://codesandbox.io/s/pensive-cache-xk9js4?file=/src/App.tsx

The relevant usage to elicit the bug is:

<Select MenuProps={{ disablePortal: true }}>
    <MenuItem value="foo">{"foo"}</MenuItem>
</Select>
  1. click into the select
  2. use ArrowDown to move focus into the dropdown. but nothing happens.

Observation: When disablePortal is set the dropdown paper will receive focus when the dropdown opens. This is not the case otherwise.

Context 🔦

No response

Your environment 🌎

No response

teetotum commented 1 year ago

I've developed a workaround that anyone can use until this is fixed. I listen for the native bubbling DOM event focusin and check if the paper has focus; if this is the case I just focus the first item. Done.

import { useFocusIn } from './useDOMEvent';

// ...

const { ref } = useFocusIn((e: any) => {
    if (e.target.matches('.MuiPaper-root')) {
      const list = e.target.querySelector('.MuiList-root');
      list?.firstElementChild?.focus();
    }
  });

<Select
    {...props}
    ref={ref}
>
    {options}
</Select>

and the custom hook

import { useEffect, useState } from 'react';

export const useDOMEvent = (
  type: keyof GlobalEventHandlersEventMap,
  listener: EventListenerOrEventListenerObject,
  options?: AddEventListenerOptions
) => {
  const [target, setTarget] = useState<any>();
  const { capture, once, passive, signal } = { ...options };

  useEffect(() => {
    if (target) {
      target.addEventListener(type, listener, { capture, once, passive, signal });
      return () => target.removeEventListener(type, listener, { capture });
      // be aware that EventListenerOptions used for 'remove' do not mirror AddEventListenerOptions
    }
  }, [target, type, listener, capture, once, passive, signal]);

  return { ref: setTarget };
};

export const useFocusIn = (listener: EventListenerOrEventListenerObject, options?: AddEventListenerOptions) =>
  useDOMEvent('focusin', listener, options);
export const useFocusOut = (listener: EventListenerOrEventListenerObject, options?: AddEventListenerOptions) =>
  useDOMEvent('focusout', listener, options);
michaldudak commented 1 year ago

Thanks for reporting this issue and the workaround. Would you like to investigate the root cause of this in our Select component?

teetotum commented 1 year ago

I would like to give it a try. I can't say when I will find the time though. Maybe next friday.

teetotum commented 1 year ago

I forked and cloned the repo today. Currently trying to write a failing testcase for the bug. working in packages\mui-material\src\Select\Select.test.js and trying to modify a duplicate of this existing testcase, which seems to be close to what I'm attempting:

it('should focus list if no selection', () => {
    const { getByRole } = render(<Select value="" autoFocus />);

    fireEvent.mouseDown(getByRole('button'));

    // TODO not matching WAI-ARIA authoring practices. It should focus the first (or selected) item.
    expect(getByRole('listbox')).toHaveFocus();
});

Update: I have to declare defeat for today. Didn't succeed in creating a failing testcase. I went down the rabbit hole from the Select to the SelectInput -> Menu -> Popover -> Modal -> ModalUnstyled -> Portal and TrapFocus. My guess at the moment is that something in the TrapFocus might be interfering: it listens for native bubbling focusin events and redistributes focus if it catches any. But the DOM structure and therefore the way native bubbling events will travel up the tree is different depending on whether a portal is used or not. So the root cause might be somewhere there. But that's just speculation for now. I have an idea how to tackle it further but will need a few carefree hours; maybe sunday. I have to do this in my spare time and that's scarce.

Update: no progress, had to work on other things. I want to try this idea to figure out what happens to focus.

GLObus303 commented 1 year ago

Hey, it seems it could also fix the hinting when the user is typing during the opened list. Which also does not work with disablePortal: true. Both are quite impactful for the accessibility of the component

By "hinting" I mean this functionality:

CleanShot 2022-12-05 at 13 42 44

litera commented 1 year ago

When anyone tackles this issue keep in mind that the first item in the list may not be selectable (i.e. disabled or ListSubhead). Focus should be put on the first selectable item in the list.

litera commented 1 year ago

Some more behaviour details. Here is a working example for testing.

When portal is enabled (default), keyboard interaction seems to work as expected. Either when opening the dropdown using the keyboard (focus on the select pushing the ArrowDown/UpArrow/Enter/Space key) or opening it using a mouse (click the select field). After the dropdown opens, the list is fully navigatable by keyboard.

But when the portal is disabled, then opening the dropdown with a mouse completely looses keyboard interaction. Mind that keyboard interaction still works if you open the dropdown using the keyboard. SO if you open the dropdown using the keyboard, you can freely use the keyboard.

yonimar87 commented 1 year ago

I assume this still isn't fixed? I've got the same problem at the moment. I'll try do the workaround meanwhile and see if that works for me