Alt text and marking elements decorative #14819
Conversation
|
Thanks for making a pull request to jupyterlab! |
github-actions
bot
added
pkg:launcher
pkg:ui-components
tag:CSS
github-actions
bot
removed
tag:CSS
There was a problem hiding this comment.
Thank you so much for working on this and being persistent even when it takes time for me to get reviewing. Overall, I agree with the idea that the launcher icons (next to the text title of a launcher section) should be decorative and note read from a user experience perspective.
I'm not clear what
Changed the getTitleSvg function to include DOMUtils as per recommendation
impacts on the user side. Based on comments on past PRs it looks like this was a request to remove unused code? Since it was requested I'm open to it, but I don't really know how to usefully review it. @gabalafou was this your request in the past?
For clarity, I don't have the resources to review this with assistive tech myself at the moment, so know I am reviewing this solely on reading the Files Changed. I also don't have quite enough knowledge of ARIA in this use case and how it interacts with JupyterLab. I'm really reviewing this based on the proposed user interaction of the PR.
I do have a few questions I'd like answered before I approve it:
- Are there any negative consequences of hiding what is also part
h2? - Is the intent for the
TODOto stay in the JupyterLab codebase, or is that a marker for the author before this PR is to be merged? I am seeing precedent for it elsewhere in the codebase, so if it's intentional I won't block on it.
|
For reference, this PR supersedes #14400 |
| /> | ||
| <h2 className="jp-Launcher-sectionTitle">{cat}</h2> | ||
| <h2 className="jp-Launcher-sectionTitle" title={cat + ' Title'}> |
There was a problem hiding this comment.
Please explain the rationale for this change. What was the user experience before this line change, what is the user experience after this line change, and why is it better?
I'm sorry but I can't figure out from reading neither the PR description nor the associated issue #9682. For example, the issue says nothing about using the title attribute on elements, as far as I can tell. It only mentions using the <title> tag in <svg> elements.
| // make the title the aria label0 | ||
|
|
||
| /// TODO set better unique id for title |
There was a problem hiding this comment.
| // make the title the aria label0 | |
| /// TODO set better unique id for title |
| if (!titleNodes[0].id) { | ||
| titleNodes[0].id = performance.now().toString(); | ||
| } |
There was a problem hiding this comment.
| if (!titleNodes[0].id) { | |
| titleNodes[0].id = performance.now().toString(); | |
| } |
| if (titleNodes.length) { | ||
| titleNodes[0].textContent = title; |
There was a problem hiding this comment.
Should this line be removed?
@gabalafou then that was my misunderstanding from the accessibility meeting. Sorry about that! I'll check in again at the upcoming meeting and won't mess with this PR until then. |
|
@s596757 are you still interested in finishing this one? |
As per comments on PR (#9682), removed redundant title attribute in code and left aria-hidden.
Changed the getTitleSvg function to include DOMUtils as per recommendation
tested via speak aloud and narrator. Previously was reading out icons descriptions but is no longer doing so in either.
Created new PR as unable to push into previous PR due to base branch changes.
Reference PR: #9682