fix(multiselect): replace checkbox with visually identical element

[tw15egan]

Closes #4008

After reading Josh's comment, this replaces the use of a checkbox entirely and uses a visually identical element to visually indicate change.

Changelog

Changed

• Replaces Checkbox inside of MultiSelect and uses an element that toggles the checkbox styles based on isChecked

Testing / Reviewing

Open the MultiSelect component, expand the MultiSelect, then run DAP and ensure there are no longer any violations. There will be 1 violation due to the nature of the Storybook.

Also, ensure new element styles match old styles

[netlify]

Deploy preview for the-carbon-components ready!

Built with commit e429737

https://deploy-preview-5039--the-carbon-components.netlify.com

[netlify]

Deploy preview for carbon-elements ready!

Built with commit e429737

https://deploy-preview-5039--carbon-elements.netlify.com

[netlify]

Deploy preview for carbon-components-react failed.

Built with commit e429737

https://app.netlify.com/sites/carbon-components-react/deploys/5e2b35c673e6ab000a865434

[joshblack]

Hey @tw15egan! 👋 We were actually taking a look at this today in the morning 👀 When looking at the DAP violation, it seemed like instead we need to remove our usage of checkbox and instead provide something that visually is identical but without the underlying checkbox <input>

Deque has a demo with their custom multiselect icon over in: https://dequelabs.github.io/combobo/demo/ that we were referencing.

The whole reason a checkbox itself isn't necessary is that aria-selected will indicate whether an item is selected for a screen reader user and the visual representation of the selection will be enough for the keyboard user as they don't need tab stops created from the checkboxes.

[tw15egan]

So something like this? We get the check functionality but don't actually use the Checkbox

<div
    className="bx--checkbox-label"
    data-contained-checkbox-state={isChecked}
    id={`${itemProps.id}__checkbox`}>
    {itemText}
 </div>

[tw15egan]

@joshblack updated to use a new element instead of a Checkbox

[joshblack]

@tw15egan sorry for the delay! Yeah, I think that'd be perfect but will bump @dakahn just to confirm 👍

[asudoh]

@dakahn Could you review @tw15egan's latest code? Thanks!

[dakahn]

Did a pass with VoiceOver and NVDA on Windows 10 and macOS. Controls work as expected and reads quite well everywhere but Firefox, but it seems like that's an existing bug that no ones noticed yet unrelated to this PR in particular. 🤦‍♂

DAP errors gone -- that's awesome. If we decided to remove role=group as well I can recheck real quick.

[tw15egan]

Alright made some modifications as I went through it with voiceover on my mac. Still not sure how to alert that an option has been selected/checked. DAP errors should still be resolved

[dakahn]

quick run through with NVDA on Windows 10 in Chrome and Edge and it looks like in Chrome I can no longer access the options menu and in Edge I can, but there is no focus or indication

[tw15egan]

Would that be due to adding the role and aria-selected? Would it be better to just remove them?

[dakahn]

lemme check VO real quick

[dakahn]

It's working with VoiceOver in Safari and Chrome, but now the option menu is no longer working on Windows 10 in Chrome 😕

In Edge it works, but there's no indications or where in the menu you are (focus states) -- you can just see the menu scrolling as the options are read. Something in the last round of changes.

[asudoh]

CC @carmacleod just in case she has some insights on using native checkbox for multi select dropdown's selection.

[carmacleod]

@asudoh

using native checkbox for multi select dropdown's selection.

A quick test of DAP and Windows Chrome/FF/CrEdge with NVDA/JAWS, and Mac Safari with VO, on the checkbox list in Example 2 on this page seems ok. Of course, that's just a very quick test. Checkboxes in the example are native, with opacity 0 and svg image.

I realize that the example listbox is not in a dropdown, but hopefully that won't change things? Make sure you use css visibility: hidden on the dropdown to completely hide it when it isn't showing (i.e. don't use html hidden or css display: none because the animation may not be smooth, and don't use opacity 0 on the dropdown or position it offscreen because that doesn't really hide it from keyboard or screen readers).

Note when using the new Chromium-based-Edge with screen readers, it helps a lot to turn off UIA.

Hope this is helpful!

[asudoh]

@carmacleod Thank you for your insights! @tw15egan Does the guidance from @carmacleod make sense? Thanks!

[joshblack]

We specifically hit the fieldset violation from DAP when using checkboxes which is what the removal of native checkboxes is for. The specific rule that is flagged is: https://aat.w3ibm.mybluemix.net/token/1899dd76-e350-4eef-aaea-8d7f41a398f6/807c38b0-4c39-41db-9d71-72230dbc0eb5/archives/2019SeptDeploy//doc/w3/help/en-US/idhi_accessibility_check_g1029.html

Ultimately, in a listbox we do not require native checkboxes to indicate selection status as this is done through aria-selected="true" on the item that has role="option". The reference for this is WAI ARIA's listbox design pattern: https://www.w3.org/TR/wai-aria-practices/#listbox_roles_states_props specifically:

• All selected options have aria-selected set to true.
• All options that are not selected have aria-selected set to false.

[carmacleod]

We specifically hit the fieldset violation from DAP when using checkboxes

fieldset?! Wild. DAP shouldn't be flagging that for checkboxes in a listbox or table.
(If you wanted to go back to native, you could open a DAP issue - it would be semantically weird and inconsistent to require a fieldset inside a listbox).

in a listbox we do not require native checkboxes to indicate selection status as this is done through aria-selected="true" on the item that has role="option".

Yep - that is a totally fine solution, too! :)

[tw15egan]

@carmacleod Ok, awesome! Good to know this is a valid solution as well.

@joshblack added in aria-selected and changed it to role="option". Let me know if you want any other changes.

@dakahn Let me know what further voiceover work we want to include in this PR, or if we should open a sperate issue for that.

[carmacleod]

Make sure you have aria-multiselectable="true" on the element with role="listbox". :)

[asudoh]

@joshblack @dakahn Any further comments?

Update 1/24/2020: Pinged them offline.

[joshblack]

Looks great! 🎉