-
Notifications
You must be signed in to change notification settings - Fork 25
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need the aria-describe-content="${this.ariaDescribeContent}"
here.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)}"
)
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.
Looks good... just a couple of super minor tweaks.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: same thing here... let's move this up below async
so that the property assignments are sorta alpha.
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.
Looks great!
🎉 This PR is included in version 1.226.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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.