[Autocomplete] Fixed useAutocomplete nextFocus

[adamhylander]

Fixed validOptionIndex method in useAutocomplete.js to account for disabled edge cases.

Closes #39281

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

[mui-bot]

Netlify deploy preview

https://deploy-preview-39289--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 3c47966

[ttlpta]

Hi @adamhylander ,
i just wonder why you not return nextFocus immediately like this

this way maybe can help reduce 1 loop "while"

[adamhylander]

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.

[ttlpta]

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.

[adamhylander]

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.

[ttlpta]

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.

[adamhylander]

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?

[adamhylander]

Hey, @mj12albert

I don't fully understand how to pass the last checks. Can you help me with this?

[mj12albert]

@adamhylander It seems under some conditions (not 100% sure but it seems whenever autoHighlight is set + using any keyboard input), an infinite loop is created which is causing some of Joy UI's Autocomplete tests to hang (internally it uses Base's useAutocomplete)

It can be reproed in a sandbox, but I couldn't figure out how to get the sandbox to restart after it hung the first time 😆

You can repro it in the local playground like this:

import * as React from 'react';
import { CssVarsProvider } from '@mui/joy';
import Autocomplete from '@mui/joy/Autocomplete';

export default function Playground() {
  return (
    <CssVarsProvider>
      <Autocomplete
        autoHighlight
        freeSolo
        placeholder="Combo box"
        options={['one', 'two']}
        sx={{ width: 300 }}
        open
      />
    </CssVarsProvider>
  );
}

1. Type oo into the input
2. It will hang after inputting the first o

The failing tests relate to prop: autoHighlight:

1. should set the focus on the first item
2. should set the focus on the first item when possible
3. should work with filterSelectedOptions too

[adamhylander]

@adamhylander It seems under some conditions (not 100% sure but it seems whenever autoHighlight is set + using any keyboard input), an infinite loop is created which is causing some of Joy UI's Autocomplete tests to hang (internally it uses Base's useAutocomplete)

It can be reproed in a sandbox, but I couldn't figure out how to get the sandbox to restart after it hung the first time 😆

You can repro it in the local playground like this:

import * as React from 'react';
import { CssVarsProvider } from '@mui/joy';
import Autocomplete from '@mui/joy/Autocomplete';

export default function Playground() {
  return (
    <CssVarsProvider>
      <Autocomplete
        autoHighlight
        freeSolo
        placeholder="Combo box"
        options={['one', 'two']}
        sx={{ width: 300 }}
        open
      />
    </CssVarsProvider>
  );
}

1. Type `oo` into the input

2. It will hang after inputting the first `o`

The failing tests relate to prop: autoHighlight:

1. `should set the focus on the first item`

2. `should set the focus on the first item when possible`

3. `should work with filterSelectedOptions too`

Hello @mj12albert

I fixed this particular issue with the if statement

if (filteredOptions.length === 0) {
    return -1;
}

However my implementation still fails 11 more tests. I did not know that the components were so heavily coupled as this change really shouldn't impact that much. I am not sure how to solve my pull request without making the code hideous, or changing most of the components that uses useAutocomplete. I will sit with this issue a bit longer though.

Thank you so much for the help and repro!

[ZeeshanTamboli]

@adamhylander @mj12albert I'm closing this PR and favoring #38788, which was opened earlier and includes a correct fix with tests. You can review it in this CodeSandbox: https://codesandbox.io/s/thirsty-wave-jg325z?file=/Demo.tsx. @adamhylander, thank you for your effort!