Skip to content
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

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

Conversation

myasonik
Copy link
Contributor

@myasonik myasonik commented Mar 13, 2020

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

@kibanamachine
Copy link

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
Contributor

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

cla-checker-service bot commented Mar 20, 2020

💚 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
Contributor Author

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
Contributor Author

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

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

Copy link
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
Contributor Author

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

@thompsongl
Copy link
Contributor

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.

Copy link
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

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

Copy link
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants