Skip to content

Conversation

francinen
Copy link
Contributor

@francinen francinen commented Jan 3, 2020

WHY are these changes introduced?

When the Filters components mounts onto the DOM, the aria-expanded attribute on the Popover activator is set to undefined, which is an invalid value; the aria-expanded attribute should be set to either true or false.

Screen Shot 2020-01-03 at 11 30 26 AM

This results in an error when running a Lighthouse accessibility audit on a page that uses the Filters component.

Screen Shot 2020-01-03 at 11 26 15 AM

The aria-expanded attribute is set to true or false once a user starts interacting with the Popover activator.

filter-aria-expanded-before

WHAT is this pull request doing?

This PR ensures the aria-expanded attribute of the Popover activator in the Filters component is always set to either true or false.

The activator's aria-expanded attribute is determined by the popoverOpen key of a given action, which is set to the value of this.state[`${key}${Suffix.Shortcut}`] in the Filters component.

// In Filters.tsx
      transformedActions.push({
        popoverOpen: this.state[`${key}${Suffix.Shortcut}`],
        // Other props...
      });

// In ConnectedFilterControl.tsx
private popoverFrom(actions: PopoverableAction[]): React.ReactElement[] {
    return actions.map((action) => {
      return (
        <Item key={action.key}>
          <Popover
            active={action.popoverOpen}
            // Other props...
          >
            {action.popoverContent}
          </Popover>
        </Item>
      );
    });
  }

On mount, however, this.state[`${key}${Suffix.Shortcut}`] is undefined.

image

The state key for a filter action gets defined when the popover is either opened or closed on click, but it is not initialized on mount.

// In Filters.tsx
  state: State = {
    open: false,
    readyForFocus: false,
  };

  private openFilter(key: string) {
    this.setState({[`${key}${Suffix.Filter}`]: true});
  }

  private closeFilter(key: string) {
    this.setState({[`${key}${Suffix.Filter}`]: false});
  }

To ensure the aria-expanded attribute is set to either true or false, this PR transforms the value of this.state[`${key}${Suffix.Shortcut}`] into a Boolean when setting the popoverOpen key of a given action:

// In Filters.tsx
      transformedActions.push({
        popoverOpen: Boolean(this.state[`${key}${Suffix.Shortcut}`]),
        // Other props...
      });

Screen Shot 2020-01-03 at 11 59 18 AM

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import React, {useState, useCallback} from 'react';
import {
  Avatar,
  Card,
  ChoiceList,
  ResourceList,
  Filters,
  TextStyle,
} from '../src';

export function Playground() {
  const [accountStatus, setAccountStatus] = useState(null);
  const [moneySpent, setMoneySpent] = useState(null);
  const [taggedWith, setTaggedWith] = useState(null);
  const [queryValue, setQueryValue] = useState(null);

  const handleAccountStatusChange = useCallback(
    (value) => setAccountStatus(value),
    [],
  );
  const handleMoneySpentChange = useCallback(
    (value) => setMoneySpent(value),
    [],
  );
  const handleTaggedWithChange = useCallback(
    (value) => setTaggedWith(value),
    [],
  );
  const handleFiltersQueryChange = useCallback(
    (value) => setQueryValue(value),
    [],
  );
  const handleAccountStatusRemove = useCallback(
    () => setAccountStatus(null),
    [],
  );
  const handleMoneySpentRemove = useCallback(() => setMoneySpent(null), []);
  const handleTaggedWithRemove = useCallback(() => setTaggedWith(null), []);
  const handleQueryValueRemove = useCallback(() => setQueryValue(null), []);
  const handleFiltersClearAll = useCallback(() => {
    handleAccountStatusRemove();
    handleMoneySpentRemove();
    handleTaggedWithRemove();
    handleQueryValueRemove();
  }, [
    handleAccountStatusRemove,
    handleMoneySpentRemove,
    handleQueryValueRemove,
    handleTaggedWithRemove,
  ]);

  const filters = [
    {
      key: 'accountStatus',
      label: 'Account status',
      filter: (
        <ChoiceList
          title="Account status"
          titleHidden
          choices={[
            {label: 'Enabled', value: 'enabled'},
            {label: 'Not invited', value: 'not invited'},
            {label: 'Invited', value: 'invited'},
            {label: 'Declined', value: 'declined'},
          ]}
          selected={accountStatus || []}
          onChange={handleAccountStatusChange}
          allowMultiple
        />
      ),
      shortcut: true,
    },
  ];

  const appliedFilters = [];

  return (
    <div style={{height: '568px'}}>
      <Card>
        <ResourceList
          resourceName={{singular: 'customer', plural: 'customers'}}
          filterControl={
            <Filters
              queryValue={queryValue}
              filters={filters}
              appliedFilters={appliedFilters}
              onQueryChange={handleFiltersQueryChange}
              onQueryClear={handleQueryValueRemove}
              onClearAll={handleFiltersClearAll}
            />
          }
          items={[
            {
              id: 341,
              url: 'customers/341',
              name: 'Mae Jemison',
              location: 'Decatur, USA',
            },
            {
              id: 256,
              url: 'customers/256',
              name: 'Ellen Ochoa',
              location: 'Los Angeles, USA',
            },
          ]}
          renderItem={(item) => {
            const {id, url, name, location} = item;
            const media = <Avatar customer size="medium" name={name} />;

            return (
              <ResourceList.Item
                id={id}
                url={url}
                media={media}
                accessibilityLabel={`View details for ${name}`}
              >
                <h3>
                  <TextStyle variation="strong">{name}</TextStyle>
                </h3>
                <div>{location}</div>
              </ResourceList.Item>
            );
          }}
        />
      </Card>
    </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

@francinen francinen force-pushed the filters-fix-undefined-aria-expanded-attr branch from 9641be7 to afb7fb3 Compare January 3, 2020 18:17
@github-actions
Copy link
Contributor

github-actions bot commented Jan 3, 2020

💦 Potential splash zone of changes introduced to src/**/*.tsx in this pull request:

No significant changes to src/**/*.tsx were detected.


This comment automatically updates as changes are made to this pull request.
Feedback, troubleshooting: open an issue or reach out on Slack in #polaris-tooling.

@francinen
Copy link
Contributor Author

It's interesting that the Popover's type definition expect active to be a boolean, but it didn't raise any type errors.

Copy link
Contributor

@tmlayton tmlayton left a comment

Choose a reason for hiding this comment

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

Mind adding a test? Otherwise looks good! 👍

@francinen francinen force-pushed the filters-fix-undefined-aria-expanded-attr branch from afb7fb3 to 3335c89 Compare January 3, 2020 19:27
@francinen francinen requested a review from tmlayton January 3, 2020 19:27
Copy link

@dpersing dpersing left a comment

Choose a reason for hiding this comment

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

Thanks so much for fixing this, @francinen!

UNRELEASED.md Outdated

- Fixed issue with `Filters` component displaying an undesired margin top and bottom on the button element on Safari ([#2292](https://github.com/Shopify/polaris-react/pull/2292))

- Fixed an issue with `Filters` component where the `aria-expanded` attribute was set to an invalid value on mount ([#2589]https://github.com/Shopify/polaris-react/pull/2589)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Fixed an issue with `Filters` component where the `aria-expanded` attribute was set to an invalid value on mount ([#2589]https://github.com/Shopify/polaris-react/pull/2589)
- Fixed an issue with the `Filters` component where the `aria-expanded` attribute was `undefined` on mount ([#2589](https://github.com/Shopify/polaris-react/pull/2589))

Copy link
Contributor

@tmlayton tmlayton left a comment

Choose a reason for hiding this comment

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

Just a small tweak to the changelog entry, otherwise 👍

@francinen francinen force-pushed the filters-fix-undefined-aria-expanded-attr branch from 3335c89 to 9dc8e09 Compare January 7, 2020 19:50
@francinen francinen merged commit b66a571 into master Jan 7, 2020
@francinen francinen deleted the filters-fix-undefined-aria-expanded-attr branch January 7, 2020 20:58
@LauraAubin LauraAubin temporarily deployed to production January 16, 2020 16:22 Inactive
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.

4 participants