[Select] Fix incorrect selecting of first element #36024

michaldudak · 2023-02-01T21:59:16Z

This PR reverts the implementation from #27299, as it was proven to be buggy (#34731), and fixes the root problem. When MenuList sets the initial highlight (by setting the tabIndex prop), it doesn't take into consideration if the element it wants to focus is indeed focusable. Non-focusable items include ListSubheaders and disabled MenuItems.
Detecting a disabled item is fairly easy - by checking its disabled prop. Skipping the ListSubheaders is more tricky. Checking if child.type === ListSubheader wouldn't work, as the header could be wrapped in a custom component. I ended up adding a static boolean field on a ListSubheader component, called muiSkipListHighlight. If the MenuList finds such a field, it does not consider the component when setting focus. The main drawback of this solution is that such a prop will have to be added to any custom wrapper components. If the PR is accepted, I'll add a proper note to the Select docs.

Fixes #34731

mui-bot · 2023-02-01T22:02:00Z

Netlify deploy preview

docs/data/material/components/selects/selects.md

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against f0d7800

litera · 2023-02-02T07:19:45Z

packages/mui-material/src/ListSubheader/ListSubheader.js

@@ -100,6 +100,8 @@ const ListSubheader = React.forwardRef(function ListSubheader(inProps, ref) {
  );
 });

+ListSubheader.muiSkipListHighlight = true;


There is already a detection mechanism implemented that skips non focusable items in the list that seems to work reliably otherwise keyboard interaction wouldn't work correctly (see grouping in the docs demo). I suggest to use the same detection for this PR instead of introducing a new static prop that adds a fairly important implementation detail to all the implementers? The other one works out of the box without anyone thinking or doing anything about it. It just skips non focusable items. It also works if you disable some otherwise focusable MenuItem.

It doesn't affect all implementers, just those who wrap the ListSubheader in a custom component.

The existing keyboard navigation behavior relies on the DOM nodes already being constructed, as it checks the existence of aria-disabled and tabindex attributes and focuses the right item.

The initial highlight works differently as it has to overwrite the tabIndex prop of the first focusable item. Overwriting the tabindex attribute won't work here, as it would conflict with the prop.

There certainly could be a way to redesign how this logic works, but I'm not eager to rewrite the Select. We've already implemented the SelectUnstyled, which will be used to power the Material UI's Select in the upcoming version. It is free from this bug (and many others).

The tabIndex solution seems much better and universal than this prop.

litera · 2023-02-02T07:21:16Z

packages/mui-material/src/Select/Select.test.js

@@ -393,6 +393,21 @@ describe('<Select />', () => {
    });
  });

+  it('should not have the selectable option selected when inital value provided is empty string on Select with ListSubHeader item', () => {


An additional test should be added with just MenuItem components where the first item is disabled.

There already is a test with the first item disabled around line 567.

mnajdova · 2023-02-03T09:41:27Z

packages/mui-material/src/MenuList/MenuList.js

@@ -232,6 +232,14 @@ const MenuList = React.forwardRef(function MenuList(props, ref) {
        activeItemIndex = index;
      }
    }
+
+    if (activeItemIndex === index && (child.props.disabled || child.type.muiSkipListHighlight)) {


Suggested change

if (activeItemIndex === index && (child.props.disabled || child.type.muiSkipListHighlight)) {

if (activeItemIndex === index && (child.props.disabled || child.props.tabindex < 0)) {

Instead of coming up with new static prop, should we rather teach developers to use the tabindex correctly?

That is a very good suggestion that is also completely independent of the list item components being used. I like it lots more than the muiSkipListHighlight prop.

The MenuList (and dependent compoenents) uses the roving tabindex pattern. Using this pattern, only one of the options (the highlighted one) should have tabindex=0. The rest should have tabindex=-1, so they are not tabbable (navigation between options is done using the arrow keys, not the tab key).

Agree, using the tabindex conflicts with the roving tabindex pattern. @michaldudak have you consider using a data- attribute instead of static field? Something like data-mui-select-skip-tab or something shorter :))

For the record, we've discussed it outside GitHub and agreed to provide both options - the static field and a prop. I'll document both options.

Since we're looking for a prop, not an attribute (as the DOM is not constructed at this point), I chose the same name as the static field: muiSkipOptionHighlight. See the added tests for usage.

…bheader

mnajdova

Looks good. Left couple of comments for the documentation, in my opinion it's more readable when annotation the options. I will leave the final decision to you :)

docs/data/material/components/selects/selects.md

Co-authored-by: Marija Najdova <mnajdova@gmail.com> Signed-off-by: Michał Dudak <michal.dudak@gmail.com>

Co-authored-by: Marija Najdova <mnajdova@gmail.com>

ycmjason · 2023-04-13T16:23:47Z

packages/mui-material/src/MenuList/MenuList.js

+    if (
+      activeItemIndex === index &&
+      (child.props.disabled || child.props.muiSkipListHighlight || child.type.muiSkipListHighlight)
+    ) {
+      activeItemIndex += 1;
+      if (activeItemIndex >= children.length) {
+        // there are no focusable items within the list.
+        activeItemIndex = -1;
+      }
+    }


Hi @michaldudak

I have been looking for solutions to skip items in the menu list. And it looks like this only skips the initial focus when the menu is opened via keyboard. The traversal logic doesn't respect the muiSkipListHighlight prop.

In other words, muiSkipListHighlight in MenuList will only skip the focusVisible state for the initial render, but it is ignored when the user press key up/down to traverse the list items.

Let me know if I should file an issue or how I can help further on fixing this. I might be missing the point of this prop too. So I'd be grateful if you can point out.

Thanks in advance!

@ycmjason Answering on behalf of @michaldudak , the muiSkipListHighlight prop is an internal prop and should be used only if you wrap the ListSubheader in a custom component. We document it's use-case here.

As for skipping the item in a menu list, I guess you need to disable the item, then it should be able to skip the highlight.

michaldudak added 2 commits February 1, 2023 22:45

Revert mui#27299 (but leave its tests)

b365431

Ignore non-focusable items when setting initial highlight

eb56017

michaldudak added bug 🐛 Something doesn't work component: select This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material regression A bug, but worse labels Feb 1, 2023

ZeeshanTamboli mentioned this pull request Feb 2, 2023

[Select] Fix first option as selected when value is empty string #35047

Closed

1 task

litera suggested changes Feb 2, 2023

View reviewed changes

michaldudak requested a review from mnajdova February 3, 2023 09:21

mnajdova reviewed Feb 3, 2023

View reviewed changes

michaldudak added 4 commits February 8, 2023 14:12

Merge branch 'master' into iss/34731-list-subheader

c07f03e

Support muiSkipOptionHighlight prop

905bdda

Merge remote-tracking branch 'upstream/master' into iss/34731-list-su…

7319f17

…bheader

Docs

d94cf02

litera approved these changes Feb 21, 2023

View reviewed changes

mnajdova approved these changes Feb 22, 2023

View reviewed changes

docs/data/material/components/selects/selects.md Outdated Show resolved Hide resolved

docs/data/material/components/selects/selects.md Outdated Show resolved Hide resolved

michaldudak and others added 3 commits February 22, 2023 09:19

Update docs/data/material/components/selects/selects.md

7c6fa53

Co-authored-by: Marija Najdova <mnajdova@gmail.com> Signed-off-by: Michał Dudak <michal.dudak@gmail.com>

Update docs/data/material/components/selects/selects.md

527709b

Co-authored-by: Marija Najdova <mnajdova@gmail.com> Signed-off-by: Michał Dudak <michal.dudak@gmail.com>

Merge branch 'master' into iss/34731-list-subheader

f0d7800

michaldudak enabled auto-merge (squash) February 22, 2023 08:54

michaldudak merged commit e3a37cb into mui:master Feb 22, 2023

michaldudak deleted the iss/34731-list-subheader branch February 22, 2023 10:24

siriwatknp pushed a commit to mnajdova/material-ui that referenced this pull request Feb 24, 2023

[Select] Fix incorrect selecting of first element (mui#36024)

14b3afa

Co-authored-by: Marija Najdova <mnajdova@gmail.com>

ycmjason reviewed Apr 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Select] Fix incorrect selecting of first element #36024

[Select] Fix incorrect selecting of first element #36024

michaldudak commented Feb 1, 2023

mui-bot commented Feb 1, 2023 •

edited

litera Feb 2, 2023 •

edited

michaldudak Feb 2, 2023

litera Feb 3, 2023

litera Feb 2, 2023

michaldudak Feb 2, 2023

mnajdova Feb 3, 2023 •

edited

litera Feb 3, 2023 •

edited

michaldudak Feb 3, 2023

mnajdova Feb 3, 2023

michaldudak Feb 8, 2023 •

edited

mnajdova left a comment

ycmjason Apr 13, 2023

ZeeshanTamboli May 1, 2023

	if (activeItemIndex === index && (child.props.disabled \|\| child.type.muiSkipListHighlight)) {
	if (activeItemIndex === index && (child.props.disabled \|\| child.props.tabindex < 0)) {

[Select] Fix incorrect selecting of first element #36024

[Select] Fix incorrect selecting of first element #36024

Conversation

michaldudak commented Feb 1, 2023

mui-bot commented Feb 1, 2023 • edited

Netlify deploy preview

Bundle size report

litera Feb 2, 2023 • edited

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

mnajdova Feb 3, 2023 • edited

Choose a reason for hiding this comment

litera Feb 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaldudak Feb 8, 2023 • edited

Choose a reason for hiding this comment

mnajdova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mui-bot commented Feb 1, 2023 •

edited

litera Feb 2, 2023 •

edited

mnajdova Feb 3, 2023 •

edited

litera Feb 3, 2023 •

edited

michaldudak Feb 8, 2023 •

edited