[Select]Bug when the first child is a ListSubheader #27299

DouglasPds · 2021-07-15T14:08:24Z

I have followed (at least) the PR section of the contributing guide.

Checking if the child is a ListSubheader to return a different object in React.cloneElement.
And setting the second child as selected when the first child is a ListSubheader, to can use the arrow to navigate the options.
And when an option is selected has no more need to set the second option as selected.

#26790

…tion as selected if the first child is a ListSubheader

mui-pr-bot · 2021-07-15T14:10:32Z

Details of bundle changes (experimental)

Generated by 🚫 dangerJS against 5a970c5

eps1lon

Could you please add some test to check the various branches of the code you introduced? It's not clear what we need all this added code for.

A demo would also be nice for this feature. Especially for manual testing.

…r is the first child

DouglasPds · 2021-07-20T18:05:03Z

Could you please add some test to check the various branches of the code you introduced? It's not clear what we need all this added code for.

A demo would also be nice for this feature. Especially for manual testing.

I will try to explain the code I've made

DouglasPds · 2021-07-20T18:11:45Z

packages/material-ui/src/Select/SelectInput.js

+      return true;
+    };
+
+    if (child.props.value === undefined) {


This returns a different object when the option is a ListSubheader. The role option is because the role listbox of the Menu needs that the children have the role option, but when the option is not a selectable option the aria-selected should be omitted.

DouglasPds · 2021-07-20T18:20:16Z

packages/material-ui/src/Select/SelectInput.js

@@ -385,7 +399,7 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
        }
      },
      role: 'option',
-      selected,
+      selected: index === 1 && arr[0].props.value === undefined ? hasOptionSelected() : selected,


Here when the first child is a ListSubheader it sets the selected true to the second option, to gains the focus when the list of options is open, with the tabindex = 0, to the arrow down works.

DouglasPds · 2021-07-20T18:26:52Z

packages/material-ui/src/Select/SelectInput.js

@@ -369,6 +369,20 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
      foundMatch = true;
    }

+    const hasOptionSelected = () => {


And here, when you select an option don't need more to set the second option as selected true to gain the focus.

DouglasPds · 2021-07-20T18:53:28Z

codesandbox example
Using the grouping example in the docs.

michaldudak

Hi @DouglasPds. I've taken a look at your implementation and found one issue - the problem remains if the second element is also a header or a disabled MenuItem. In such cases, instead of selecting the second element, we'd have to look for the first selectable one.

DouglasPds · 2022-03-02T17:53:19Z

@michaldudak you're right, this creates another problem, I will try this approach.

mui-bot · 2022-03-02T18:40:06Z

Details of bundle changes

Generated by 🚫 dangerJS against 36402c8

…to the new mui-material folder

michaldudak · 2022-03-03T13:17:04Z

@DouglasPds you may be interested to know that we've recently created an unstyled select (https://mui.com/components/selects/#unstyled) that does not suffer from this issue. If you're still having this problem in your application, you may consider evaluating the SelectUnstyled instead.

…t child is a disabled element

… set the focus

DouglasPds · 2022-03-03T14:14:31Z

@michaldudak oh nice, this issue doesn't occur. I will try them. And I find a solution to use with the Select, which I will share here for you to see and evaluate.

DouglasPds · 2022-03-03T14:21:54Z

codesandbox example

michaldudak

Sorry for the delay. This solution does work better, but I think the code may be improved a bit.

michaldudak · 2022-03-22T09:43:58Z

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

+        if (value) {
+          return selected;
+        }


This could be placed before arr.find(...) as it doesn't use its result.

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

michaldudak

This looks good! Thanks for your work!

hbjORbj · 2022-04-13T08:54:29Z

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

@@ -405,7 +425,10 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
        }
      },
      role: 'option',
-      selected,
+      selected:
+        arr[0].props.value === undefined || arr[0].props.disabled === true


@michaldudak @DouglasPds

This assumes that the first child is always a valid React component.
This leads to crash if the first child is of different type. Here is a first report

Yes, I tried here and when the first child is a number, string, NaN, it crashes. The first solution that works was to put optional chaining to check the values after the props. Here where the value is passed to selected and inside the function isFirstSelectableElement.

arr[0].props?.value === undefined || arr[0].props?.disabled === true

You are suggesting this, right? As long as this doesn't break the logic of the fix this PR originally contained, I am fine with that. Will you be willing to ensure that and open a PR with the change?

hbjORbj · 2022-04-13T13:47:09Z

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

+        return selected;
+      }
+      const firstSelectableElement = arr.find(
+        (item) => item.props.value !== undefined && item.props.disabled !== true,


@DouglasPds Let's ensure that item is a valid React component here as well to prevent potential crash.

litera · 2023-02-01T10:45:17Z

@michaldudak How in the world could you approve this PR because it's not resolving the root cause of keyboard interactivity issue but invalidly selects an item that shouldn't be selected in the first place (just to mitigate the keyboard bug)? This code should be reverted and an actual solution should be made. The problem seems to be that the code puts the focus on the wrong item. That code should be changed to resolve the bug and not as it was done here.

DouglasPds added 2 commits July 15, 2021 10:54

Now check if the child is an option with value, and set the second op…

8f97e85

…tion as selected if the first child is a ListSubheader

Merge branch 'next' into bug/listSubheader-not-navigate-with-arrow

247e7fe

eps1lon requested changes Jul 17, 2021

View reviewed changes

eps1lon added component: select This is the name of the generic UI component, not the React module! new feature New feature or request PR: needs test The pull request can't be merged labels Jul 17, 2021

DouglasPds added 2 commits July 20, 2021 14:50

Adding test to check the usability of the arrow when the ListSubheade…

3e2d580

…r is the first child

Correcting test_types to circleci

5a970c5

github-actions bot mentioned this pull request Jul 20, 2021

Update snapshots for pr/27299 eps1lon/mui-scripts-incubator#2796

Open

DouglasPds commented Jul 20, 2021

View reviewed changes

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

eps1lon changed the base branch from next to master September 14, 2021 09:15

mnajdova requested a review from michaldudak November 19, 2021 10:49

michaldudak requested changes Dec 13, 2021

View reviewed changes

Merge with master

52130ee

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 2, 2022

Deleting the file Select.test.js of material-ui, and passing my test …

d028882

…to the new mui-material folder

DouglasPds added 3 commits March 3, 2022 11:02

Returning the GroupedSelect docs to the previous value

5f197e1

Improving the readability of the test name and checking when the firs…

f966267

…t child is a disabled element

Created a function to check which is the first selectable element, to…

408d3a0

… set the focus

Changing also the GroupedSelected typescript file

9a45a10

michaldudak reviewed Mar 22, 2022

View reviewed changes

DouglasPds added 3 commits March 31, 2022 09:45

Merge branch 'master' into bug/listSubheader-not-navigate-with-arrow

26a47e9

Preventing an unnecessary arr.find

efffa1f

Adding a test to check two headers at the top of the Select

36402c8

michaldudak approved these changes Apr 1, 2022

View reviewed changes

michaldudak removed the PR: needs test The pull request can't be merged label Apr 6, 2022

michaldudak merged commit a8db12e into mui:master Apr 11, 2022

wadehammes mentioned this pull request Apr 11, 2022

[Select] Throwing a value undefined error in latest 5.6.1 #32253

Closed

2 tasks

hbjORbj reviewed Apr 13, 2022

View reviewed changes

This was referenced Oct 14, 2022

[Select] Keyboard navigation does not work if the first child is a ListSubheader #26790

Closed

[Select] First option chosen as selected when grouping with ListSubheader #34731

Closed

ZeeshanTamboli mentioned this pull request Nov 16, 2022

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

Closed

1 task

litera mentioned this pull request Feb 1, 2023

Select dropdown menu selects the first non-disabled element when no value is selected #36018

Closed

2 tasks

michaldudak added a commit to michaldudak/material-ui that referenced this pull request Feb 1, 2023

Revert mui#27299 (but leave its tests)

b365431

michaldudak mentioned this pull request Feb 1, 2023

[Select] Fix incorrect selecting of first element #36024

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Select]Bug when the first child is a ListSubheader #27299

[Select]Bug when the first child is a ListSubheader #27299

DouglasPds commented Jul 15, 2021 •

edited

Loading

mui-pr-bot commented Jul 15, 2021 •

edited

Loading

eps1lon left a comment •

edited

Loading

DouglasPds commented Jul 20, 2021

DouglasPds Jul 20, 2021

DouglasPds Jul 20, 2021

DouglasPds Jul 20, 2021

DouglasPds commented Jul 20, 2021

michaldudak left a comment

DouglasPds commented Mar 2, 2022

mui-bot commented Mar 2, 2022 •

edited

Loading

michaldudak commented Mar 3, 2022

DouglasPds commented Mar 3, 2022

DouglasPds commented Mar 3, 2022

michaldudak left a comment

michaldudak Mar 22, 2022

michaldudak left a comment

hbjORbj Apr 13, 2022

DouglasPds Apr 13, 2022

hbjORbj Apr 13, 2022

hbjORbj Apr 13, 2022

litera commented Feb 1, 2023 •

edited

Loading

[Select]Bug when the first child is a ListSubheader #27299

[Select]Bug when the first child is a ListSubheader #27299

Conversation

DouglasPds commented Jul 15, 2021 • edited Loading

mui-pr-bot commented Jul 15, 2021 • edited Loading

eps1lon left a comment • edited Loading

Choose a reason for hiding this comment

DouglasPds commented Jul 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DouglasPds commented Jul 20, 2021

michaldudak left a comment

Choose a reason for hiding this comment

DouglasPds commented Mar 2, 2022

mui-bot commented Mar 2, 2022 • edited Loading

michaldudak commented Mar 3, 2022

DouglasPds commented Mar 3, 2022

DouglasPds commented Mar 3, 2022

michaldudak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaldudak 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

litera commented Feb 1, 2023 • edited Loading

DouglasPds commented Jul 15, 2021 •

edited

Loading

mui-pr-bot commented Jul 15, 2021 •

edited

Loading

eps1lon left a comment •

edited

Loading

mui-bot commented Mar 2, 2022 •

edited

Loading

litera commented Feb 1, 2023 •

edited

Loading