[IndexFiltering]

[zakwarsame]

WHY are these changes introduced?

Fixes https://github.com/Shopify/web/pull/100983

• Currently when navigating to a given index table with the query included in the URL, the input field will contain the value but will not be focused
• Since only indexes containing queries have the state of mode as Filtering, it makes sense to initialize with mode. This way when the URL includes a query, the input field is focused without accessibility trade-offs.

WHAT is this pull request doing?

• This PR sets the initial focus of the input fields to true when IndexFiltersMode is Filtering.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

• Spin URL with query
  • Ensure the input is focus when you go to the page or refresh with the query included

Copy-paste this code in playground/Playground.tsx:

🎩 checklist

• Tested on mobile
• Tested on multiple browsers
• Tested for accessibility
  ~ - [ ] Updated the component's README.md with documentation changes~
• Tophatted documentation changes in the style guide

[zakwarsame]

/snapit

[github-actions]

🫰✨ Thanks @zakwarsame! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/polaris-cli@0.0.0-snapshot-release-20230810145959

yarn add @shopify/polaris-codemods@0.0.0-snapshot-release-20230810145959

yarn add @shopify/polaris-icons@0.0.0-snapshot-release-20230810145959

yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20230810145959

yarn add @shopify/polaris@0.0.0-snapshot-release-20230810145959

yarn add @shopify/polaris-tokens@0.0.0-snapshot-release-20230810145959

yarn add @shopify/stylelint-polaris@0.0.0-snapshot-release-20230810145959

[jesstelford]

Thanks for the contribution @zakwarsame!

I'm not sure of the accessibility implications of defaulting focus to true; wouldn't this steal the focus for every page with an IndexTable, jumping the user directly to the search field? How would a screen reader user handle that?

Relatedly; how would we handle the case when there's more than one <IndexTable> on the page? Which one should receive focus?

Finally, should this also trigger the onQueryFocus() callback?

An alternate solution might be a defaultFocus prop which is passed into useToggle(defaultFocus) + a useEffect() which calls onQueryFocus() once on mount if defaultFocus == true. This leaves the auto-focusing up to the callsite, allowing for more flexibility.

[zakwarsame]

@jesstelford I appreciate the feedback!

I'm not sure of the accessibility implications of defaulting focus to true; wouldn't this steal the focus for every page with an IndexTable, jumping the user directly to the search field? How would a screen reader user handle that?

I think that's a fair point but also, the useOnValueChange hook already runs on every render except the first. It checks the current mode of the IndexFilters and sets the focus when the mode is Filtering. From an a11y perspective, this hook should protect the focus on tables that aren't actively filtering.

I understand why blindly setting it to true isn't ideal though. Instead of defaulting to false on the first render, I think it makes sense to either:

a. Check if the mode is set to Filtering and set that as the initial value i.e useToggle(mode === IndexFiltersMode.Filtering); or
b. Add a useEffect that runs once throughout the lifecycle of the component i.e

useEffect(() => {
  handleModeChange(mode);
}, []);

Relatedly; how would we handle the case when there's more than one on the page? Which one should receive focus?

With the above solution in mind, whichever has the current mode set to Filtering.

Finally, should this also trigger the onQueryFocus() callback?

Currently, onQueryFocus() is independent from the mode logic. I think this is because handleQueryFocus is meant to handle it separately. If we want the onQueryFocus() to run each time the mode logic is run, we should add it inside the callback being passed to useOnValueChange which will automatically run it.

An alternate solution might be a defaultFocus prop which is passed into useToggle(defaultFocus) + a useEffect() which calls onQueryFocus() once on mount if defaultFocus == true. This leaves the auto-focusing up to the callsite, allowing for more flexibility.

I think this makes sense. Although, it might be a little redundant to add the prop if the intention is to simply ensure the mode is in Filtering state. Let me know what you think of the above solutions, otherwise, I'll add the prop.

[zakwarsame]

/snapit

[github-actions]

🫰✨ Thanks @zakwarsame! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20230830220301

yarn add @shopify/polaris@0.0.0-snapshot-release-20230830220301

[zakwarsame]

Closing in favor of #10284