feat(floating-menu): add focus wrap element to components #3652

elizabethsjudd · 2019-08-05T20:29:18Z

Move the focusable element used for the focus wrap feature to be within the component so developers don't have to add a hidden element to get this feature. This impacts modal, tooltip, and overflow menu. Waiting on #3476 to be merged so I can test with the new functionality of tooltip.

Changelog

Changed

floating menu now supports a wrapper node around the floating menu that contains a focusable span used to detect menu blur
modal demo updated
overflow menu and tooltip demos updated and vanilla JS updated to support the new wrapper element

Testing / Reviewing

Test overflow menu, modal, and tooltip keyboard interactions

netlify · 2019-08-05T20:38:32Z

Deploy preview for the-carbon-components ready!

Built with commit abf3402

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

netlify · 2019-08-05T20:40:46Z

Deploy preview for carbon-elements ready!

Built with commit abf3402

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

netlify · 2019-08-05T20:41:11Z

Deploy preview for carbon-components-react ready!

Built with commit abf3402

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

asudoh

Thank you for coming up with a PR @elizabethsjudd!

asudoh · 2019-08-05T22:19:12Z

packages/components/src/components/modal/modal.hbs

-        {{ carbon-icon 'Close16' class=(add @root.prefix '--modal-close__icon') }}
-      </button>
-    </div>
+<div>


Why did you put additional <div> rather than simply adding focus trap element to the last? Note that no user-visible focusable element should be put outside <div class="bx--modal-container">.

packages/components/src/components/overflow-menu/overflow-menu.hbs

asudoh · 2019-08-05T22:22:27Z

packages/components/src/components/tooltip/tooltip.hbs

-    <a href="#" class="{{@root.prefix}}--link">Learn More</a>
-    <button class="{{@root.prefix}}--btn {{@root.prefix}}--btn--primary {{@root.prefix}}--btn--sm"
-      type="button">Create</button>
+<div class="{{@root.prefix}}--tooltip-wrapper">


Same question to overflow menu - Would you put the trap element next to <div class="bx--tooltip__footer">?

@asudoh the trap element needs to be outside of the content area of the tooltip not a sibling within it. Currently with the way that the trap element works, it can not be a direct sibling of the content area which in the case of tooltips content is any direct child of .bx--tooltip

Current structure of tooltip

<bx--tooltip> <bx--tooltip__caret/> <p>Tooltip content</p> <bx--tooltip__footer/>  <trap-element> </bx--tooltip>

There are a couple different options:

1. Scope the content area

We could add a div within the tooltip around any of the content including the footer and update the event listener to watch the new div instead of the tooltip element

<bx--tooltip> <bx--tooltip__caret/> <div> <p>Tooltip content</p> <bx--tooltip__footer/> </div> <trap-element> </bx--tooltip>

2. Add a unique selector to the trap element

We could give a unique selector to the trap element and watch for it to get focus and when it gets focus, immediate move it. If that's the case we can keep the original HTML structure.

<bx--tooltip> <bx--tooltip__caret/> <p>Tooltip content</p> <bx--tooltip__footer/> <span tabindex="0" class="bx--focus-trap"></span> </bx--tooltip>

My apologies, I seems to have missed this comment. I think 1. makes sense. BTW instead of wrapper node ref, let's move the root node up to the wrapper as above structure tells. If we need to refer to where the root node used to be, we can introduce options.selectorContent. Thanks!

asudoh · 2019-08-05T22:22:50Z

packages/components/src/components/tooltip/tooltip.hbs

-    <a href="#" class="{{@root.prefix}}--link">Learn More</a>
-    <button class="{{@root.prefix}}--btn {{@root.prefix}}--btn--primary {{@root.prefix}}--btn--sm"
-      type="button">Create</button>
+<div class="{{@root.prefix}}--tooltip-wrapper">


Some question as another instance of tooltip body.

asudoh · 2019-08-05T22:22:55Z

packages/components/src/components/tooltip/tooltip.hbs

-    <a href="#" class="{{@root.prefix}}--link">Learn More</a>
-    <button class="{{@root.prefix}}--btn {{@root.prefix}}--btn--primary {{@root.prefix}}--btn--sm"
-      type="button">Create</button>
+<div class="{{@root.prefix}}--tooltip-wrapper">


Some question as another instance of tooltip body.

elizabethsjudd · 2019-08-06T14:05:17Z

@asudoh here's the thought process behind my work

the focusable span needs to move with the floating menu so it's less DOM manipulation to grab a container that contains both the menu and the focusable tab vs having to grab both elements separately to move them
most frameworks require a root element so by not having a containing div for some components that created sibling elements at the root and we should keep consistent HTML between vanilla and the other JS frameworks (React, Angular, Vue, etc)
I thought the span has to be completely outside of the component (affected modal and tooltip)

asudoh · 2019-08-07T01:53:24Z

Thank you for explaining @elizabethsjudd. 1. makes sense to me and I'll withdraw my comments to overflow menu due to <ul>/<li> semantics.

Wrt. 2., you are right that Angular, etc. requires root element. Modal already has two-level containers, though - One (<div class="bx--modal">) works as the root and the backdrop, another (<div class="bx--modal-container">) works as the container of the content. Given we don't have content in backdrop area, putting hidden content in the backdrop area makes more sense. An example can be seen here.

Wrt 3., I guess you meant where the content is put in with your "in component" wording. If so, my response to 2. would address it.

…feature/floating-menu-focus-wrap

elizabethsjudd · 2019-08-08T17:16:35Z

@asudoh I made changes to each of the components so the span is contained within the component and added the selectorContent feature to floating menus.

elizabethsjudd · 2019-08-08T17:23:29Z

@asudoh While I'm working on some of the more JS extensive issues for a11y, I feel like it might be easier for me to work in the evening/night so that we are on an overlapping schedule so that I can I can get feedback quicker. Right now the issue that I'm having is that I'm spending an hour or so a day working on tasks and then waiting for you to be able to respond. I recognize that with a 13 hour time difference this is expected but as I have more issues open I'm building more merge conflicts (which are never fun). I've already ran the idea of working one day a week with this different schedule with my team and they agree with this shift to help with communication and hopefully speed up the contribution process. Can we coordinate a day/times for me to do this schedule shift?

asudoh · 2019-08-08T21:24:29Z

The updated code looks basically good to me. Could you update tests? BTW thanks for ideas on collaboration, I pinged you offline for my ideas.

…feature/floating-menu-focus-wrap

elizabethsjudd · 2019-08-12T17:48:42Z

@asudoh tests have been updated and ready for re-review

asudoh · 2019-08-12T20:55:55Z

Cool thanks @elizabethsjudd! Would you want to cover your enhancements in the test as well?

elizabethsjudd · 2019-08-12T21:30:32Z

@asudoh the tests for the focus wrap feature seem to already be in there because the HTML has the old hidden input. Are you wanting me to test for the presence of the element or what?

asudoh

Oh I didn't realize that, sorry @elizabethsjudd!

elizabethsjudd · 2019-08-26T17:57:27Z

@carbon-design-system/developers This is still waiting on review.

elizabethsjudd · 2019-09-04T20:21:45Z

@tw15egan or @emyarod can one of you review this so it can get merged? TIA

jnm2377 · 2019-09-04T20:48:54Z

Will take a look at this!

@elizabethsjudd can you update the PR title? It should follow our commit message conventions for our changelog. You can read more about this in our developer handbook.

tw15egan

LGTM -- sorry for delays!

dakahn

Thanks so much for being patient our new(ish) system for assigning PRs to reviewers still has a few kinks to work out.

elizabethsjudd · 2019-09-04T21:13:36Z

Thanks for taking a look @tw15egan and @dakahn!

liz.judd added 5 commits August 5, 2019 16:22

feat(vanilla): remove global hidden input

47e6aaa

feat(floating-menu): support focus wrap within component

ff5325f

feat(modal): update demo with inline focus wrap

41c4025

feat(overflow-menu): inline focus wrap

42a5ee5

feat(tooltip): inline focus wrap

e4e3d4a

asudoh reviewed Aug 5, 2019

View reviewed changes

liz.judd added 7 commits August 7, 2019 13:15

Merge branch 'master' of github.com:carbon-design-system/carbon into …

562d94c

…feature/floating-menu-focus-wrap

fix(modal): remove need for extra div

f824b39

Merge branch 'master' of github.com:carbon-design-system/carbon into …

53c9f79

…feature/floating-menu-focus-wrap

fix(floating-menu): support inline focus wrap

1a634f5

fix(overflow-menu): uses inline focus element

9c235c5

fix(tooltip): uses inline focus element

6c5f857

Merge branch 'master' into feature/floating-menu-focus-wrap

fb23775

liz.judd added 2 commits August 12, 2019 13:30

Merge branch 'master' of github.com:carbon-design-system/carbon into …

45b4fce

…feature/floating-menu-focus-wrap

fix(floating-menu): update test selectors

d325877

elizabethsjudd marked this pull request as ready for review August 12, 2019 17:48

asudoh approved these changes Aug 12, 2019

View reviewed changes

Merge branch 'master' into feature/floating-menu-focus-wrap

9c69195

Merge branch 'master' into feature/floating-menu-focus-wrap

0c5b828

asudoh added the status: one more review 👀 label Aug 26, 2019

elizabethsjudd added 2 commits August 29, 2019 15:36

Merge branch 'master' into feature/floating-menu-focus-wrap

3bb9e4c

Merge branch 'master' into feature/floating-menu-focus-wrap

47e60d8

joshblack requested a review from a team September 4, 2019 20:39

ghost requested review from aledavila and dakahn and removed request for a team September 4, 2019 20:39

jnm2377 requested review from a team and removed request for aledavila and dakahn September 4, 2019 20:42

ghost requested review from jnm2377 and vpicone and removed request for a team September 4, 2019 20:42

Merge branch 'master' into feature/floating-menu-focus-wrap

41360ff

elizabethsjudd changed the title ~~floating menu focus wrap~~ feat(floating-menu): add focus wrap element to components Sep 4, 2019

Merge branch 'master' into feature/floating-menu-focus-wrap

d5b347b

tw15egan approved these changes Sep 4, 2019

View reviewed changes

dakahn approved these changes Sep 4, 2019

View reviewed changes

Merge branch 'master' into feature/floating-menu-focus-wrap

abf3402

joshblack added status: ready to merge 🎉 and removed status: one more review 👀 labels Sep 4, 2019

joshblack merged commit 34d1fdd into carbon-design-system:master Sep 4, 2019

elizabethsjudd deleted the feature/floating-menu-focus-wrap branch September 6, 2019 16:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(floating-menu): add focus wrap element to components #3652

feat(floating-menu): add focus wrap element to components #3652

elizabethsjudd commented Aug 5, 2019

netlify bot commented Aug 5, 2019 •

edited

Loading

netlify bot commented Aug 5, 2019 •

edited

Loading

netlify bot commented Aug 5, 2019 •

edited

Loading

asudoh left a comment

asudoh Aug 5, 2019

asudoh Aug 5, 2019

elizabethsjudd Aug 7, 2019

asudoh Aug 7, 2019

asudoh Aug 5, 2019

asudoh Aug 5, 2019

elizabethsjudd commented Aug 6, 2019

asudoh commented Aug 7, 2019

elizabethsjudd commented Aug 8, 2019

elizabethsjudd commented Aug 8, 2019

asudoh commented Aug 8, 2019

elizabethsjudd commented Aug 12, 2019

asudoh commented Aug 12, 2019

elizabethsjudd commented Aug 12, 2019

asudoh left a comment

elizabethsjudd commented Aug 26, 2019

elizabethsjudd commented Sep 4, 2019

jnm2377 commented Sep 4, 2019

tw15egan left a comment

dakahn left a comment

elizabethsjudd commented Sep 4, 2019

feat(floating-menu): add focus wrap element to components #3652

feat(floating-menu): add focus wrap element to components #3652

Conversation

elizabethsjudd commented Aug 5, 2019

Changelog

Testing / Reviewing

netlify bot commented Aug 5, 2019 • edited Loading

netlify bot commented Aug 5, 2019 • edited Loading

netlify bot commented Aug 5, 2019 • edited Loading

asudoh left a comment

Choose a reason for hiding this comment

asudoh Aug 5, 2019

Choose a reason for hiding this comment

asudoh Aug 5, 2019

Choose a reason for hiding this comment

elizabethsjudd Aug 7, 2019

Choose a reason for hiding this comment

asudoh Aug 7, 2019

Choose a reason for hiding this comment

asudoh Aug 5, 2019

Choose a reason for hiding this comment

asudoh Aug 5, 2019

Choose a reason for hiding this comment

elizabethsjudd commented Aug 6, 2019

asudoh commented Aug 7, 2019

elizabethsjudd commented Aug 8, 2019

elizabethsjudd commented Aug 8, 2019

asudoh commented Aug 8, 2019

elizabethsjudd commented Aug 12, 2019

asudoh commented Aug 12, 2019

elizabethsjudd commented Aug 12, 2019

asudoh left a comment

Choose a reason for hiding this comment

elizabethsjudd commented Aug 26, 2019

elizabethsjudd commented Sep 4, 2019

jnm2377 commented Sep 4, 2019

tw15egan left a comment

Choose a reason for hiding this comment

dakahn left a comment

Choose a reason for hiding this comment

elizabethsjudd commented Sep 4, 2019

netlify bot commented Aug 5, 2019 •

edited

Loading

netlify bot commented Aug 5, 2019 •

edited

Loading

netlify bot commented Aug 5, 2019 •

edited

Loading