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.

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

Remove combobox autocomplete modes #2376

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

jattasNI
Copy link
Contributor

@jattasNI jattasNI commented Sep 4, 2024

Pull Request

🤨 Rationale

Fixes #2347. In that issue we decided to deprecate the combobox autocomplete modes both and inline because they are too aggressive about filtering out options that should match what the user typed but don't match what the component autocompleted. Short term, clients can either use autocomplete="list" or switch to a filterable nimble-select. Long term we may change combobox autocomplete to behave more like select.

👩‍💻 Implementation

  1. Created a type for the autocomplete attribute in Nimble to replace the one from FAST
  2. Deleted code from nimble-components that was only relevant to those modes
  3. Angular and Blazor: trivial test and example app updates, plus updating the Blazor type that corresponds to the nimble-components one.
  4. Storybook: remove docs and tests that reference those modes

In this SLE PR I'm updating places that used "both" to now use "list".

🧪 Testing

  • Updated tests that didn't care about the mode to use a mode that's still supported.
  • Deleted tests that no longer apply
  • the forked "foundation" test still uses the FAST type since it's testing FAST code that uses that type. There was one place where the test is relying on the Nimble element so it produced a type mismatch, but by luck that code was using modes that are still supported in both places, so some type tweaks allowed it to continue working.

✅ Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

@@ -525,7 +519,7 @@ describe('Combobox', () => {
// prettier-ignore
const viewTemplate = html`
<${comboboxTag}
autocomplete="inline"
autocomplete="list"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change causes the test in this describe block to fail both with and without the other changes in this PR. I haven't yet investigated why: interactively it behaves correctly to scroll the selected item into view when autocomplete="list".

Copy link
Contributor

@atmgrifter00 atmgrifter00 Sep 24, 2024

Choose a reason for hiding this comment

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

So, the issue is that when you change to list the items in the dropdown are limited to what has been typed in the input, whereas before they were not. Changing this to none (or simply removing this attribute), in addition to a change needed in the focusAndScrollOptionIntoView implementation (see this commit).

this.firstSelectedOption?.scrollIntoView({
block: 'nearest'
});
if (this.firstSelectedOption) {
Copy link
Contributor

@atmgrifter00 atmgrifter00 Sep 24, 2024

Choose a reason for hiding this comment

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

@jattasNI and @rajsitem the code I've removed here was partially responsible for keeping a test from passing. Essentially document.activeElement had not been updated to something contained within the Combobox. While I can't say I confidently know why that is, the removed logic still seems wholly unnecessary. I see no reason why we have to explicitly place focus on the input in the template when scrolling the selected option into view, nor a reason to gate this on the currently focused element already being within the Combobox. This method should just be concerned with scrolling the current selected item into view.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Combobox with autocomplete="both" filters out too many options
3 participants