[Autocomplete] Remove tags with the Delete key

[sfavello]

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

This was to solve the issue that Autocomplete does not handle the functionality of the delete key, more specifically the forward delete key. When creating a Chip and the chip is on hover, it passes on the impression that you can use either the backspace key or the delete key to eliminate the chip. It's best to stay consistent and include the functionality of a delete key.

React.Autocomplete.component.-.Material.UI.-.Google.Chrome.2022-08-06.13-09-14_Trim.mp4

[mui-bot]

	Messages
📖	Netlify deploy preview: https://deploy-preview-33822--material-ui.netlify.app/

Details of bundle changes

Generated by 🚫 dangerJS against 8d3da58

[michaldudak]

Good idea, thanks!
It would be great to include this new functionality in tests.

[michaldudak]

Looks like the code is duplicated. Could you add the case 'Delete' to the previous case block?

[sfavello]

Awesome, thank you! I will do that asap.

[michaldudak]

Thanks!

During testing, I found one behavior I'm not sure about. When no tags are selected (the cursor is visible), pressing Delete removes the last tag (just like the Backspace key does). This may be a bit confusing, as Delete usually removes a character to the right of the visible cursor, not to the left as in this case. So, after all, we may need a separate case for 'Delete' and run the code only if focusedTag !== -1.

The behavior for deleting selected tags looks fine.

---

Could you please merge/rebase on the latest master? We've had some issues with uploading regression tests' screenshots in the past, but they have been fixed, and the current master should work fine.

[sfavello]

Thanks!

During testing, I found one behavior I'm not sure about. When no tags are selected (the cursor is visible), pressing Delete removes the last tag (just like the Backspace key does). This may be a bit confusing, as Delete usually removes a character to the right of the visible cursor, not to the left as in this case. So, after all, we may need a separate case for 'Delete' and run the code only if focusedTag !== -1.

The behavior for deleting selected tags looks fine.

Could you please merge/rebase on the latest master? We've had some issues with uploading regression tests' screenshots in the past, but they have been fixed, and the current master should work fine.

I updated it whereDelete key will not delete the value at the end but will delete the value to the right when it's on a focusedTag. 😄

Video update (you can't see but in beginning I was hitting the delete key to show it won't work) 😅
_From around 0 to 5 seconds is the test to make sure the delete will only run when focusedTag !== -1

autocomplete.mov

[michaldudak]

👍 It does work well now when the cursor is behind tags. But when a tag is focused, Delete should delete the focused tag itself, not the next one.

[sfavello]

I completely misunderstood. 😅 My apologies. I fixed it where it will not delete a chip when the cursor is behind the tags and when on focusedTag it will delete the tag that's on focus. I also updated the test to reflect the changes. 😄

DEMO

autocompleteDemo.mov

[michaldudak]

Now the behavior is correct, thanks!
I found one possible improvement to do in the code.

[michaldudak]

No need to check if focusedTag === -1, as the outer if handles this already.

[sfavello]

Awesome, thank you! I appreciate it. I went in and made the change. 😄

[michaldudak]

Thanks for your work!

[michaldudak]

There's some issue with tests - would you mind checking it?

Failing tests

[sfavello]

So I checked the 3. failing tests

1. argos states that there is 8 differences detected in the screenshots
2. the e2e test is failing in a material-docs.spec.ts where the icon AcUnit is not visible.
   I'm still researching around why my changes could have possibly effected these two things, but I'll keep investigating.
3. The other failing test was fixed. 😄

[michaldudak]

Try merging in the latest master. It should help to solve the problems with Argos and E2E tests.
Could you also add another unit test that would verify if no tags are deleted if they are not highlighted? Something along these lines:

it('does not delete any tags with the delete key when no tag is highlighted', () => {
  const handleChange = spy();
  const options = ['one', 'two'];
  render(
    <Autocomplete
      defaultValue={options}
      options={options}
      onChange={handleChange}
      renderInput={(params) => <TextField {...params} autoFocus />}
      multiple
    />,
  );

  const textbox = screen.getByRole('combobox');

  fireEvent.keyDown(textbox, { key: 'Delete' });

  expect(handleChange.callCount).to.equal(0);
  expect(textbox).toHaveFocus();
});

[sfavello]

Try merging in the latest master. It should help to solve the problems with Argos and E2E tests. Could you also add another unit test that would verify if no tags are deleted if they are not highlighted? Something along these lines:

it('does not delete any tags with the delete key when no tag is highlighted', () => {
  const handleChange = spy();
  const options = ['one', 'two'];
  render(
    <Autocomplete
      defaultValue={options}
      options={options}
      onChange={handleChange}
      renderInput={(params) => <TextField {...params} autoFocus />}
      multiple
    />,
  );

  const textbox = screen.getByRole('combobox');

  fireEvent.keyDown(textbox, { key: 'Delete' });

  expect(handleChange.callCount).to.equal(0);
  expect(textbox).toHaveFocus();
});

Added test and updated the branch. That fixed the problem. 🥳 . Thank you for your patience. I appreciate it.

[michaldudak]

Looks good to me. Thanks again for your contribution!

[oliviertassinari]

Oh nice, I didn't think about the Del key. When I was on Windows, I always struggled to reach this key on my keyboard.