Add more alt text descriptions #14320
Conversation
|
Thanks for making a pull request to jupyterlab! |
|
Thanks for submitting your first pull request! You are awesome! 🤗 |
s596757
changed the title
s596757
changed the title
There was a problem hiding this comment.
Thanks @s596757 I have suggestions to support alternative text localization.
@isabela-pf @gabalafou @steff456 would you mind reviewing this one? I'm not sure about the title on the icon and the title. For me, using title feels in proper (in particular for the icon) as it will result in a pop-up display by the browser for people without disabilities.
Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>
Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>
There was a problem hiding this comment.
Hi!
Thanks for adding alt text descriptions! I'm seeing that you want to add this property to icons and I'm wondering if we could add a more detailed description on what happens when the users click them more than just describing the actual icon.
I'm sharing with you this blog post where you can see some examples, https://www.searchenginejournal.com/alt-text-for-logos-and-buttons/469801/#close
Let me know if you need any extra help!
Benchmark reportThe execution time (in milliseconds) are grouped by test file, test type and browser. The mean relative comparison is computed with 95% confidence. Results table
Changes are computed with expected as reference.
Waiting for localhost:8888 Cell memory leaksCreate a code cellMemory change: +160 kB Leak detected: YesLeaking objects:
Leaking collections: Create a markdown cellMemory change: -143 kB Leak detected: NoLeaking objects:
Leaking collections: Create a raw cellMemory change: +143 kB Leak detected: YesLeaking objects:
Leaking collections: File editor memory leaksCreate a fileMemory change: -80.1 kB Leak detected: NoLeaking objects:
Notebook memory leaksCreate a notebookMemory change: +22.9 kB Leak detected: YesLeaking objects:
2 passing (6m) |
There was a problem hiding this comment.
Thank you so much for working on adding image labels in JupyterLab! As I mentioned in the accessibility meeting this week, I don't want to block this with my limited time. Please move forward with the other reviewers' comments first. I won't block it if other people are ready to merge.
I'm going to try and address the comments that have come before me.
I'm not sure about the title on the icon and the title. For me, using title feels in proper (in particular for the icon) as it will result in a pop-up display by the browser for people without disabilities.
@fcollonval That doesn't sound ideal, but I don't know how to test it in this case, sorry. Are you talking about how each of the labels, or one in particular. I can try it out if I know which one I'm looking for.
I'd also second @steff456 that it would be helpful to say what the icons do when selected in the description. I would also be happy to include suggestions if I know which icons these are appearing on, but I don't have enough time to dig at the moment.
If it's helpful, I can tell you the main things I look for in image descriptions are summarized in the Jupyter accessibility workshop resources. But not all of JupyterLab follows this yet. Thank you again!
There was a problem hiding this comment.
Some modifications here are not proper - alt tag does not exist in SVG: https://stackoverflow.com/questions/23146570/alt-tag-possible-for-inline-svg. title should be used or aria-labelledby
Moreover we should not set id as a icon may appears multiple time.
Finally setting the title/alternate text and the description in the inline SVG forbid any internationalization.
So I would suggest ensuring we can provide an aria-labelledby attribute and/or title and description when injecting an icon. So those attribute will be passed to the the SVG properly.
If
titleis provided, we could automatically add anarial-labelledbyreferencing thetitlewith a dynamically generatedidto ensure uniqueness.
|
Please can you have a look at the changes we've made |
|
Galata snapshots updated. |
Changes made, please review thank you |
|
Accidentally removed review requests, added back |
|
@krassowski @fcollonval Please can you review asap? Thank you :) |
There was a problem hiding this comment.
Hello again! I gave this another review and it seems solid based on my knowledge. I don't have further information about the implementation details, so the content of the image descriptions is the only thing I reviewed.
I have provided suggestions to make my feedback as clear as possible, but I'm open to other solutions for the feedback. I'll be happy to give it another approving review if you tag me when this updates. Thank you for your patience.
For general reference of anyone stumbling into my review in the future, I try to check my alt text reviews against the lovely wealth of example descriptions the W3C provides.
| @@ -867,6 +867,7 @@ export namespace Dialog { | |||
| iconClass="jp-Icon" | |||
| className="jp-ToolbarButtonComponent-icon" | |||
| tag="span" | |||
| title={trans.__('Close Icon, closes toolbar')} | |||
There was a problem hiding this comment.
I usually recommend against words like icon, image, or the like because it is often otherwise declared or not more important than the fact it is an interactive area with an attached action.
| title={trans.__('Close Icon, closes toolbar')} | |
| title={trans.__('Close toolbar')} |
| @@ -386,7 +386,9 @@ const resources: JupyterFrontEndPlugin<void> = { | |||
| isEnabled, | |||
| execute: () => { | |||
| // Create the header of the about dialog | |||
| const headerLogo = <img src={kernelIconUrl} />; | |||
| const headerLogo = ( | |||
| <img src={kernelIconUrl} alt={trans.__('Header logo')} /> | |||
There was a problem hiding this comment.
Usually I have logo alt text describe the name of the thing the logo describes. I think in this case I would be wondering "which logo?" if this were read to me, especially since there are a number of viable options in the Jupyter space. (I am referencing this W3C logo description example.)
I am relatively confident this is always the Project Jupyter logo, but if there are cases where it might be JupyterLab or something else I would have a different suggestion.
| <img src={kernelIconUrl} alt={trans.__('Header logo')} /> | |
| <img src={kernelIconUrl} alt={trans.__('Project Jupyter')} /> |
| @@ -549,10 +549,6 @@ export class LabIcon implements LabIcon.ILabIcon, VirtualElement.IRenderer { | |||
| if (uuid) { | |||
| svgElement.dataset.iconId = uuid; | |||
| } | |||
|
|
|||
| if (title) { | |||
There was a problem hiding this comment.
You need to go one step further and delete title from the arguments passed into _initSvg
|
Closing as superseded by #15265. |
PR-label: Accessibility
Labels: Enhancement
Have added titles and descriptions to headings, icons and images as per ticket request. Have removed descriptions from within svgs and instead added changes to the addtitlestosvgs function to allow titles to be read @krassowski please can you check and approve so we can move forward
References
#9682 and ticket from prioratised task list
Code changes
Added alt description to images and icons
User-facing changes
Icons and images have descriptive labels when you hover over them