-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat(floating-menu): add focus wrap element to components #3652
Changes from 5 commits
47e6aaa
ff5325f
41c4025
42a5ee5
e4e3d4a
562d94c
f824b39
53c9f79
1a634f5
9c235c5
6c5f857
fb23775
45b4fce
d325877
9c69195
b790e14
092bc7c
0cfdc79
48d72bc
0c5b828
3bb9e4c
47e60d8
41360ff
d5b347b
abf3402
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,20 +15,25 @@ | |
{{ carbon-icon 'Information16' }} | ||
</div> | ||
</div> | ||
{{!-- @todo add aria-hidden once JavaScript can toggle it --}} | ||
<div id="{{noHeading.idSuffix}}" data-floating-menu-direction="bottom" class="{{@root.prefix}}--tooltip" | ||
data-avoid-focus-on-open role="dialog" aria-describedby="{{noHeading.idSuffix}}-body" aria-labelledby="{{noHeading.idSuffix}}-label"> | ||
<span class="{{@root.prefix}}--tooltip__caret"></span> | ||
<p id="{{noHeading.idSuffix}}-body" >This is some tooltip text. This box shows the maximum amount of text that should appear inside. If more room is | ||
needed | ||
please use a modal instead.</p> | ||
<div class="{{@root.prefix}}--tooltip__footer"> | ||
<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"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same question to overflow menu - Would you put the trap element next to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 Current structure of 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
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.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
{{!-- @todo add aria-hidden once JavaScript can toggle it --}} | ||
<div id="{{noHeading.idSuffix}}" data-floating-menu-direction="bottom" class="{{@root.prefix}}--tooltip" | ||
data-avoid-focus-on-open role="dialog" aria-describedby="{{noHeading.idSuffix}}-body" aria-labelledby="{{noHeading.idSuffix}}-label"> | ||
<span class="{{@root.prefix}}--tooltip__caret"></span> | ||
<p id="{{noHeading.idSuffix}}-body" >This is some tooltip text. This box shows the maximum amount of text that should appear inside. If more room is | ||
needed | ||
please use a modal instead.</p> | ||
<div class="{{@root.prefix}}--tooltip__footer"> | ||
<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> | ||
</div> | ||
<!-- Note: focusable span allows for focus wrap feature within Tooltips --> | ||
<span tabindex="0"></span> | ||
</div> | ||
|
||
|
||
{{!-- Tooltip with visible label within tooltip --}} | ||
<div id="{{heading.idSuffix}}-label" class="{{@root.prefix}}--tooltip__label"> | ||
Tooltip label | ||
|
@@ -39,19 +44,22 @@ | |
{{ carbon-icon 'Information16' }} | ||
</div> | ||
</div> | ||
{{!-- @todo add aria-hidden once JavaScript can toggle it --}} | ||
<div id="{{heading.idSuffix}}" data-floating-menu-direction="bottom" class="{{@root.prefix}}--tooltip" | ||
data-avoid-focus-on-open role="dialog" aria-describedby="{{heading.idSuffix}}-body" aria-labelledby="{{heading.idSuffix}}-heading"> | ||
<span class="{{@root.prefix}}--tooltip__caret"></span> | ||
<h4 id="{{heading.idSuffix}}-heading">Heading within a Tooltip</h4> | ||
<p id="{{heading.idSuffix}}-body" >This is some tooltip text. This box shows the maximum amount of text that should appear inside. If more room is | ||
needed | ||
please use a modal instead.</p> | ||
<div class="{{@root.prefix}}--tooltip__footer"> | ||
<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"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some question as another instance of tooltip body. |
||
{{!-- @todo add aria-hidden once JavaScript can toggle it --}} | ||
<div id="{{heading.idSuffix}}" data-floating-menu-direction="bottom" class="{{@root.prefix}}--tooltip" | ||
data-avoid-focus-on-open role="dialog" aria-describedby="{{heading.idSuffix}}-body" aria-labelledby="{{heading.idSuffix}}-heading"> | ||
<span class="{{@root.prefix}}--tooltip__caret"></span> | ||
<h4 id="{{heading.idSuffix}}-heading">Heading within a Tooltip</h4> | ||
<p id="{{heading.idSuffix}}-body" >This is some tooltip text. This box shows the maximum amount of text that should appear inside. If more room is | ||
needed | ||
please use a modal instead.</p> | ||
<div class="{{@root.prefix}}--tooltip__footer"> | ||
<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> | ||
</div> | ||
<span tabindex="0"></span> | ||
</div> | ||
|
||
{{!-- Tooltip without visible label (not recommended for WCAG 2.1) --}} | ||
|
@@ -64,16 +72,19 @@ | |
{{ carbon-icon 'Information16' }} | ||
</div> | ||
</div> | ||
{{!-- @todo add aria-hidden once JavaScript can toggle it --}} | ||
<div id="{{label.idSuffix}}" data-floating-menu-direction="bottom" class="{{@root.prefix}}--tooltip" | ||
data-avoid-focus-on-open role="dialog" aria-describedby="{{label.idSuffix}}-body" aria-label="Tooltip label"> | ||
<span class="{{@root.prefix}}--tooltip__caret"></span> | ||
<p id="{{label.idSuffix}}-body" >This is some tooltip text. This box shows the maximum amount of text that should appear inside. If more room is | ||
needed | ||
please use a modal instead.</p> | ||
<div class="{{@root.prefix}}--tooltip__footer"> | ||
<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"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some question as another instance of tooltip body. |
||
{{!-- @todo add aria-hidden once JavaScript can toggle it --}} | ||
<div id="{{label.idSuffix}}" data-floating-menu-direction="bottom" class="{{@root.prefix}}--tooltip" | ||
data-avoid-focus-on-open role="dialog" aria-describedby="{{label.idSuffix}}-body" aria-label="Tooltip label"> | ||
<span class="{{@root.prefix}}--tooltip__caret"></span> | ||
<p id="{{label.idSuffix}}-body" >This is some tooltip text. This box shows the maximum amount of text that should appear inside. If more room is | ||
needed | ||
please use a modal instead.</p> | ||
<div class="{{@root.prefix}}--tooltip__footer"> | ||
<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> | ||
</div> | ||
<span tabindex="0"></span> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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">
.