-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Improve keyboard navigation in navbar, sidenav and top controls #23756
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| } | ||
|
|
||
| .accessibility-skip-to-content { | ||
| padding: 0 0.5rem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ To separate a bit the focus ring and the link label
| a:focus-visible, | ||
| button:focus-visible { | ||
| outline-color: @theme-color-focus-ring-alternative; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Update focus ring color for the navigation to improve contrast and avoid a blue ring over a blue background.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to change, just a note.
This is already a good improvement, I just thought I'd mention that the new green colour doesn't meet 3:1 contrast ratio. We're definitely not in a WCAG 2.1 AA compliant state, so just a note for future reference.
| <a href="{{ logoLink|default('index.php') }}" tabindex="3" | ||
| title="{% if isCustomLogo %}{{ 'General_PoweredBy'|translate }} {% endif %}Matomo{% if not isCustomLogo %} # {{ 'General_OpenSourceWebAnalytics'|translate }}{% endif %}" | ||
| > | ||
| {% endif %} | ||
| {% if hasSVGLogo %} | ||
| <img src='{{ logoSVG }}?matomo' tabindex="3" | ||
| <img src='{{ logoSVG }}?matomo' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ The logo image was focused instead of the link
| if (binding.value?.onExpand) { | ||
| binding.value.onExpand(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ All the changes in this file is to allow an optional onExpand option. It was a bad idea since I do not want to force focus for the mouse users so I added onExpandByKeyboard too.
| </button> | ||
|
|
||
| <a | ||
| <button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Moved to <button> since there is no href. a <a> without is not a link but an anchor.
| :href="`#?${makeUrl(category, subcategory)}`" | ||
| class="item" | ||
| @click="loadSubcategory(category, subcategory, $event)" | ||
| tabindex="5" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ The missing tabindex made the submenu inaccessibles
815cafc to
b431ef9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ matomo logo centered and selector title centered
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ matomo logo centered
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ matomo logo centered
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ matomo logo centered
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ matomo logo centered
| a:focus-visible, | ||
| button:focus-visible { | ||
| outline: 2px solid @theme-color-focus-ring; | ||
| outline-offset: -2px; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ :focus-visible is the selector that is only triggered by keyboard navigation
2a006e5 to
c5a3a30
Compare
| background-color: transparent; | ||
| border: none; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Add button reset style since I added button support
| padding-right: 1rem; | ||
|
|
||
| text-decoration: none; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Improve logo position and focus ring size
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Centered role selector
c9eab06 to
cf6189f
Compare
michalkleiner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice improvement. The taborder is still a bit unintuitive, but it's definitely better than what it was on the areas fixed in this PR.
@sgiehl I'm not sure what implications adding two new colour variables might have for 3rd-party themes, is it a safe thing to do or do we need to mention it somewhere?
Please don't merge yet until Stefan had a chance to answer the question above.
| a:focus-visible, | ||
| button:focus-visible { | ||
| outline-color: @theme-color-focus-ring-alternative; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to change, just a note.
This is already a good improvement, I just thought I'd mention that the new green colour doesn't meet 3:1 contrast ratio. We're definitely not in a WCAG 2.1 AA compliant state, so just a note for future reference.
|
I guess we could consider mentioning those new color variables in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Added the changelog entry, created an internal ticket to add changelog entry for 5.5.0 that seems to be missing.
Description
Nice and catchy blue focus ring everywhere
Blue by default.
Alternative color on the main navigation to avoid blue over blue.
Fix sidenav selection with keyboard navigation
It was impossible to open a submenu.
Small improvements
Improve « skip to content » focus ring
Improve Matomo logo focus ring
Fix period selection with keyboard navigation
Fixing the period selector was harder, broke tests, and change different files. So I choose to separate it in another PR that is not ready yet:
#23768
Checklist
Review