Skip to content
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(ui5-dialog): labeling of header slots is now possible #3155

Merged
merged 9 commits into from
Apr 28, 2021

Conversation

GerganaKremenska
Copy link
Member

@GerganaKremenska GerganaKremenska commented Apr 15, 2021

When Dialog's header slot is used, developers now would be able to use newly added accessible-name attribute to label the dialog properly. (If the header slot is not used, the Dialog labels the internally created header as previous.)

Fixes: #2838

@@ -271,8 +273,8 @@
</div>
</ui5-dialog>

<ui5-dialog id="draggable-dialog" draggable>
<div slot="header" style="flex: 1 0 0; height: 2.5rem;">Draggable dialog</div>
<ui5-dialog id="draggable-dialog" accessible-name-id="tlt" draggable>
Copy link
Member

@ilhan007 ilhan007 Apr 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just as an option.
Instead of providing the proposed API, could you add private "ariaLabelledBy" property and use it the same way as "accessibleNameId".

aria-labelledby is a standard attribute, so we can declare it in the metadata as private not to end up in the documentation, but at the same time we weill have it in the component's state (we used this approach in the Button for example).

 * @private
 * @since 1.0.0-rc.15
ariaLabelledby: {
			type: String,
			defaultValue: "",
},
	<ui5-dialog aria-labelledby="tlt">

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tip, but if it added with aria-labelledby, jaws does double readout with the combination of getEffectiveAriaLabelText. With the approach of the accessible-name validator and screen reader is happy.

@elenastoyanovaa elenastoyanovaa self-requested a review April 16, 2021 06:08
Copy link
Contributor

@elenastoyanovaa elenastoyanovaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @GerganaKremenska @ilhan007 ,

With the current implementation the generated html is invalid. You are labelling the section element with an element with an id of "tlt", which is not an element in the same DOM as the section element. Validators complain and also if you inspect the section itself, you can see that in the accessibility TAB, the aria-labelledby is not mapped correctly. For the same reason aria-labelledby attribute is also not an option since it will result in the same thing. What is more, even if the screen reader somehow maps the label correctly, adding the same aria-labelledby/aria-label on both custom element and the element which receives the focus, many times resulted in repetitive speech output and I would not recommend this approach. @ilhan007 mentioned that we used the aria-labelledby in the ui5-button, but there internally (as in many other components) we get the text inside the aria-labelledby element and map it as aria-label in the DOM eventually - this is done because of those invalid html results I mentioned earlier. You can check the getEffectiveAriaLabelText helper for more infomation. IMO you should be careful with getEffectiveAriaLabelText again, since this may result in repetitive speech output. This is why in many controls we provided the accessibleName property in order to avoid the invalid html and repetitive speech announcements. As the support for aria-label/aria-labelledby via getEffectiveAriaLabelText, the accessibleName results in a aria-label attribute, but avoids strange speech output by readers.

@ilhan007 ilhan007 changed the title feature(ui5-dialog): labeling of header slots is now possible feat(ui5-dialog): labeling of header slots is now possible Apr 20, 2021
@ilhan007
Copy link
Member

The build fails due to eslint error
Screenshot 2021-04-20 at 14 13 48

@GerganaKremenska GerganaKremenska dismissed elenastoyanovaa’s stale review April 28, 2021 06:49

done wioht accessibleName

@ilhan007 ilhan007 merged commit 9943ee7 into SAP:master Apr 28, 2021
ilhan007 pushed a commit that referenced this pull request Apr 28, 2021
When Dialog's header slot is used, developers now would be able to use newly added accessible-name attribute to label the dialog properly. (If the header slot is not used, the Dialog labels the internally created header as previous.)

Fixes: #2838
ilhan007 pushed a commit that referenced this pull request Apr 28, 2021
When Dialog's header slot is used, developers now would be able to use newly added accessible-name attribute to label the dialog properly. (If the header slot is not used, the Dialog labels the internally created header as previous.)

Fixes: #2838
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ui5-dialog: Accessibility: using header slot causes broken aria-labelledby reference
4 participants