Add labels for Contacts menu and Settings, thanks to @MarcoZehe #9224
Conversation
jancborchardt
added
bug
design
core/templates/layout.user.php
Outdated
| </div> | ||
| <div id="settings"> | ||
| <nav id="settings" aria-label="Settings"> | ||
| <div id="expand" tabindex="0" role="link" class="menutoggle"> |
There was a problem hiding this comment.
This would create a navigation landmark with the label "Settings", but the actual link (which should be a button) will still not be labeled. Also, due to the tabindex attribute, keyboard focus will land on here. So what the user will hear is:
"Settings navigation landmark"
followed by "Link".
And that link still won't say that it is the Settings link, that it's actually hiding a dropdown menu (aria-haspopup="true", and that it's currently hiding it (aria-expanded="false"). Oh and yes that div should have a role of "button", not "link". Links go somewhere, buttons do something.
There was a problem hiding this comment.
Just added the aria-haspopup, -expanded and -controls attributes. Will look into the other things now, thank you!
There was a problem hiding this comment.
I moved the aria-label="Settings" down to the actual link, and also added an aria-label="Settings menu" to the menu. Is that correct?
core/templates/layout.user.php
Outdated
| <div class="menu"></div> | ||
| </div> | ||
| <div id="settings"> | ||
| <nav id="settings" aria-label="Settings"> |
There was a problem hiding this comment.
Shouldn't this be localized as well?
There was a problem hiding this comment.
Yes, aria-label, or anything referenced by aria-labeledby via ID, as well as aria-describedby (for additional info, less often used), are spoken by a screen reader and thus should be localized.
Codecov Report
@@ Coverage Diff @@
## master #9224 +/- ##
============================================
- Coverage 51.91% 51.91% -0.01%
Complexity 25361 25361
============================================
Files 1606 1606
Lines 95311 95317 +6
Branches 1394 1394
============================================
+ Hits 49478 49480 +2
- Misses 45833 45837 +4
|
|
Also added labels to the apps now. @MarcoZehe I did it via |
|
@MarcoZehe I think now all the feedback points you mentioned on Twitter should be fixed? (Except making the elements buttons, because that will need some more global CSS work.) @nextcloud/accessibility please check also. :) |
core/templates/layout.user.php
Outdated
| </ul> | ||
|
|
||
| </div> | ||
| </nav> |
There was a problem hiding this comment.
Indentation seems wrong here.
… thanks to @MarcoZehe Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
jancborchardt
force-pushed
the
accessibility-improvements
branch
from
f704b5f to
df3c1ac
Compare
|
Squashed the commits into one also because it’s not so much, and in case we want to backport. @MorrisJobke? |
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
|
I also went through it with aXe again and fixed some more color contrast and issues on the log in page. This should be good to go now, and other aspects like settings and such we should do in separate pull requests. |
|
Retriggered the CI job |
|
@MarcoZehe by the way, I’d like to invite you to the Nextcloud organization here, and the accessibility team as well. Let me know if you are ok with that. :) |
|
We should backport this to stable13 for 13.0.3 |
|
Yes, please, i would be glad as well. |
@jancborchardt Could you do this? Thanks |
|
@MorrisJobke I have issues with my laptop currently as it is not properly charging, and is off to repair since yesterday eve. :/ Could I ask you to do it? |
Sure - there you go #9433 |
Ideally, the labels should show up as popovers also when hovering just like for the apps cc @juliushaertl @skjnldsv.
Please review @nextcloud/accessibility @nextcloud/designers @MarcoZehe