-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Focus guidance and experience update #7585
Comments
Beyond just the focus ring, it looks like we're trying to manually set things into the "focused" state outside of the Browser's normal algorithm. Ie; What is the second example trying to convey as the "focused" item? I can see three different things that appear to have some form of focus:
Only one thing can be focused at a time, so I don't understand how it's even possible to have two different focus ring styles, plus what looks like a hover-state style all at the same time 🤔 |
…8054) ### WHY are these changes introduced? Fixes #8053 Related to: #7585 and #818 We currently use a focus-ring to show a merchant which element is currently in focus. This is useful particularly for users that don't use a traditional pointer device to navigate through the admin. We currently use the :focus CSS selector to add these focus rings to interactive elements. Whilst this is an important accessibility necessity, it can leave the admin looking very busy for merchants that do use a traditional pointer device, with focus rings appearing on every interaction. There is a CSS selector named :focus-visible, which allows the browser to determine when to show the focus state depending on the input device that controls the focus state, and the element in question. This will allow us to persist the vital focus state visibility for users navigating the admin via keyboard, but will leave the admin feeling cleaner for those merchants using a mouse or other pointer device. ### WHAT is this pull request doing? This PR updates instances of `:focus` to show a focus ring and replaces them with `:focus-visible`. It also tidies up some legacy code in some components to detect for keyFocus only, preferring to use the browser standard `:focus-visible` rather than some internal component logic to figure out when focus is being managed by a keyboard. _Note: The Listbox remains untouched as the focus state logic there is pretty custom and will require somebody with more domain knowledge to update that component_ <!-- ℹ️ Delete the following for small / trivial changes --> ### How to 🎩 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development) 🗒 [General tophatting guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md) 📄 [Changelog guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog) <!-- Give as much information as needed to experiment with the component in the playground. --> <details> <summary>Copy-paste this code in <code>playground/Playground.tsx</code>:</summary> ```jsx import React from 'react'; import {Page} from '../src'; export function Playground() { return ( <Page title="Playground"> {/* Add the code you want to test in here */} </Page> ); } ``` </details> ### 🎩 checklist - [x] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [x] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [x] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [x] Updated the component's `README.md` with documentation changes - [x] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide
Going to close this issue and focus on this deliverable first: #9093 We can create a new issue when we are ready to change the visual style or document guidance around the focus. |
…hopify#8054) ### WHY are these changes introduced? Fixes Shopify#8053 Related to: Shopify#7585 and Shopify#818 We currently use a focus-ring to show a merchant which element is currently in focus. This is useful particularly for users that don't use a traditional pointer device to navigate through the admin. We currently use the :focus CSS selector to add these focus rings to interactive elements. Whilst this is an important accessibility necessity, it can leave the admin looking very busy for merchants that do use a traditional pointer device, with focus rings appearing on every interaction. There is a CSS selector named :focus-visible, which allows the browser to determine when to show the focus state depending on the input device that controls the focus state, and the element in question. This will allow us to persist the vital focus state visibility for users navigating the admin via keyboard, but will leave the admin feeling cleaner for those merchants using a mouse or other pointer device. ### WHAT is this pull request doing? This PR updates instances of `:focus` to show a focus ring and replaces them with `:focus-visible`. It also tidies up some legacy code in some components to detect for keyFocus only, preferring to use the browser standard `:focus-visible` rather than some internal component logic to figure out when focus is being managed by a keyboard. _Note: The Listbox remains untouched as the focus state logic there is pretty custom and will require somebody with more domain knowledge to update that component_ <!-- ℹ️ Delete the following for small / trivial changes --> ### How to 🎩 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development) 🗒 [General tophatting guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md) 📄 [Changelog guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog) <!-- Give as much information as needed to experiment with the component in the playground. --> <details> <summary>Copy-paste this code in <code>playground/Playground.tsx</code>:</summary> ```jsx import React from 'react'; import {Page} from '../src'; export function Playground() { return ( <Page title="Playground"> {/* Add the code you want to test in here */} </Page> ); } ``` </details> ### 🎩 checklist - [x] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [x] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [x] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [x] Updated the component's `README.md` with documentation changes - [x] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide
Our focus ring is misused and is overstated for the experience we are trying to provide:
We should update our focus design to feel more crafted and not as intense when navigating by keyboard. See examples:
The focus ring is also showing up when user are not using keyboard navigation. We should remove this as it causes confusion on the purpose of the ring and it's usage at scale.
#7585 (comment)
The text was updated successfully, but these errors were encountered: