Skip to content

[Feature / Selectable A11y] Improve basic use case a11y#3070

Merged
myasonik merged 2 commits intoelastic:feature/selectable-a11yfrom
myasonik:selectable/replace-virtualize
Mar 25, 2020
Merged

[Feature / Selectable A11y] Improve basic use case a11y#3070
myasonik merged 2 commits intoelastic:feature/selectable-a11yfrom
myasonik:selectable/replace-virtualize

Conversation

@myasonik
Copy link
Copy Markdown
Contributor

@myasonik myasonik commented Mar 13, 2020 •
edited
Loading

Sorry for conflating two things in this PR - I thought all the changes were going to be smaller but as they started to pile on, I thought it'd be better to cut this PR and open a feature branch for future work.

Summary

This PR accomplishes two things:

  1. Swaps react-virtualized for react-window+react-virtualized-auto-sizer
    • virtualized makes some bad a11y assumptions that react-window doesn't make. They're both made by the same person actually and react-window is his slimmer replacement for virtualized.
    • I used react-virtualized-auto-sizer because after this effort, only one other component uses react-virtualized which would probably be good to swap for react-window as well, completetly removing react-virtualized as a dependency
  2. Fixes the a11y for a "basic" selectable
    • multiselect is done
    • searchable selectable is not
    • including vs excluding options is not

Breaking changes

  • virtualizedProps off of EuiSelectableOptionsList is now passed into react-window instead of react-virtualized which accept different hings

Future work

Mostly a11y work ahead:

  • For searchable selectables, we need to move a lot of the aria attributes to the <input /> instead of the <ul> where they are now
  • For excluding options, we need to figure out a better way to communicate if something is included/excluded and how that corresponds to the aria-selected state
  • Some a11y work on wiring up the loading/messaging states for searchable
  • Improved keyboard shortcuts
  • Improve mobile focus support (everything works but there are multiple focus states depending on how/where you tap/long-press). Will address after the rest of the DOM stuff settles in the future PRs.

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes

  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
    - [ ] Added documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
    - [ ] A changelog entry exists and is marked appropriately

@myasonik myasonik added breaking change PRs with breaking changes. (Don't delete - used for automation) accessibility priority labels Mar 13, 2020
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Mar 13, 2020

Preview documentation changes for this PR: https://eui.elastic.co/pr_3070/

@cchaos cchaos changed the title [Selectable] Swapped React-Virtualized for React-Window (& some a11y) [Feature / Selectable A11y] Swapped React-Virtualized for React-Window (& some a11y) Mar 14, 2020
@thompsongl thompsongl self-requested a review March 16, 2020 15:11
@chandlerprall
Copy link
Copy Markdown
Contributor

chandlerprall commented Mar 17, 2020

There's a visible flash in the icons when arrow-ing through the options. Wrapping the whole row in a memo seemed to help but didn't fix it. @thompsongl @chandlerprall, maybe one of you could take a look?

This is coming from the ListRow component being re-created every render. When React reconciles changes, it tests if the identity between components is the same. If it is, it will not unmount the component. identity will be the same if:

  • component type is the same - PrevType === NewType, e.g. "div" === "div" or ListRow === ListRow
  • AND key is not different

because ListRow is created every render pass, it's in-memory object identity will always be different, so React constantly unmounts & remounts the components.

@cla-checker-service
Copy link
Copy Markdown

cla-checker-service bot commented Mar 20, 2020 •
edited
Loading

💚 CLA has been signed

@myasonik myasonik force-pushed the selectable/replace-virtualize branch 2 times, most recently from 0181aa4 to 74525fb Compare March 20, 2020 23:26
@myasonik myasonik force-pushed the selectable/replace-virtualize branch from 74525fb to ec23a14 Compare March 20, 2020 23:35
@myasonik
Copy link
Copy Markdown
Contributor Author

myasonik commented Mar 20, 2020

Ready for review!

3 things to note:

  • The future work section will be in future PRs against the feature branch
  • I did a cursory VO check, but will do more in depth work closer to the end
  • Same for tests

@myasonik
Copy link
Copy Markdown
Contributor Author

myasonik commented Mar 20, 2020

What do people think — is this a breaking change?

It's only breaking in that an underlying library changed and DOM changed. No direct props changed.

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Mar 21, 2020

Preview documentation changes for this PR: https://eui.elastic.co/pr_3070/

thompsongl
thompsongl reviewed Mar 23, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

EUI's interface didn't change, so it doesn't appear to be a breaking change.

@myasonik
Copy link
Copy Markdown
Contributor Author

myasonik commented Mar 23, 2020

EUI does have a prop that just passes everything through to the underlying library though...

@thompsongl
Copy link
Copy Markdown
Contributor

thompsongl commented Mar 23, 2020

EUI does have a prop that just passes everything through to the underlying library though...

Ah yes, ListProps is the same name, but a totally different interface. Breaking change, it is.

thompsongl
thompsongl reviewed Mar 23, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@thompsongl thompsongl 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 couple notes:

This is going into a feature branch, so no changelog entry is necessary yet (do it when the feature branch goes into master). So, just make a note of the breaking change.

react-window is a good replacement 👍: https://github.com/bvaughn/react-window#how-is-react-window-different-from-react-virtualized

EuiSelectable functionality appears unchanged from what's currently released

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Mar 24, 2020

Preview documentation changes for this PR: https://eui.elastic.co/pr_3070/

chandlerprall
chandlerprall approved these changes Mar 24, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM; pulled this and tested locally, especially the keyboard navigation & focus stuff.

Definitely a breaking change, that button -> li update might introduce a few headaches but shouldn't be major.

@myasonik myasonik changed the title [Feature / Selectable A11y] Swapped React-Virtualized for React-Window (& some a11y) [Feature / Selectable A11y] Improve basic use case a11y Mar 25, 2020
@myasonik myasonik merged commit fdc9a6b into elastic:feature/selectable-a11y Mar 25, 2020
@myasonik myasonik deleted the selectable/replace-virtualize branch March 25, 2020 21:32
@myasonik myasonik mentioned this pull request Mar 25, 2020
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@cchaos cchaos cchaos left review comments

@chandlerprall chandlerprall chandlerprall approved these changes

@thompsongl thompsongl thompsongl approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

accessibility breaking change PRs with breaking changes. (Don't delete - used for automation)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@myasonik @kibanamachine @chandlerprall @thompsongl @cchaos