[Popover] Prevent setting a11y attributes when activator disabled #2473

chloerice · 2019-11-27T00:56:24Z

WHY are these changes introduced?

Currently, the Popover sets accessibility attributes on either the activator (if focusable) or the element it wraps the activator with. If the activator is not focusable because it is disabled, the activator's wrapper gets these attributes. This is a problem because the activator wrapper is a div (unless the activatorWrapper prop is set to something else) and the div having a tabIndex="-1" makes it focusable ironically!

A good example of this can be seen in the "All filters disabled" Filters example. When viewing the playground code on master, you can view the unwanted attributes being set on and staying set on the wrapper div after toggling the disabled state of the filters.

WHAT is this pull request doing?

Uses a version of the findFirstFocusableNode utility that doesn't eliminate disabled elements.
Prevents tabIndex from being set on the activator when the activator is disabled

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy and paste the collapsed playground code below into /playground/Playground.tsx
While still on master
- yarn dev
- View the playground and inspect the disabled "File type" filter popover activator.
- You'll see the focus ring is due to the tabindex="-1" on the wrapper div as well as the other accessibility attributes.
- Click the "Toggle empty state" secondary action
- Now that the activator button is no longer disabled it now has all of the expected accessibility attributes, but you'll see that the wrapper div still has all of the accessibility attributes on it as well.
git checkout popover-activatorDisabled
- Inspect the disabled "File type" filter popover activator again, you should see no attributes on the wrapper div
- Click the "Toggle empty state" secondary action
- You'll see that the accessibility attributes are all on the button

Click to view collapsed example code

import React, {useState, useCallback} from 'react';
import {
  Page,
  Layout,
  ResourceList,
  ResourceItem,
  EmptyState,
  Card,
  TextStyle,
  Select,
  Filters,
  Thumbnail,
  Tabs,
  SkeletonBodyText,
  Spinner,
} from '../src';

const allItems = [
  {
    id: 341,
    url: 'files/341',
    name: 'Point of sale staff',
    imgSource:
      'https://user-images.githubusercontent.com/29233959/61383074-fc5e6800-a87b-11e9-8e92-487153efd0dc.png',
    fileType: 'png',
  },
  {
    id: 256,
    url: 'files/256',
    name: 'File upload',
    imgSource:
      'https://user-images.githubusercontent.com/18447883/59945230-fa4be980-9434-11e9-9106-a373f0efbe08.png',
    fileType: 'png',
  },
];

const tabs = [
  {
    id: 'all-files',
    content: 'All',
    accessibilityLabel: 'All files',
    panelID: 'all-files-content',
  },
];

export function Playground() {
  const [viewLoadingState, setLoadingState] = useState(false);
  const [viewEmptyState, setEmptyState] = useState(true);
  const [filteredItems, setFilteredItems] = useState([]);
  const [fileType, setFileType] = useState('');
  const [queryValue, setQueryValue] = useState('');

  const toggleLoadingState = useCallback(
    () => setLoadingState((viewLoadingState) => !viewLoadingState),
    [],
  );

  const toggleEmptyState = useCallback(
    () => setEmptyState((viewEmptyState) => !viewEmptyState),
    [],
  );

  const updateFilteredItems = useCallback((items) => {
    setFilteredItems(items);
  }, []);

  const updateFileTypeFilter = useCallback(
    (value) => {
      setFileType(value);
      const itemsToFilter =
        queryValue && filteredItems.length > 0 ? filteredItems : allItems;
      updateFilteredItems(filterByFileType(value, itemsToFilter));
    },
    [filteredItems, queryValue, updateFilteredItems],
  );

  const updateQueryValue = useCallback(
    (value) => {
      setQueryValue(normalizeString(value));
      const itemsToFilter =
        fileType && filteredItems.length > 0 ? filteredItems : allItems;

      updateFilteredItems(
        filterByQueryValue(normalizeString(value), itemsToFilter),
      );
    },
    [fileType, filteredItems, updateFilteredItems],
  );

  const handleRemove = useCallback(() => {
    updateFileTypeFilter('');
  }, [updateFileTypeFilter]);

  const handleQueryClear = useCallback(() => {
    updateQueryValue('');
  }, [updateQueryValue]);

  const handleClearAll = useCallback(() => {
    updateFileTypeFilter('');
    updateQueryValue('');
  }, [updateFileTypeFilter, updateQueryValue]);

  let items = fileType || queryValue ? filteredItems : allItems;

  if (viewEmptyState) {
    items = [];
  }

  const filters = [
    {
      key: 'fileType',
      label: 'File type',
      filter: (
        <Select
          labelHidden
          label="File type"
          value={fileType}
          options={[
            {label: 'JPEG', value: 'jpeg'},
            {label: 'PNG', value: 'png'},
            {label: 'MP4', value: 'mp4'},
          ]}
          onChange={updateFileTypeFilter}
        />
      ),
      shortcut: true,
    },
  ];

  const appliedFilters = fileType
    ? [
        {
          key: 'fileType',
          label: `File type ${fileType.toUpperCase()}`,
          onRemove: handleRemove,
        },
      ]
    : [];

  const filterControl = (
    <Filters
      disabled={viewEmptyState}
      queryValue={queryValue}
      filters={filters}
      appliedFilters={appliedFilters}
      onQueryChange={updateQueryValue}
      onQueryClear={handleQueryClear}
      onClearAll={handleClearAll}
    />
  );

  const emptyStateMarkup = (
    <EmptyState
      heading="Upload a file to get started"
      action={{content: 'Upload files'}}
      image="https://cdn.shopify.com/s/files/1/2376/3301/products/file-upload-empty-state.png"
    >
      <p>
        You can use the Files section to upload images, videos, and other
        documents
      </p>
    </EmptyState>
  );

  const loadingStateMarkup = viewLoadingState ? (
    <Card>
      <SkeletonTabs />

      <div>
        <div style={{padding: '16px', marginTop: '1px'}}>
          <Filters
            disabled={viewEmptyState}
            queryValue={queryValue}
            filters={filters}
            appliedFilters={appliedFilters}
            onQueryChange={updateQueryValue}
            onQueryClear={handleQueryClear}
            onClearAll={handleClearAll}
          />
        </div>
        <div
          style={{
            display: 'flex',
            justifyContent: 'center',
            alignItems: 'center',
            minHeight: '160px',
          }}
        >
          <Spinner />
        </div>
      </div>
    </Card>
  ) : null;

  const withDataStateMarkup = !viewLoadingState ? (
    <Card>
      <Tabs selected={0} tabs={tabs}>
        <ResourceList
          showHeader
          hasMoreItems={!viewEmptyState && items.length < allItems.length}
          resourceName={{singular: 'file', plural: 'files'}}
          items={items}
          renderItem={renderItem}
          filterControl={filterControl}
        />
      </Tabs>
    </Card>
  ) : null;

  return (
    <Page
      title="Files"
      secondaryActions={[
        {
          content: 'Toggle empty state',
          onAction: toggleEmptyState,
          disabled: viewLoadingState,
        },
        {
          content: 'Toggle loading state',
          onAction: toggleLoadingState,
        },
      ]}
      breadcrumbs={[{content: 'Home', url: '/'}]}
    >
      <Layout>
        <Layout.Section>
          {loadingStateMarkup}
          {withDataStateMarkup}
        </Layout.Section>
      </Layout>
    </Page>
  );
}

function renderItem(item) {
  const {id, url, name, imgSource, altText = '', fileType} = item;
  const media = <Thumbnail alt={altText} source={imgSource} />;
  return (
    <ResourceItem id={id} url={url} media={media}>
      <h3>
        <TextStyle variation="strong">{name}</TextStyle>
      </h3>
      <div>{fileType}</div>
    </ResourceItem>
  );
}

function filterByFileType(fileType, items) {
  return items.filter((item) => item.fileType === fileType);
}

function filterByQueryValue(query, items) {
  return items.filter((item) => {
    return (
      normalizeString(item.name).includes(query) ||
      normalizeString(item.fileType).includes(query) ||
      normalizeString(item.imgSource).includes(query)
    );
  });
}

function normalizeString(string) {
  return string.toLowerCase();
}

function SkeletonTabs() {
  return (
    <div
      style={{
        width: '100%',
        display: 'flex',
        borderBottom: '1px solid #DFE3E8',
        height: '53px',
      }}
    >
      <div
        style={{
          width: '80px',
          padding: '21px 20px',
        }}
      >
        <SkeletonBodyText lines={1} />
      </div>
    </div>
  );
}

🎩 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
For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

github-actions · 2019-11-27T00:58:17Z

🟡 This pull request modifies 6 files and might impact 66 other files. This is an average splash zone for a change, remember to tophat areas that could be affected.

Details:

All files potentially affected (total: 66)

📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Popover/Popover.tsx (total: 56)

Files potentially affected (total: 56)

🧩 src/components/Popover/set-activator-attributes.ts (total: 57)

Files potentially affected (total: 57)

🧩 src/components/Popover/tests/Popover.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Popover/tests/set-activator-attributes.test.ts (total: 0)

Files potentially affected (total: 0)

🧩 src/utilities/focus.ts (total: 66)

Files potentially affected (total: 66)

AndrewMusgrave

Nice catch 😄

AndrewMusgrave · 2019-11-28T00:17:19Z

src/components/Popover/Popover.tsx

    const focusableActivator = firstFocusable || activatorContainer.current;
-    setActivatorAttributes(focusableActivator, {id, active, ariaHaspopup});
-  }, [active, ariaHaspopup, id]);
+    setActivatorAttributes(focusableActivator, {


Rather than adding an activatorDisabled prop, what do you think about deriving the value rather than explicitly setting it?

// HTMLElement doesn't contain `disabled` in its interface // We could alternatively use instanceof `focusableActivator instnaceof HTMLButton && focusableActivator.disabled` const activatorDisabled = (focusableActivator as HTMLButtonElement).disabled;

The reason I ended up going the prop route is that the Popover can't expect/know what kind of element its activator is or whether it's even focusable. The above snippet would work, but we'd need to chain all other possibilities to that conditional (it could be a textfield, etc). Since disabling a Button, TextField etc is done explicitly, just telling the Popover that the activator, whatever it may be, is disabled explicitly is a lot easier to test.

I wasn't aware that activators can be non-focusable. Maybe this is something we should try and enforce through types. Seems like a huge accessibility issue to not be able to open a popover through keyboard interactions 😬 cc/ @dpersing since your assigned as well, what do you think?

Using HTMLButtonElement type was just a quick example. There's other ways we can achieve the same type safety that'll work on any disablable element e.g.

const focusableActivator: HTMLElement & {disabled?: boolean} = firstFocusable || activatorContainer.current; const activatorDisabled = 'disabled' in focusableActivator && Boolean(focusableActivator.disabled);

Unfortunately adding the typing alone doesn't help. The real source of the problem is the findFirstFocusableNode utility because it eliminates disabled elements as focusable selectors.

Modifying the FOCUSABLE_SELECTORs to remove the :not(disabled) in @shopify/javascript-utilities could cause unexpected breaking changes for other consumers of the utility, so I've updated the PR to use our own version.

dpersing

The attribute assigning situation looks great!

One note on the default collapsed state of the popover button: the value of the aria-expanded attribute is currently undefined on load. It should be collapsed. Without a valid value, the fact that the button expands/collapses isn't conveyed when it is read, so users won't know what to expect.

<button id="Activator-fileType" type="button" class="Button-Button_2pL_i" tabindex="0" aria-controls="Polarispopover10" aria-owns="Polarispopover10" aria-expanded="undefined">...</button>

Issue addressed (added missing default value of false for aria-expanded)

chloerice · 2020-01-27T21:26:33Z

src/components/Popover/set-activator-attributes.ts

  {
    id,
-    active,
+    active = false,


@dpersing Adding a default value for the active prop prevents aria-expanded being defined as undefined on line 18/24.

[Popover] Only set accessibility attributes when activatorDisabled is false [Popover] update change log [Popover] Support finding disabled focusable node

AndrewMusgrave

💯

chloerice mentioned this pull request Nov 27, 2019

[ResourceList] Add support for in context empty state #2160

Merged

6 tasks

chloerice requested review from AndrewMusgrave and dpersing November 27, 2019 03:44

chloerice force-pushed the popover-activatorDisabled branch from 8309ed4 to 2170080 Compare November 27, 2019 04:15

AndrewMusgrave reviewed Nov 28, 2019

View reviewed changes

dpersing previously requested changes Dec 2, 2019

View reviewed changes

chloerice force-pushed the popover-activatorDisabled branch 3 times, most recently from 0933908 to 1f2c6f3 Compare January 27, 2020 21:05

chloerice requested a review from dpersing January 27, 2020 21:20

chloerice force-pushed the popover-activatorDisabled branch from 1f2c6f3 to 271c2ea Compare January 27, 2020 21:24

chloerice commented Jan 27, 2020

View reviewed changes

chloerice requested a review from AndrewMusgrave February 3, 2020 16:09

chloerice force-pushed the popover-activatorDisabled branch 6 times, most recently from e84c9ea to 88090f0 Compare February 27, 2020 06:19

[Popover] Support finding disabled focusable node

ea4b453

[Popover] Only set accessibility attributes when activatorDisabled is false [Popover] update change log [Popover] Support finding disabled focusable node

chloerice force-pushed the popover-activatorDisabled branch from 88090f0 to ea4b453 Compare February 27, 2020 06:22

AndrewMusgrave approved these changes Mar 2, 2020

View reviewed changes

chloerice merged commit 397432b into master Mar 4, 2020

chloerice deleted the popover-activatorDisabled branch March 4, 2020 00:25

tmlayton temporarily deployed to production March 7, 2020 05:24 Inactive

[Popover] Prevent setting a11y attributes when activator disabled #2473

[Popover] Prevent setting a11y attributes when activator disabled #2473

Uh oh!

Conversation

chloerice commented Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

Uh oh!

github-actions bot commented Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Files potentially affected (total: 0)

Files potentially affected (total: 56)

Files potentially affected (total: 57)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 66)

Uh oh!

AndrewMusgrave left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewMusgrave Nov 28, 2019

Choose a reason for hiding this comment

Uh oh!

chloerice Dec 2, 2019

Choose a reason for hiding this comment

Uh oh!

AndrewMusgrave Feb 26, 2020

Choose a reason for hiding this comment

Uh oh!

chloerice Feb 27, 2020

Choose a reason for hiding this comment

Uh oh!

dpersing left a comment

Choose a reason for hiding this comment

Uh oh!

chloerice Jan 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewMusgrave left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chloerice commented Nov 27, 2019 •

edited

Loading

github-actions bot commented Nov 27, 2019 •

edited

Loading

chloerice Jan 27, 2020 •

edited

Loading