Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Autocomplete] Fixed useAutocomplete nextFocus #39289

Closed
wants to merge 1 commit into from
Closed

[Autocomplete] Fixed useAutocomplete nextFocus #39289

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 11 additions & 10 deletions packages/mui-base/src/useAutocomplete/useAutocomplete.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,22 +300,23 @@ export function useAutocomplete(props) {
let nextFocus = index;

while (true) {
// Out of range
if (
(direction === 'next' && nextFocus === filteredOptions.length) ||
(direction === 'previous' && nextFocus === -1)
) {
return -1;
}

const option = listboxRef.current.querySelector(`[data-option-index="${nextFocus}"]`);

// Same logic as MenuList.js
const nextFocusDisabled = disabledItemsFocusable
? false
: !option || option.disabled || option.getAttribute('aria-disabled') === 'true';

if ((option && !option.hasAttribute('tabindex')) || nextFocusDisabled) {
if (direction === 'next' && nextFocus === filteredOptions.length) {
if (!nextFocusDisabled) {
return -1;
}
nextFocus = 0;
Copy link
Contributor

@ttlpta ttlpta Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @adamhylander ,
i just wonder why you not return nextFocus immediately like this
image
this way maybe can help reduce 1 loop "while"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ttlpta,

The reason we set nextFocus = 0/nextFocus = filteredOptions.length is because we wish to run the while loop until an option that is not disabled is found. We essentially wrap the focus around to the end/beginning of the list, depending on direction, and continue to look for an option that isn't disabled. I hope this helps. Let me know if you have any more questions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @adamhylander
Thank you for ur answer,

I still dont get your point about we need to run the while loop until an option that is not disabled is found.
Because as your condition, you have already check the direction and the next index of item should be focus. I mean the condition if (direction === 'next' && nextFocus === filteredOptions.length) and else if (direction === 'previous' && nextFocus === -1) is specify to case the disable option is on first/last position of the list. So the next focus index only should be 0, or length of the list.
Btw, Can you send me a test case will be cause of your concern? I have tried some case, but it still works fine when we return imd.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey again @ttlpta

Here is index.tsx from the playground environment I used to test the code if you wanna try your suggestion out yourself:

import * as React from 'react';
import { StyledEngineProvider } from '@mui/material/styles';
import Autocomplete from '@mui/material/Autocomplete'
import TextField from '@mui/material/TextField'
import Stack from '@mui/material/Stack'

export default function Playground() {
  return (
    <StyledEngineProvider injectFirst>
      <Stack spacing={5}>
        <Autocomplete
          id="disabled-options-demo"
          options={[1, 2, 3, 4, 5, 6, 7]}
          getOptionDisabled={(option) => option % 2 !== 0}
          sx={{ width: 300 }}
          renderInput={(params) => (
            <TextField {...params} label="Disabled options" />
          )}
        />
        <Autocomplete
          id="all-enabled"
          options={[1, 2, 3, 4, 5, 6, 7]}
          sx={{ width: 300 }}
          renderInput={(params) => <TextField {...params} label="All enabled" />}
        />
      </Stack>
    </StyledEngineProvider>
  );
}

The if statements if (direction === 'next' && nextFocus === filteredOptions.length) and if (direction === 'previous' && nextFocus === -1) are not used to specify the case of when the disabled option is on first/last position, it is only used to specify that we are at the beginning or end of the list of options and that we wanna move "out of bounds" (i.e. being at filteredOptions.length[0] and pressing the up key, or being at filteredOptions.length[filteredOptions.length] and pressing the down key).

I would be happy to share my thougts/reasoning on Discord as well if you'd like, Nonetheless, I hope my code is a bit clearer now.

Copy link
Contributor

@ttlpta ttlpta Oct 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @adamhylander

Perfect, you are correct, i am too focus on the case the first/ last option has been disabled.

Thank for you answer.

I think you should fix all the cicd step to pass the review from author.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ttlpta

Great! Understanding the logic of nextFocus was confusing for me too. I was wondering what I need to do to pass the last tests? Can you help me with this?

} else if (direction === 'previous' && nextFocus === -1) {
if (!nextFocusDisabled) {
return -1;
}
nextFocus = filteredOptions.length;
} else if ((option && !option.hasAttribute('tabindex')) || nextFocusDisabled) {
// Move to the next element.
nextFocus += direction === 'next' ? 1 : -1;
} else {
Expand Down
Loading