fix(listbox): use filter tag #5952

janhassel · 2020-04-28T07:09:59Z

Use <Tag /> component in ListBoxes instead of custom styles to ensure styling is up-to-date.

Changelog

New

Included Tag in ListBoxSelection

Removed

Custom styling for "tag clone" in ListBoxes
handleOnKeyDown in ListBoxSelection (see question below)

Testing / Reviewing

Automated tests should make sure functionality isn't affected. Visually, new implementation and old implementation should look the same.

The tests are currently failing since I removed the current keydown logic. Tbh, checking out the current implementation I am a bit confused about the keyboard navigation and therefore wanted to see if that might need to be updated before re-creating it.

janhassel · 2020-04-28T07:15:21Z

Elaborating on the keyboard question, I'm wondering if...

the listbox should open/close on Enter as well (like a dropdown)
the listbox should close on blur (like a dropdown)
the tabstop on the list of options can be omitted

netlify · 2020-04-28T07:18:50Z

Deploy preview for carbon-elements ready!

Built with commit d62061a

https://deploy-preview-5952--carbon-elements.netlify.app

netlify · 2020-04-28T07:25:32Z

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit d62061a

https://deploy-preview-5952--carbon-components-react.netlify.app

asudoh · 2020-04-28T08:16:19Z

Thanks @janhassel for your contribution. Do you want to resolve style dependency as well? (Note that usage of our entrypoint Sass code is not recommended for apps for performance sake)

the listbox should open/close on Enter as well (like a dropdown)
the listbox should close on blur (like a dropdown)

Agreed personally, though I'd like separate issues/PRs for those

the tabstop on the list of options can be omitted

I imagine so, but I don't see it's keyboard-focusable (only programmatic-focusable)

janhassel · 2020-04-28T12:27:39Z

@asudoh Good point, I improted the tag styles into list-box.scss. Also restored the onKeyDown handler to be on par with the current interaction.

I'll update the snapshots once #5951 is merged as there are some overlaps.

emyarod · 2020-04-28T14:45:30Z

packages/react/src/components/ListBox/ListBoxSelection.js

@@ -49,17 +48,17 @@ const ListBoxSelection = ({
  };
  const description = selectionCount ? t('clear.all') : t('clear.selection');
  return (
-    <div
-      role="button"
+    <Tag


I think we can add the disabled prop here so we can get the correct styles when the multiselect is disabled

I was thinking that too and compared it to the current implementation where the tag styling is not changed on disable.

Personally I would also prefer to have the tag disabled since otherwise it really stands out. Do you if there was a reason behind the current implementation, like keeping it well readable for accessibility maybe?

it looks like there is a slight color change on the disabled tag svg in the current multiselect, but I'm not sure if it should look like the standalone disabled tag styles cc @laurenmrice

Just as a reference: this is how it would look when the tag enters disabled state as well:

Let me make a spec for this 👍🏻, will get back to you shortly

This is what we can do for a selected-disabled state.

Thanks for the quick specs, @laurenmrice 👍
I updated the tag color for the disabled state accordingly, you can check it out in the deploy preview: https://5eaae7cd3ab6150007c0340b--carbon-components-react.netlify.app/?path=/story/multiselect--default

dakahn · 2020-04-29T03:00:51Z

Elaborating on the keyboard question, I'm wondering if...

the listbox should open/close on Enter as well (like a dropdown)

the listbox should close on blur (like a dropdown)

the tabstop on the list of options can be omitted

With a combobox specifically:

neither Enter or Spacebar should open or close the listbox dropdown. Only either A) entering a text value into the field or in some cases B) hitting ArrowDown should open the listbox. This keyboard interaction is described in detail at this link.
the listbox should close on blur
Although they're not tabstops, focus should move to the option being highlighted in the listbox dialog as described here under the Keyboard Support section

The current Carbon Filterable Multiselect component is far more complex and includes many unexpected features and interactions (which can/have proven problematic for our screen reader and keyboard users), but when triaging or remediating accessibility bugs moving forward we'll reference the WAI-ARIA Authoring Practices Guidelines for how to proceed.

dakahn

The button is keyboard accessible 🥂. The only point of contention I have is that it's read by JAWS 2020 and NVDA as "Clear selection button" without any information about the selected items the user would be clearing by activating it.
There also seems to be some styling that's off in Chrome latest on Windows 10

janhassel · 2020-04-30T11:51:51Z

@dakahn Thanks for catching that, I fixed the styling, should be correctly aligned now.

Regarding the first point: I removed the explicit aria-label and VoiceOver now reads "Clear all 2, button". Sadly I currently don't have access to our windows machine to test with JAWS, could you see if it works there as well?

laurenmrice

Hey Jan, just one thing: When disabled, the tag should also be getting the disabled cursor.

janhassel · 2020-05-01T07:11:17Z

@laurenmrice Good point. Updated. https://deploy-preview-5952--carbon-components-react.netlify.app/?path=/story/multiselect--default

laurenmrice · 2020-05-01T15:17:36Z

It still seems to be showing up on the x

laurenmrice · 2020-05-01T15:19:02Z

Also I am not sure if this is related to this pr (its not happening in the live react version) but when filtering a multiselect, the closex is using a tag.

janhassel · 2020-05-04T14:19:28Z

@laurenmrice The issue with the X button on the filter will be addressed in #5996
Thanks for noticing the X button issue in the filterable multiselect, this is indeed caused by the changes in this PR, I'll make sure to fix it!

laurenmrice

This looks good to me. thank you Jan! 👍🏻

janhassel · 2020-06-02T08:00:17Z

@dakahn I'm wondering what the best strategy would be for this issue: validateDOMNesting(...): <button> cannot appear as a descendant of <button>

The Tag component uses a <button> for its remove button. The .bx--list-box__field element is also a <button> and contains a tag component. Therefore the tests report invalid DOM nesting. On master, the tag appearing inside the MultiSelect is a <div> with role="button" which makes it technically valid.

I see four main ways to go from here:

Discard this PR, leave the custom tag

This might result in inconsistencies (both visual and interaction) between the actual Tag component and the tag displayed in MultiSelect component.

Modify the Tag component

Instead of using a <button> for the remove button, we could use a <div> with role="button", basically making the Tag component behave as the custom tag behaved before.

Modify the ListBox field element

Same as with the tag: use a div instead of a button element. Might bring problems with downshift (not tested/confirmed)

Unnest tag from field

One obvious solution would be to unnest the two elements, this would bring in complexity to account for the right styling when a tag is displayed vs. when there is no tag.

I'm unsure which of these options would be the best. Maybe I'm also missing an obvious one here?
What do you think?

joshblack · 2020-06-22T20:18:16Z

Hey @janhassel! Sorry for the delay in response, this is definitely a problem with the main widget with the button in a button. bump @dakahn again if you have any thoughts on the above questions and what we would need to do.

dakahn · 2020-06-22T21:59:52Z

First option seems like the safest path of least resistance to me. Buttons with descendant buttons make multiple appearances in our code base sadly. We can come up with ways to address any hypothetical future visual inconsistency. Refactoring to a div isn't out of the question, but that will require careful work to ensure we don't break/degrade the experience for our screen reader users across the types of readers, browsers, and OSs' we build for.

dakahn · 2020-06-22T22:56:06Z

related #5880

janhassel · 2020-06-29T07:36:19Z

Ok, that sounds reasonable! Feel free to close the PR in this case.

If this is an overall accessibility issue with other components as well I'd be interested in how this will be addressed. Is it something you guys have on the roadmap for v11 or later?

dakahn · 2020-06-29T23:12:13Z

Honestly, we need to rethink fundamental design decisions around element layout in a number of components with nested interactables. Buttons in buttons is a very pressing issue that we should get to way before v11 if I have my way.

fix(listbox): use filter tag

0dd3e5e

janhassel requested a review from a team as a code owner April 28, 2020 07:09

ghost requested review from dakahn and emyarod April 28, 2020 07:10

asudoh added the status: failing CI 🙅‍♂️ label Apr 28, 2020

fix(listbox): restore onKeyDown handler

8e8bd9c

emyarod reviewed Apr 28, 2020

View reviewed changes

emyarod added the status: visual review 🔍 label Apr 28, 2020

dakahn suggested changes Apr 29, 2020

View reviewed changes

janhassel added 2 commits April 30, 2020 13:44

fix(listbox): fix tag spacing on filterable multiselect

dd1c100

fix(listbox): remove explicit aria-label from tag

2327f5f

janhassel added 6 commits April 30, 2020 14:47

Merge branch 'master' into listbox-tag

2718b37

test(listbox): update snapshots

d859ed4

test(listbox): update snapshots

81d51e5

test(listbox): update selectors

66270e5

fix(listbox): apply correct styling to disabled tag

817907f

chore(listbox): fix prettier issue

b7c779c

laurenmrice requested changes Apr 30, 2020

View reviewed changes

fix(listbox): pass disabled state to tag

481e914

janhassel and others added 6 commits May 4, 2020 16:19

Merge branch 'master' into listbox-tag

dcb9f62

Merge branch 'master' into listbox-tag

5dd96db

fix(multi-select): correct clear input button for filterable multiselect

e90ccfa

Merge branch 'master' into listbox-tag

5400128

chore(multi-select): fix prettier issue

abc0b98

Merge branch 'master' into listbox-tag

d62061a

laurenmrice approved these changes Jun 1, 2020

View reviewed changes

laurenmrice removed the status: visual review 🔍 label Jun 1, 2020

joshblack added the status: waiting for maintainer response 💬 label Jun 22, 2020

dakahn closed this Jun 29, 2020

janhassel deleted the listbox-tag branch July 21, 2020 15:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(listbox): use filter tag #5952

fix(listbox): use filter tag #5952

janhassel commented Apr 28, 2020

janhassel commented Apr 28, 2020

netlify bot commented Apr 28, 2020 •

edited

Loading

netlify bot commented Apr 28, 2020 •

edited

Loading

asudoh commented Apr 28, 2020

janhassel commented Apr 28, 2020

emyarod Apr 28, 2020

janhassel Apr 28, 2020

emyarod Apr 28, 2020

janhassel Apr 30, 2020

laurenmrice Apr 30, 2020 •

edited

Loading

laurenmrice Apr 30, 2020

janhassel Apr 30, 2020

dakahn commented Apr 29, 2020 •

edited

Loading

dakahn left a comment •

edited

Loading

janhassel commented Apr 30, 2020

laurenmrice left a comment

janhassel commented May 1, 2020

laurenmrice commented May 1, 2020

laurenmrice commented May 1, 2020 •

edited

Loading

janhassel commented May 4, 2020

laurenmrice left a comment

janhassel commented Jun 2, 2020

joshblack commented Jun 22, 2020

dakahn commented Jun 22, 2020

dakahn commented Jun 22, 2020

janhassel commented Jun 29, 2020

dakahn commented Jun 29, 2020

fix(listbox): use filter tag #5952

fix(listbox): use filter tag #5952

Conversation

janhassel commented Apr 28, 2020

Changelog

Testing / Reviewing

janhassel commented Apr 28, 2020

netlify bot commented Apr 28, 2020 • edited Loading

netlify bot commented Apr 28, 2020 • edited Loading

asudoh commented Apr 28, 2020

janhassel commented Apr 28, 2020

emyarod Apr 28, 2020

Choose a reason for hiding this comment

janhassel Apr 28, 2020

Choose a reason for hiding this comment

emyarod Apr 28, 2020

Choose a reason for hiding this comment

janhassel Apr 30, 2020

Choose a reason for hiding this comment

laurenmrice Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

laurenmrice Apr 30, 2020

Choose a reason for hiding this comment

janhassel Apr 30, 2020

Choose a reason for hiding this comment

dakahn commented Apr 29, 2020 • edited Loading

dakahn left a comment • edited Loading

Choose a reason for hiding this comment

janhassel commented Apr 30, 2020

laurenmrice left a comment

Choose a reason for hiding this comment

janhassel commented May 1, 2020

laurenmrice commented May 1, 2020

laurenmrice commented May 1, 2020 • edited Loading

janhassel commented May 4, 2020

laurenmrice left a comment

Choose a reason for hiding this comment

janhassel commented Jun 2, 2020

joshblack commented Jun 22, 2020

dakahn commented Jun 22, 2020

dakahn commented Jun 22, 2020

janhassel commented Jun 29, 2020

dakahn commented Jun 29, 2020

netlify bot commented Apr 28, 2020 •

edited

Loading

netlify bot commented Apr 28, 2020 •

edited

Loading

laurenmrice Apr 30, 2020 •

edited

Loading

dakahn commented Apr 29, 2020 •

edited

Loading

dakahn left a comment •

edited

Loading

laurenmrice commented May 1, 2020 •

edited

Loading