Ensure that the download modal is announced to screen reader users #3593
Conversation
NickColley
force-pushed
the
improve-modal
branch
5 times, most recently
from
dd6882c to
67846d9
Compare
| <div id="download-modal" class="dialog modal"> | ||
| <img alt="Download elementary OS icon" src="images/icons/apps/48/system-os-installer.svg"> | ||
| <dialog id="download-modal" class="dialog" aria-labelledby="download-modal-title"> | ||
| <img src="images/icons/apps/48/system-os-installer.svg" alt=""/> |
There was a problem hiding this comment.
I have set the alt to nothing to mark the image as presentational as it is just for show.
| <span id="translate-purchase" style="display:none;" hidden>Purchase elementary OS</span> | ||
| <div id="download-modal" class="dialog modal"> | ||
| <img alt="Download elementary OS icon" src="images/icons/apps/48/system-os-installer.svg"> | ||
| <dialog id="download-modal" class="dialog" aria-labelledby="download-modal-title"> |
There was a problem hiding this comment.
Here I label the dialog based on the heading so that the dialog has a clear name "Choose a Download modal dialog" etc
NickColley
changed the title
|
Hi Nick sorry for the extremely long delay! Tested this and looks good to me in the a11y tree. I couldn't get a real screen reader setup with the site but this is still an improvement. If you can resolve this conflict we'll merge this in 🙏 |
Adds a lot of good features that make it way more accessible like preventing outside elements from being focused, which can be difficult to do otherwise. Removes dependency on leanmodel2 so quicker page loads. TODO before ready: - Fallback gracefully in browsers that dont support it - Test in real screen readers
NickColley
force-pushed
the
improve-modal
branch
from
67846d9 to
f47acd9
Compare
|
@RMcNeely thank you updated to match the new linting standards |
|
Can you request a review so I can add one and then merge? I think that's what GH is complaining about |
|
It's not letting me do that but the button should be top right here: |
|
Just a driveby: nice work! |
|
@janxious thank you friend ❤️ |
3 participants
I did consider improving the current implementation, I think adding focus management would have been fine but keyboard trapping is non-trivial so that's why adopting this is worthwhile as we get all that for free.
Resolves #3378
Browser support strategy
For browsers that do not support the native dialog we aim to show them this at the bottom of the page:
Which'll be linked to using regular anchor jumping.
For users that support the dialog element it'll look the same as it currently does.
I have not been able to test what happens when someone donates as I cant get stripe working locally but it should work since I've just changed the internal modal and not the place it gets called.
Screen reader testing
NVDA
Using https://assistivlabs.com/.
When I open the dialog I get this announced:
Browser testing
Note the modal icon is not shown in the screenshots because I cant get it to load for some reason, but it is unchanged.