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

Components: Add a WAI-ARIA compliant custom select. #17926

Merged
merged 11 commits into from Nov 28, 2019
Merged

Conversation

@epiqueras
Copy link
Contributor

epiqueras commented Oct 13, 2019

Closes #16473

Description

This PR implements a fully accessible and customizable select component with individually style-able items, in the @wordpress/components package.

It uses Downshift, a library that will be very useful for any of our other complex a11y input needs.

It has some modifications hooked on top of it to make it fully compliant with the spec. These modifications might soon make it into a future Downshift release and at that point we should remove them from our implementations.

The story for the component is:

https://deploy-preview-17926--gutenberg-playground.netlify.com/design-system/components/?path=/story/customselect--default

How has this been tested?

It was verified that the component works as expected.

Although the libraries used are heavily tested, unit testing this implementation once the approach is approved wouldn't be a bad idea to make sure we never regress to configuring something incorrectly.

Screenshots

Screen Shot 2019-10-13 at 12 46 18 PM

Types of Changes

New Feature: @wordpress/components now has a fully WAI-ARIA compliant custom select.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Oct 14, 2019

Design-wise the options should be shown in a Popover I think.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Oct 14, 2019

Design-wise the options should be shown in a Popover I think.

I want to tick all the a11y boxes first, and then see what we can do with styling without regressing.

@epiqueras epiqueras force-pushed the add/a11y-custom-select branch 2 times, most recently from 70ae1d5 to 421f9fb Oct 17, 2019
Copy link
Contributor

tellthemachines left a comment

Good work on this! Checked with VO/Safari and NVDA/Firefox and it's behaving pretty well on both, apart from the repetition of "Font size" label I commented on below.

One thing I noticed on NVDA but was unable to reproduce consistently is that sometimes the font size button is announced as "expanded" when focused, and with the dropdown closed. I realise this may not be very helpful, will try and debug further 😄

Another potential issue is that focus is not being limited to the component when it is open, so if we open the dropdown and then press tab we move out of it but the dropdown stays open. Shift-tabbing until the font size button is focused again allows us to continue interacting with the dropdown, but after that pressing esc will not close it.

Also, minor styling issue on Firefox and IE:

Screen Shot 2019-10-18 at 11 36 46 am

Removing display: inline-flex and adding text-align: left to components-button will fix it for both browsers 🙂

import { Button, Dashicon } from '../';

const itemToString = ( item ) => item.name;
// This is needed so that in Windows, where

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Oct 18, 2019

Contributor

Just checked this on Windows and the behaviour is identical to Mac: on arrow down/up the menu opens. I don't see that as a major issue though. If we did want the arrow keys to work with the menu closed we'd probably have to make sure the options always render, and hide them with CSS, but then it's not clear that we could limit that behaviour to Windows 🤷‍♀

This comment has been minimized.

Copy link
@silviuavram

silviuavram Oct 18, 2019

the issue here is the returned changes. @epiqueras just return the selectedItem because if you also return the rest of the changes then one of them is isOpen that comes as true :D

This comment has been minimized.

Copy link
@epiqueras

epiqueras Oct 18, 2019

Author Contributor

Fixed 😄

{ ...getLabelProps( { className: 'components-custom-select__label' } ) }
>
{ label }
</span>

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Oct 18, 2019

Contributor

Any reason not to make this an actual <label> instead of using aria-labelledby? Then we might not need the aria-label. Having both, VoiceOver and NVDA are announcing the button as "Font size font size". Would be good to avoid the repetition.

This comment has been minimized.

Copy link
@silviuavram

silviuavram Oct 18, 2019

The way I wrote useSelect was with having a physical label that also labels the button and the menu. As I understood there was an issue with Dragon Speech because of this.

Having said that, anyone tried Dragonn Speech to select an item then check what the console.log prints?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Oct 18, 2019

Author Contributor

Any reason not to make this an actual instead of using aria-labelledby? Then we might not need the aria-label. Having both, VoiceOver and NVDA are announcing the button as "Font size font size". Would be good to avoid the repetition.

Dragon Speech still needs the explicit aria label on the toggle to be able to click on it.

Having said that, anyone tried Dragonn Speech to select an item then check what the console.log prints?

It registers a click on the toggle button I think:

Screen Shot 2019-10-18 at 1 58 17 PM

In that test^^ "Small" was already the selected item.

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Oct 22, 2019

Contributor

Hmm can we use the semantic <label> and add aria-label too? @enriquesanchez 's testing on #17418 showed that approach works with Dragon, and aria-label seems to supersede <label> so the screen readers don't read out both.

With the current font size controls we're already having an issue where VoiceOver reads out the fieldset legend twice (that's a VO bug, but still annoying), so it would be good to avoid unnecessary repetition wherever we can.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 5, 2019

Author Contributor

Done!

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Oct 18, 2019

@tellthemachines Thanks for the help!

Removing display: inline-flex and adding text-align: left to components-button will fix it for both browsers 🙂

Done 😄

@karmatosed karmatosed added this to Inbox in Tightening Up Oct 21, 2019
@karmatosed karmatosed moved this from Inbox to Ready to create in Tightening Up Oct 21, 2019
@karmatosed karmatosed moved this from Ready to create to In Progress in Tightening Up Oct 21, 2019
Copy link
Contributor

tellthemachines left a comment

We're getting there!

One remaining issue, apart from the comments I left: pressing Tab when the dropdown is open still causes focus to be moved to the button control. Subsequently pressing Esc will not close the dropdown, but pressing any arrow key will close it.

Also, would be good to test replacing the current font size picker with this component, to check focus management when entering/exiting it.

{ ...getLabelProps( { className: 'components-custom-select__label' } ) }
>
{ label }
</span>

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Oct 22, 2019

Contributor

Hmm can we use the semantic <label> and add aria-label too? @enriquesanchez 's testing on #17418 showed that approach works with Dragon, and aria-label seems to supersede <label> so the screen readers don't read out both.

With the current font size controls we're already having an issue where VoiceOver reads out the fieldset legend twice (that's a VO bug, but still annoying), so it would be good to avoid unnecessary repetition wherever we can.

border: 1px solid $dark-gray-200;
border-radius: 4px;
color: $dark-gray-500;
display: initial;

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Oct 22, 2019

Contributor

I'm afraid IE doesn't support initial 🙀
Can we replace this with inline, which is iirc the default value for <button>?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 5, 2019

Author Contributor

Done!

@enriquesanchez

This comment has been minimized.

Copy link
Contributor

enriquesanchez commented Oct 22, 2019

I ran some tests of the component in https://deploy-preview-17926--gutenberg-playground.netlify.com/design-system/components/?path=/story/customselect--default with Dragon, NVDA and Voice Over.

Here are the videos of the test:

With Dragon: https://cldup.com/cihauaUTN4.m4v
So far with Dragon I haven't able to select an option from the list by saying "Click Large", but I'm able to do so if I use the "move up/down" commands to switch focus to an option and then say "press enter". I'm a fairly new user of speech recognition software, so I'm not sure how common or uncommon this flow is. That being said, I was able to interact with the component and select an option.

With NVDA: https://cldup.com/7n79VbZMo3.m4v (sorry for the shaky camera)
Besides "Font Size" being announced repetitively, the only thing that I felt was missing was that on Windows I can't change the selection by using the up/down keys, like on Mac.

With Voice Over: https://cldup.com/A12FmcNrSn.m4v
Same as with NVDA, "Font Size" is announced repetitively. Other than that, interaction was as expected.

@tellthemachines

This comment has been minimized.

Copy link
Contributor

tellthemachines commented Oct 22, 2019

Besides "Font Size" being announced repetitively, the only thing that I felt was missing was that on Windows I can't change the selection by using the up/down keys, like on Mac.

@enriquesanchez this happens because NVDA is not switching into forms mode when entering this component. If you manually switch by pressing Insert+Space then you'll be able to use up/down keys. It would be worth investigating if it's possible to make NVDA switch automatically as that would be closer in behaviour to a native control.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 3, 2019

Yeah Downshift has an esm distribution. I think our tooling doesn't support it, but we can work on that right @aduth?

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Dec 3, 2019

If it can be tree-shaken, our build should be able to drop the unused code. I was judging based on the BundlePhobia page, specifically this text under "Exports Analysis":

Exports analysis is available only for packages that export ES Modules and are side-effect free. This package exports ES6 modules, but isn't marked side-effect free.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 3, 2019

I guess it's not explicitly marked side-effect free, but I don't think it has any side-effects does it?

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Dec 3, 2019

It still needs to be included in the published package.json for Webpack to consider it for tree shaking:

The new webpack 4 release expands on this capability with a way to provide hints to the compiler via the "sideEffects" package.json property to denote which files in your project are "pure" and therefore safe to prune if unused.

https://webpack.js.org/guides/tree-shaking/#mark-the-file-as-side-effect-free

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 3, 2019

It looks like it will still try to tree-shake most things, but will trip up with things like HOCs without directives:

https://webpack.js.org/guides/tree-shaking/#clarifying-tree-shaking-and-sideeffects

@silviuavram can we add the sideEffects hint to make this more efficient?

@silviuavram

This comment has been minimized.

Copy link

silviuavram commented Dec 4, 2019

I am really interested to try this out, I will make changes on Downshift side if needed. Let me know on the progress please!

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 4, 2019

@silviuavram it's Downshift that is missing the sideEffects property, we need to add it there.

@silviuavram

This comment has been minimized.

Copy link

silviuavram commented Dec 4, 2019

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 4, 2019

@silviuavram downshift-js/downshift#843

Do we need
to publish an alpha version to test if it works?

Yeah, that would be helpful. It should work, unless Downshift has side effects, but better to be safe.

@silviuavram

This comment has been minimized.

Copy link

silviuavram commented Dec 4, 2019

@silviuavram

This comment has been minimized.

Copy link

silviuavram commented Dec 5, 2019

I published downshift@3.5.0-alpha.0 and could not manage to properly test it. Maybe my testing project is not configured as it should. If you have any tips that will help me looking into it tomorrow that will be super helpful.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 5, 2019

@silviuavram

This comment has been minimized.

Copy link

silviuavram commented Dec 5, 2019

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 5, 2019

Can you push your test somewhere?

@silviuavram

This comment has been minimized.

Copy link

silviuavram commented Dec 6, 2019

https://github.com/silviuavram/check-downshift-shake tried it here. But I probably did not configure webpack or babel. The alpha version contains useCombobx along with the sideEffects in package.json.

@silviuavram

This comment has been minimized.

Copy link

silviuavram commented Dec 6, 2019

I updated the repo with some things I tried on babel. Still it imports the whole things in the ES bundle.

@epiqueras

This comment has been minimized.

@silviuavram

This comment has been minimized.

Copy link

silviuavram commented Dec 6, 2019

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 6, 2019

I just debugged this and it's probably because of how Downshift bundles its esm module.

There are a lot of static property assignments in there like Downshift.stateChangeTypes = stateChangeTypes; and useCombobox.stateChangeTypes = stateChangeTypes$2;.

Some prior discussion:

downshift-js/downshift#380

@silviuavram

This comment has been minimized.

Copy link

silviuavram commented Dec 7, 2019

Looks good, I was also suspicious about those state change things while going through the bundled code. Thanks so much for looking into this!

The bad part is the fact that we need to deliver a breaking change to make the bundle tree-shakeable again: exporting those stateChangeTypes separately and removing the static allocation. Will check if indeed, with these changes, the problem will be fixed.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 7, 2019

There are other workarounds:

  • You could break down the output into multiple files with their ESM imports intact and that would allow the sideEffects flag to work. Currently, Webpack evaluates the single output file, because there are used exports there, and it can't prune the unused ones because they are mutated. If they were imported and mutated from a different file, Webpack could use the sideEffects hint and avoid evaluating those files.

  • Another, perhaps easier change, would be to use the /*#__PURE__*/ annotations. They're like the sideEffects hint, but for statements instead of modules.

Related discussion: styled-components/babel-plugin-styled-components#245.

@silviuavram

This comment has been minimized.

Copy link

silviuavram commented Dec 7, 2019

Found a few problems with today's investigation:

The hooks were not tree-shaken because of

const validatePropTypes = getPropTypesValidator(useSelect, propTypes)

Downshift component not tree-shaken because of the static props stateChangeTypes and defaultProps!!!. The propTypes static one was correctly removed by a babel transpiler (I think) at bundle time, like:

process.env.NODE_ENV !== "production" ? Downshift.propTypes = {
  children: PropTypes.func,
  ...

Probably I could add this same check manually to the hooks, because I don't need that prop types check function to run in production. On the Downshift static fields stateChangeTypes and defaultProps maybe there is a solution with one of your suggestions?

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 7, 2019

Yeah, both should work, albeit the first one is a better long-term solution that will work for future code as well.

@youknowriad youknowriad modified the milestones: Future, Gutenberg 7.1 Dec 9, 2019
@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Dec 9, 2019

Any plans to add a README for this component.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 9, 2019

#19026

@adamsilverstein

This comment has been minimized.

Copy link
Contributor

adamsilverstein commented Dec 10, 2019

With Downshift merged, we should reconsider using it elsewhere (eg for Author selection, page parent selection, etc... eg #7478).

@afercia I missed most of the work/conversation here - does the progress on Downshift address the feedback from the original ticket? shall I work on a new PR for you to review?

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 10, 2019

Yes, that is a good next step with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.