feat(Tag): support click handler for filter tag close icon #5470

emyarod · 2020-02-27T21:28:15Z

Closes #5419

This PR adds support for a click handler on the filter tag close icon

Changelog

New

onClose prop support for filter tag

Testing / Reviewing

Ensure that the proper events are fired depending on the click target in filter tags

emyarod · 2020-02-27T21:31:21Z

now that there are 2 distinct click targets for filter tags, should there be an extra tab stop? if so, should there be a visual difference between the tag body on focus and the tag close icon on focus? cc @aagonzales

netlify · 2020-02-27T21:40:26Z

Deploy preview for carbon-components-react ready!

Built with commit 8390593

https://deploy-preview-5470--carbon-components-react.netlify.com

netlify · 2020-02-27T21:43:26Z

Deploy preview for carbon-elements ready!

Built with commit 8390593

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

aagonzales

It looks like clicking on tag area is still activating the x.

aagonzales · 2020-02-27T22:34:40Z

And I don't think the x should add a tab stop, but it should be trigger on keyboard nav by clicking esc and/or allow arrow + enter. I'm not sure what would be the best practice.

emyarod · 2020-02-27T22:41:25Z

it only appears that the x is focused, but it does not fire the same event:

so I'm wondering if there should be a visual difference between tag body focus and close icon focus. but since you said there shouldn't be an added tab stop the current focus behavior of outlining the close icon may be confusing

aagonzales

Filter tag color knob isn't working
Different focus states for tag area and close x
I would not think the x would be in the tab order, I think it would get excessive to read tab label then read the close function for every tab if there are many. I will defer to the accessibility team on this however. @dakahn any thoughts?

emyarod · 2020-03-02T19:54:16Z

FYI for filter tag color knob #3318

I think there would need to be an extra tab stop if we will differentiate tag body actions from close icon actions, otherwise the full component functionality wouldn't be available to keyboard only users (unless pressing Esc to close while the tag body is focused is sufficient), will await updated guidance

dakahn · 2020-03-02T22:00:22Z

Agreed @emyarod any mouse functionality should have a keyboard only alternative as well

abbeyhrt · 2020-03-03T17:36:01Z

Is there any reason that the tag area needs focus? If it isn't going to dismiss the tag then the only thing that needs to receive focus accessibility-wise would be the close icon, I believe.

aagonzales · 2020-03-03T17:50:28Z

No, I don't think the content needs a focus I just don't want the focus for the x to appear when the user clicks on the content area. The user must click on the x to close the icon and it seems confusing that the focus would appear when not clicking directly on the x.

This is the behavior that needs to be corrected:

emyarod · 2020-03-03T17:52:23Z

ok so in that case we are just moving the only clickable area from the tag body to the close icon?

aagonzales · 2020-03-03T17:56:27Z

Yes

emyarod · 2020-03-03T17:58:05Z

ok since the tag body is currently a button element, by default it will have a tab stop. in that case I will change the base element of the tag body to something else. this may be a breaking change if users have been attaching click handlers to the tag body so we will have to announce this change

asudoh · 2020-03-03T23:51:17Z

@emyarod Is this ready for re-review...? Thanks!

emyarod · 2020-03-04T00:17:50Z

@asudoh yes this is ready for re-review

aagonzales

Looks good now!

abbeyhrt · 2020-03-04T17:11:45Z

packages/components/src/components/tag/_tag.scss


    &:hover {
      background-color: $inverse-hover-ui;
    }
  }

-  .#{$prefix}--tag--filter:focus > svg {
+  .#{$prefix}--tag__close-icon svg {
+    fill: $inverse-01;


For my own edification, why did you pull this out? I see it done a lot throughout our styles and I just wanted to learn why, thanks! 😄

are you referring to the class (.bx--tag--filter to .bx--tag__close-icon)? or the > selector?

abbeyhrt

Looks good to me! 🎉

joshblack · 2020-03-04T21:57:18Z

packages/react/src/components/Tag/Tag.js

@@ -53,8 +58,10 @@ const Tag = ({
      <span className={`${prefix}--tag__label`}>
        {children !== null && children !== undefined ? children : TYPES[type]}
      </span>
-      <Close16 />
-    </button>
+      <button className={`${prefix}--tag__close-icon`} onClick={handleClose}>


With the button shifting over will we also need to move the aria-label over?

not sure but the aria label was removed from the close button to the tag body in #4863

asudoh · 2020-03-05T22:16:39Z

@joshblack Any further comments...? Thanks!

dakahn

Tested with JAWS 2020 on Windows 10. The button is read as "clear filter this is not a tag unlabeled 1 button"

Looks like the button under the hood has no label

Worth noting as well is NVDA reads this as "main landmark clickable button" -- so not even picking up the tag text at all there.

And we've also got the expected subsequent DAP error

Adding an aria-labelledby attribute referencing an id on that wrapper div with the aria-label seems to fix both JAWS and NVDA and clear the DAP error though 🎉

dakahn

Sounds good and reads well 👍 DAP errors cleared

emyarod added the status: ready for review 👀 label Feb 27, 2020

emyarod requested a review from a team as a code owner February 27, 2020 21:28

ghost requested review from abbeyhrt and dakahn February 27, 2020 21:28

emyarod requested a review from aagonzales February 27, 2020 21:31

aagonzales reviewed Feb 27, 2020

View reviewed changes

emyarod added status: visual review 🔍 status: waiting for reviewer's response 💬 labels Feb 27, 2020

emyarod requested a review from aagonzales February 28, 2020 19:20

aagonzales requested changes Mar 2, 2020

View reviewed changes

emyarod force-pushed the 5419-filter-tag-close-handler branch from d4d39e6 to 8f3b32f Compare March 2, 2020 19:57

emyarod force-pushed the 5419-filter-tag-close-handler branch from 8f3b32f to 6621f3d Compare March 3, 2020 18:16

emyarod removed the status: waiting for reviewer's response 💬 label Mar 3, 2020

emyarod requested a review from aagonzales March 3, 2020 18:17

emyarod force-pushed the 5419-filter-tag-close-handler branch from 6621f3d to 4f146b3 Compare March 3, 2020 18:17

aagonzales approved these changes Mar 4, 2020

View reviewed changes

emyarod removed the status: visual review 🔍 label Mar 4, 2020

abbeyhrt reviewed Mar 4, 2020

View reviewed changes

abbeyhrt approved these changes Mar 4, 2020

View reviewed changes

joshblack reviewed Mar 4, 2020

View reviewed changes

emyarod force-pushed the 5419-filter-tag-close-handler branch from 4f146b3 to 7422043 Compare March 5, 2020 16:18

dakahn suggested changes Mar 5, 2020

View reviewed changes

emyarod force-pushed the 5419-filter-tag-close-handler branch from 7422043 to a368d5f Compare March 6, 2020 14:51

emyarod requested a review from dakahn March 6, 2020 15:19

emyarod added 4 commits March 10, 2020 10:15

feat(Tag): support click handler for filter tag close icon

ee91a29

refactor(tag): move interactivity styles to close button

3dd0a79

fix(Tag): add aria-labelledby

2d7558e

chore: update snapshot

a3eb65d

emyarod force-pushed the 5419-filter-tag-close-handler branch from a368d5f to a3eb65d Compare March 10, 2020 15:15

Merge branch 'master' into 5419-filter-tag-close-handler

cf9278d

dakahn approved these changes Mar 10, 2020

View reviewed changes

Merge branch 'master' into 5419-filter-tag-close-handler

8390593

tw15egan removed the status: ready for review 👀 label Mar 10, 2020

tw15egan approved these changes Mar 10, 2020

View reviewed changes

asudoh merged commit 57f48c0 into carbon-design-system:master Mar 10, 2020

emyarod deleted the 5419-filter-tag-close-handler branch March 11, 2020 16:08

joshblack mentioned this pull request Mar 31, 2020

chore(release): v10.11.0-rc.0 #5762

Closed

janhassel mentioned this pull request May 1, 2020

fix(tag): pass props.disabled to close button #5996

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Tag): support click handler for filter tag close icon #5470

feat(Tag): support click handler for filter tag close icon #5470

emyarod commented Feb 27, 2020

emyarod commented Feb 27, 2020

netlify bot commented Feb 27, 2020 •

edited

Loading

netlify bot commented Feb 27, 2020 •

edited

Loading

aagonzales left a comment

aagonzales commented Feb 27, 2020 •

edited

Loading

emyarod commented Feb 27, 2020

aagonzales left a comment

emyarod commented Mar 2, 2020

dakahn commented Mar 2, 2020

abbeyhrt commented Mar 3, 2020

aagonzales commented Mar 3, 2020

emyarod commented Mar 3, 2020 •

edited

Loading

aagonzales commented Mar 3, 2020

emyarod commented Mar 3, 2020

asudoh commented Mar 3, 2020

emyarod commented Mar 4, 2020

aagonzales left a comment •

edited

Loading

abbeyhrt Mar 4, 2020 •

edited

Loading

emyarod Mar 4, 2020 •

edited

Loading

abbeyhrt left a comment

joshblack Mar 4, 2020

emyarod Mar 5, 2020

asudoh commented Mar 5, 2020

dakahn left a comment •

edited

Loading

dakahn left a comment •

edited

Loading

feat(Tag): support click handler for filter tag close icon #5470

feat(Tag): support click handler for filter tag close icon #5470

Conversation

emyarod commented Feb 27, 2020

Changelog

Testing / Reviewing

emyarod commented Feb 27, 2020

netlify bot commented Feb 27, 2020 • edited Loading

netlify bot commented Feb 27, 2020 • edited Loading

aagonzales left a comment

Choose a reason for hiding this comment

aagonzales commented Feb 27, 2020 • edited Loading

emyarod commented Feb 27, 2020

aagonzales left a comment

Choose a reason for hiding this comment

emyarod commented Mar 2, 2020

dakahn commented Mar 2, 2020

abbeyhrt commented Mar 3, 2020

aagonzales commented Mar 3, 2020

emyarod commented Mar 3, 2020 • edited Loading

aagonzales commented Mar 3, 2020

emyarod commented Mar 3, 2020

asudoh commented Mar 3, 2020

emyarod commented Mar 4, 2020

aagonzales left a comment • edited Loading

Choose a reason for hiding this comment

abbeyhrt Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

emyarod Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

abbeyhrt left a comment

Choose a reason for hiding this comment

joshblack Mar 4, 2020

Choose a reason for hiding this comment

emyarod Mar 5, 2020

Choose a reason for hiding this comment

asudoh commented Mar 5, 2020

dakahn left a comment • edited Loading

Choose a reason for hiding this comment

dakahn left a comment • edited Loading

Choose a reason for hiding this comment

netlify bot commented Feb 27, 2020 •

edited

Loading

netlify bot commented Feb 27, 2020 •

edited

Loading

aagonzales commented Feb 27, 2020 •

edited

Loading

emyarod commented Mar 3, 2020 •

edited

Loading

aagonzales left a comment •

edited

Loading

abbeyhrt Mar 4, 2020 •

edited

Loading

emyarod Mar 4, 2020 •

edited

Loading

dakahn left a comment •

edited

Loading

dakahn left a comment •

edited

Loading