DE46715 added ariaDescribeContent property for a11y #2030
Conversation
components/dialog/dialog.js
Outdated
| const content = html` | ||
| ${loading} | ||
| <div style=${styleMap(slotStyles)}><slot></slot></div> | ||
| <div id="${this._textId}" aria-describe-content="${this.ariaDescribeContent}" style=${styleMap(slotStyles)}><slot></slot></div> |
There was a problem hiding this comment.
I don't think you need the aria-describe-content="${this.ariaDescribeContent}" here.
There was a problem hiding this comment.
you are right! I will take that out :)
|
Visual diff tests failed - pull request #2031 has been opened with the updated goldens. |
There was a problem hiding this comment.
Overall I think this strategy makes sense and seems inline with example 3 in the w3 examples here https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/dialog.html
components/dialog/dialog.js
Outdated
| /** | ||
| * Whether to read the contents of the dialog on open | ||
| */ | ||
| ariaDescribeContent: { type: Boolean, attribute: 'aria-describe-content' }, |
There was a problem hiding this comment.
I believe we'd want to just call this describeContent / describe-content since it seems that this isn't an actual aria- attribute (and we generally want to avoid putting aria-* attributes directly on our custom elements - we more recommend having a property like label on the custom element, which would then be transferred onto the aria-label html element that the screenreader would then pick up on).
You'll also want to initialize this to false in the constructor, like with async (https://github.com/BrightspaceUI/guide/wiki/LitElement-Best-Practices-&-Gotchas#-do-default-boolean-properties-to-false)
There was a problem hiding this comment.
This should also be added to an accessibility properties table in the README. (e.g., https://github.com/BrightspaceUI/core/tree/main/components/button#accessibility-properties). The table can just go under Events, after the <!-- docs: end hidden content --> comment in the README. We might also want to add some sort of recommendation on when to use as this seems more recommended for dialogs with shorter text (though I could be wrong on that)
There was a problem hiding this comment.
Agreed with everything Margaree mentioned here -- I like calling this describe-content.
Also it might be good to have some kind of guidance in the documentation for when and when not this should be used... like should we be defaulting it to true for all our prompt-style dialogs? And if so, maybe this doesn't need to be a customizable thing at all but just something that's built into the <d2l-dialog-confirm>?
There was a problem hiding this comment.
This is already built in to d2l-dialog-confirm but we need it in d2l-dialog as well. Not sure if it would make sense to build it in there as well unconditionally?
components/dialog/dialog.js
Outdated
| @@ -151,9 +156,11 @@ class Dialog extends LocalizeCoreElement(AsyncContainerMixin(DialogMixin(LitElem | |||
| 'd2l-footer-no-content': !this._hasFooterContent | |||
| }; | |||
|
|
|||
| if (!this._textId) this._textId = getUniqueId(); | |||
There was a problem hiding this comment.
nit:
| if (!this._textId) this._textId = getUniqueId(); | |
| if (!this._textId && this.describeContent) this._textId = getUniqueId(); |
since you don't need to add this to the div unless we're using aria-describe-content. If you do change this, you'd want to use ifDefined with the id (id="${ifDefined(this._textId)}")
components/dialog/README.md
Outdated
|
|
||
| | Attribute | Description | | ||
| |--|--| | ||
| | `describe-content` | When set to 'true' reads the dialog content on open. | |
There was a problem hiding this comment.
I might just rephrase this so consumers don't attempt to assign "true". Maybe When set, screen readers will announce the contents when opened. Useful when the the dialog contains no other focusable elements.
components/dialog/dialog.js
Outdated
| /** | ||
| * Whether to read the contents of the dialog on open | ||
| */ | ||
| describeContent: { type: Boolean, attribute: 'describe-content' }, |
There was a problem hiding this comment.
Nit: let's move this below async to keep the public properties sorta alpha.
components/dialog/dialog.js
Outdated
| @@ -103,6 +108,7 @@ class Dialog extends LocalizeCoreElement(AsyncContainerMixin(DialogMixin(LitElem | |||
| this.width = 600; | |||
| this._handleResize = this._handleResize.bind(this); | |||
| this._handleResize(); | |||
| this.describeContent = false; | |||
There was a problem hiding this comment.
Nit: same thing here... let's move this up below async so that the property assignments are sorta alpha.
|
🎉 This PR is included in version 1.226.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
ghost
added
the
Original Rally Story:
DE46715: [A11y] Update all FACE help dialogs focus to be on the close (x) button instead of the okay button
In FACE quiz accessibility reviews it was pointed out that when the user opens the help dialog with the screen reader, the focus goes directly to the footer making it difficult for non-sighted users to access the dialog content.
The original suggestion was to have the focus land on the close button at the top so users could navigate to the content text more easily, but based on my conversation with Dave B this would require larger changes to the dialog component.
This is an alternative solution where there is an optional property added to the dialog that allows the content to be read out automatically on opening the dialog by using the aria-describedby property in the dialog-mixin.
If aria-describe-content="true" the dialog content will be read when the dialog is opened.