Accessibility review #3384
Accessibility review #3384
Conversation
There was a problem hiding this comment.
👍 on the H6 -> div update, and the "Submit search" to "Search in the document" aria label update.
On changing the sidebar toggle button label from "Toggle sidebar" to "Show / hide sidebar" I'm not as sure. I have a couple of thoughts about this:
-
I see the comment from the accessibility review report (note that I'm reading the English translation, which I believe is just Google Translate, so it's possible the advice is different in the original German report) on page 17 about how the German-language button label is not meaningful ("Switch sidebar" is what the translated report says it reads). But "Toggle sidebar" is meaningful in English, so one solution is to leave the English label as is and just update the German translation to whatever makes more sense in German than the literal translation of "toggle".
-
If we do want to change the label to "Show / hide sidebar" I think that would be great, but only if we can use the specific label for the current state of the sidebar. For instance, if we can display the label "Show sidebar" when the sidebar is closed, and "Hide sidebar" when the sidebar is open. I think that is better than "Toggle sidebar". But "Show / hide sidebar" for both states feels like we are taking a shortcut. And it's inconsistent with the nearby expand/collapse sidebar button, where we do show the appropriate label for the current state ("Collapse sidebar" when it's open and "Expand sidebar" when it's closed).
|
Thank you very much for your feedback, @ggeisler ! The text from our accessability review was the following:
But you are right, the label should depend on the current state of the sidebar 👍 I will fix that. |
morpheus-87
force-pushed
the
a11y-fixes
branch
2 times, most recently
from
04e8776 to
fd9360e
Compare
Codecov Report
@@ Coverage Diff @@
## master #3384 +/- ##
==========================================
+ Coverage 89.06% 89.17% +0.11%
==========================================
Files 196 196
Lines 3393 3382 -11
==========================================
- Hits 3022 3016 -6
+ Misses 371 366 -5
Continue to review full report at Codecov.
|
morpheus-87
force-pushed
the
a11y-fixes
branch
from
fd9360e to
a622465
Compare
|
I also just added one more |
morpheus-87
force-pushed
the
a11y-fixes
branch
from
b8cbe10 to
dbd913d
Compare
|
@morpheus-87 Thanks for working on the updates!
The difference between the two elements is subtle, but intentional. Toggle sidebar
This enables an implementer to configure the sidebar to be completely closed by default, so the user can focus on the workspace area without any interference from any UI controls in the workspace area. We do this at Stanford, for example, when using the Mirador viewer as an item viewer in various catalog-like pages, where the item metadata is often already on the page outside of the viewer and we don't expect most users to need to use the sidebar at all. So we give the user the cleanest view of the item but also an easy way to open the sidebar if they want to. Expand/collapse sidebar
This enables the user who does open the sidebar to collapse it to de-clutter the workspace (when zooming and panning on the image, for example) but the visible tab to re-expand the sidebar serves as a reminder of sorts that the sidebar can be re-expanded and that there are other sidebar categories available. (Notice that when the sidebar is collapsed, the sidebar menu is still visible, while when the sidebar is closed it is not.) Again, I know the difference is subtle, but the idea is that the visible tab might be more handy for the user who is opening and closing the sidebar frequently while they examine and work with the item, looking at the different sidebar category content, etc. |
There was a problem hiding this comment.
I don't really have a Mirador dev environment working so I can't easily see these updates, but based on the file changes and what @morpheus-87 says he is trying to do, I assume this is good to go. I can double-check after the PR is merged and the updates are visible in the Mirador demo instance.
EDIT: I forgot about the Netlify preview. I checked that out and it looks good to me.
morpheus-87
force-pushed
the
a11y-fixes
branch
from
dbd913d to
b0ae30a
Compare
|
Thank you very much for your explanation, @ggeisler ! That makes totally sense to me 👍 I also tried to fix the integration tests, but I don't understand, why one of them is failing 🤔 |
|
Could someone please help me with the integration tests? Perhaps @mejackreed ? |
morpheus-87
force-pushed
the
a11y-fixes
branch
from
2c097f3 to
075c7ff
Compare
morpheus-87
force-pushed
the
a11y-fixes
branch
from
82495dc to
87bbae2
Compare
morpheus-87
force-pushed
the
a11y-fixes
branch
from
87bbae2 to
b5f3521
Compare
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## master #3384 +/- ##
==========================================
- Coverage 92.49% 89.17% -3.32%
==========================================
Files 201 196 -5
Lines 3465 3382 -83
==========================================
- Hits 3205 3016 -189
- Misses 260 366 +106
... and 111 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
This PR fixes some things that were found in our accessability review:
<ul>, which suffices for screenreaders to properly interpret the structureariaattributes