Skip to content

Commit

Permalink
[Focus states] Use :focus-visible over :focus for focus-ring state. (#…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
mrcthms authored Jan 18, 2023
1 parent 1aeed24 commit c44c96c
Show file tree
Hide file tree
Showing 35 changed files with 64 additions and 250 deletions.
5 changes: 5 additions & 0 deletions .changeset/pink-students-compare.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/polaris': minor
---

Update focus states to be present on :focus-visible rather than :focus
2 changes: 1 addition & 1 deletion polaris-react/src/components/ActionList/ActionList.scss
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@
background-color: var(--p-surface-pressed);
}

&:focus:not(:active) {
&:focus-visible:not(:active) {
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
@include focus-ring($style: 'focused');
outline: var(--p-border-width-3) solid transparent;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@
background-color: var(--p-surface-pressed);
}

&:focus:not(:active) {
&:focus-visible:not(:active) {
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
@include focus-ring($style: 'focused');
}
Expand Down
4 changes: 2 additions & 2 deletions polaris-react/src/components/Banner/Banner.scss
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@
text-decoration: underline;
}

&:focus > .Text {
&:focus-visible > .Text {
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
@include plain-button-backdrop;
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
Expand Down Expand Up @@ -269,7 +269,7 @@
letter-spacing: initial;
color: var(--p-text);

&:focus {
&:focus-visible {
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
@include focus-ring($style: 'focused');
}
Expand Down
4 changes: 2 additions & 2 deletions polaris-react/src/components/Breadcrumbs/Breadcrumbs.scss
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,11 @@
}
}

&:focus {
&:focus-visible {
outline: none;
}

&:focus:not(:active) {
&:focus-visible:not(:active) {
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
@include focus-ring($style: 'focused');
}
Expand Down
6 changes: 3 additions & 3 deletions polaris-react/src/components/Button/Button.scss
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@
box-shadow: none;
}

&:focus {
&:focus-visible {
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
@include no-focus-ring;
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
Expand All @@ -303,7 +303,7 @@
}
}

&:focus:not(:active) {
&:focus-visible:not(:active) {
> .Content {
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
@include focus-ring($style: 'focused');
Expand Down Expand Up @@ -574,7 +574,7 @@
}
}

&:focus {
&:focus-visible {
box-shadow: 0 0 0 var(--p-border-width-1) currentColor;
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
@include focus-ring($style: 'focused');
Expand Down
2 changes: 1 addition & 1 deletion polaris-react/src/components/Checkbox/Checkbox.scss
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
@include visually-hidden;

&.keyFocused {
&:focus-visible {
+ .Backdrop {
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
@include focus-ring($style: 'focused');
Expand Down
15 changes: 1 addition & 14 deletions polaris-react/src/components/Checkbox/Checkbox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import React, {
forwardRef,
useRef,
useImperativeHandle,
useState,
useContext,
} from 'react';
import {MinusMinor, TickSmallMinor} from '@shopify/polaris-icons';
Expand All @@ -13,7 +12,7 @@ import {useUniqueId} from '../../utilities/unique-id';
import {Choice, helpTextID} from '../Choice';
import {errorTextID} from '../InlineError';
import {Icon} from '../Icon';
import {Error, CheckboxHandles, Key} from '../../types';
import type {Error, CheckboxHandles} from '../../types';
import {WithinListboxContext} from '../../utilities/listbox/context';

import styles from './Checkbox.scss';
Expand Down Expand Up @@ -76,7 +75,6 @@ export const Checkbox = forwardRef<CheckboxHandles, CheckboxProps>(
setTrue: handleMouseOver,
setFalse: handleMouseOut,
} = useToggle(false);
const [keyFocused, setKeyFocused] = useState(false);
const isWithinListbox = useContext(WithinListboxContext);

useImperativeHandle(ref, () => ({
Expand All @@ -89,15 +87,6 @@ export const Checkbox = forwardRef<CheckboxHandles, CheckboxProps>(

const handleBlur = () => {
onBlur && onBlur();
setKeyFocused(false);
};

const handleKeyUp = (event: React.KeyboardEvent) => {
const {keyCode} = event;

if (keyCode === Key.Space || keyCode === Key.Tab) {
!keyFocused && setKeyFocused(true);
}
};

const handleOnClick = () => {
Expand Down Expand Up @@ -142,7 +131,6 @@ export const Checkbox = forwardRef<CheckboxHandles, CheckboxProps>(
const inputClassName = classNames(
styles.Input,
isIndeterminate && styles['Input-indeterminate'],
keyFocused && styles.keyFocused,
);

return (
Expand Down Expand Up @@ -170,7 +158,6 @@ export const Checkbox = forwardRef<CheckboxHandles, CheckboxProps>(
onChange={noop}
onClick={handleOnClick}
onFocus={onFocus}
onKeyUp={handleKeyUp}
aria-invalid={error != null}
aria-controls={ariaControls}
aria-describedby={ariaDescribedBy}
Expand Down
26 changes: 0 additions & 26 deletions polaris-react/src/components/Checkbox/tests/Checkbox.test.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import React, {AllHTMLAttributes} from 'react';
import {mountWithApp} from 'tests/utilities';

import {Key} from '../../../types';
import {Checkbox} from '../Checkbox';

describe('<Checkbox />', () => {
Expand Down Expand Up @@ -276,31 +275,6 @@ describe('<Checkbox />', () => {
});
});
});

describe('Focus className', () => {
it('on keyUp adds a keyFocused class to the input', () => {
const checkbox = mountWithApp(<Checkbox label="Checkbox" />);

checkbox.find('input')!.trigger('onKeyUp', {
keyCode: Key.Space,
});

expect(checkbox).toContainReactComponent('input', {
className: 'Input keyFocused',
});
});

it('on change does not add a keyFocused class to the input', () => {
const checkbox = mountWithApp(<Checkbox label="Checkbox" />);
const checkboxInput = checkbox.find('input');
checkboxInput!.trigger('onChange', {
currentTarget: checkboxInput!.domNode as HTMLInputElement,
});
expect(checkbox).not.toContainReactComponent('input', {
className: 'Input keyFocused',
});
});
});
});

function noop() {}
2 changes: 1 addition & 1 deletion polaris-react/src/components/DataTable/DataTable.scss
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@
}
}

&:focus:not(:active) {
&:focus-visible:not(:active) {
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
@include focus-ring($style: 'focused');

Expand Down
2 changes: 1 addition & 1 deletion polaris-react/src/components/DatePicker/DatePicker.scss
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
@include focus-ring;

&:focus:not(:active) {
&:focus-visible:not(:active) {
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
@include focus-ring($style: 'focused');
}
Expand Down
2 changes: 1 addition & 1 deletion polaris-react/src/components/Filters/Filters.scss
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ $list-filters-footer-height: 70px;
outline: var(--p-border-width-1) solid transparent;
}

&:focus:not(:active) {
&:focus-visible:not(:active) {
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
@include focus-ring($style: 'focused');
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
background: var(--p-surface-pressed);
}

&:focus:not(:active) {
&:focus-visible:not(:active) {
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
@include focus-ring($style: 'focused');
}
Expand Down
17 changes: 8 additions & 9 deletions polaris-react/src/components/Navigation/Navigation.scss
Original file line number Diff line number Diff line change
Expand Up @@ -141,12 +141,12 @@ $disabled-fade: 0.6;
@include focus-ring;

&:hover,
&.keyFocused {
&:focus-visible {
background-color: var(--p-background-hovered);
color: var(--p-text-primary-hovered);
}

.keyFocused {
:focus-visible {
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
@include focus-ring($style: 'focused');
}
Expand Down Expand Up @@ -394,7 +394,7 @@ $disabled-fade: 0.6;
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
@include focus-ring;

&:focus {
&:focus-visible {
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
@include focus-ring($style: 'focused');
}
Expand Down Expand Up @@ -443,8 +443,7 @@ $disabled-fade: 0.6;
color: var(--p-text-subdued);
}

// stylelint-disable-next-line selector-max-class -- generated by polaris-migrator DO NOT COPY
&.keyFocused {
&:focus-visible {
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
@include focus-ring($style: 'focused');
color: var(--p-text);
Expand Down Expand Up @@ -482,8 +481,8 @@ $disabled-fade: 0.6;
&:hover {
color: var(--p-text-primary-hovered);
}
// stylelint-disable-next-line selector-max-class -- generated by polaris-migrator DO NOT COPY
&.keyFocused {

&:focus-visible {
color: var(--p-text-primary);
}

Expand Down Expand Up @@ -610,7 +609,7 @@ $disabled-fade: 0.6;
@include recolor-icon($filter-color: var(--p-filter-icon));
}

&:focus {
&:focus-visible {
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
@include focus-ring($style: 'focused');
}
Expand Down Expand Up @@ -653,7 +652,7 @@ $disabled-fade: 0.6;
color: var(--p-text-primary);
}

&:focus {
&:focus-visible {
outline: none;
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
@include focus-ring($style: 'focused');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -719,6 +719,7 @@ export function WithVariousStatesAndSecondaryElements() {
{
url: '/admin/products',
disabled: false,
selected: true,
label: 'Selected sub item',
},
{
Expand Down
6 changes: 3 additions & 3 deletions polaris-react/src/components/Navigation/_variables.scss
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ $nav-animation-variables: (
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
@include focus-ring;

&.keyFocused {
&:focus-visible {
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
@include focus-ring($style: 'focused');
background: var(--p-background-hovered);
Expand Down Expand Up @@ -127,7 +127,7 @@ $nav-animation-variables: (
}

.Item:hover &,
.Item.keyFocused & {
.Item:focus-visible & {
// stylelint-disable -- generated by polaris-migrator DO NOT COPY
@include recolor-icon(
$fill-color: var(--p-icon),
Expand All @@ -142,7 +142,7 @@ $nav-animation-variables: (
.subNavigationActive:hover &,
.Item-child-active &,
.Item-child-active:hover &,
.Item-selected.keyFocused & {
.Item-selected:focus-visible & {
// stylelint-disable -- generated by polaris-migrator DO NOT COPY
@include recolor-icon(
$fill-color: var(--p-action-primary),
Expand Down
Loading

0 comments on commit c44c96c

Please sign in to comment.