feat: Dialogs: If no focusable elements in container, make content container focusable [GAUD-7001] #4994
Conversation
|
Thanks for the PR! 🎉 We've deployed an automatic preview for this PR - you can see your changes here:
Note The build needs to finish before your changes are deployed. |
components/dialog/dialog-mixin.js
Outdated
| @@ -43,6 +43,7 @@ export const DialogMixin = superclass => class extends RtlMixin(superclass) { | |||
| */ | |||
| titleText: { type: String, attribute: 'title-text' }, | |||
| _autoSize: { state: true }, | |||
| _focusableContentElem: { state: true }, | |||
There was a problem hiding this comment.
Nit: I'd probably name this _hasFocusableContentElement or something along those lines. Currently, it sounds like it would be reference to a DOM node. I think we sorta try to avoid prefixes like that though, so it could also be named more along the lines of _focusContentContainer (which unfortunately also sounds like a method, not a property). 🤷♂️
There was a problem hiding this comment.
I wondered about that - I also thought _has would be more appropriate but left it out since we avoid those. I'll think a bit more on it.
|
It looks like it's getting the default outline in the diffs. I'm guessing we probably want to apply our preferred focus styles? |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
components/dialog/dialog-mixin.js
Outdated
| @@ -476,6 +481,9 @@ export const DialogMixin = superclass => class extends RtlMixin(superclass) { | |||
| styles.top = `${iframeTopOverride}px`; | |||
| } | |||
|
|
|||
| if (!this._focusableContentElemPresent) this.shadowRoot.querySelector('.d2l-dialog-content')?.setAttribute('tabindex', '0'); | |||
| else this.shadowRoot.querySelector('.d2l-dialog-content')?.removeAttribute('tabindex'); | |||
There was a problem hiding this comment.
Is there a way to avoid manipulating DOM nodes inside of render() here? I guess the mixin isn't responsible for rendering the dialog content, but maybe the places that are could either be passed the value of focusableContenteElemPresent or they could access it themselves?
There was a problem hiding this comment.
Yup I think we can do that. The disadvantage is that it becomes another thing that consumers of DialogMixin would have to do, and we'd want to update places that use DialogMixin (but outside of core there are only 2 so that's not too bad).
There was a problem hiding this comment.
Cool. I think I'd personally prefer that... it feels weird that the mixin even knows about an element with that CSS class, let alone reaching in to manipulate it.
There was a problem hiding this comment.
Agreed - I'll make that change.
…omponents rather than in DialogMixin _render, switch box-shadow to outline
| @@ -22,7 +22,7 @@ <h2>Confirm Dialog</h2> | |||
| <d2l-demo-snippet> | |||
| <template> | |||
| <d2l-button id="openConfirm">Show Confirm</d2l-button> | |||
| <d2l-dialog-confirm id="confirm" text="Are you sure you want more cookies?"> | |||
| <d2l-dialog-confirm id="confirm" title-text="Confirm Title" text="Are you sure you want more cookies?"> | |||
There was a problem hiding this comment.
This had been removed in a previous PR but there's another example on the page that's specifically for this case so I re-added this.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
🎉 This PR is included in version 3.42.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Jira ticket
Deque context
Dialogs have a potential axe violation where if there are no focusable elements in dialog content but the content overflowed, then focus should go to the content container, which was not happening.