-
-
Notifications
You must be signed in to change notification settings - Fork 410
Make tooltip content in popup screenreader-accessible #3143
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
base: master
Are you sure you want to change the base?
Conversation
| "message": "Move the slider right to allow a domain", | ||
| "description": "Domain slider list header tooltip." | ||
| }, | ||
| "popup_slider_label": { |
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.
Is this redundant information? I ask because this wasn't raised as an issue when we revised/fixed a couple of screen reader bugs with the toggle labels last time.
Also, any the tracking domains tab is already in poor shape (crashes screen readers). Any additional toggle HTML will contribute to the tracker list performance problem.
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.
I added the aria-label because groupings of radio buttons should have a radiogroup role, and a radiogroup must have an accessible name (see: https://www.w3.org/WAI/ARIA/apg/patterns/radio/, https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/radiogroup_role)
Currently, when tabbing through interactive elements, the screenreader just reads "Block entirely, selected, radio button, 1 of 3." It's unclear what is blocked entirely (or cookieblocked or allowed). Adding an aria-label to the radio group provides context. However, this might be redundant when a screen reader user navigates through the non-interactive content (since that will include the domain before the slider).
Let's wait for Jesse to review and I'll remove if he doesn't think it's necessary.
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.
I like the how the tooltips appear on keyboard navigation!
We want want to have the same stuff happen in the options page. If we don't do it here, we might want to have the helper functions live in a common location so that we can do it on the options page when ready.
It doesn't look like the "Sign in with google" breakage warning is accounted for when keyboard navigating in this PR.
src/js/popup.js
Outdated
| * Make tooltips keyboard accessible | ||
| */ | ||
| function triggerTooltipOnFocus() { | ||
| $('.tooltip').on('focus', function() { |
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 selector here is always ".tooltip" but we don't always call .tooltipster() on ".tooltip".
Also, does triggerTooltipOnFocus() get called repeatedly? Do we attach the same listener to previously processed elements over and over?
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.
So far, I think the only elements with a tooltip that are keyboard focusable have a tooltip class. For example, I don't call this function after$('#not-yet-blocked-header').tooltipster() because the header is not interactive/tabbable (also that element has the tooltip class). In case that changes, I added an optional parameter (with default value .tooltip) for the selector.
Ah yes the same listener was being attached several times, but I've moved the function call in renderDomains and now remove the listener before adding to prevent this!
src/js/popup.js
Outdated
| /** | ||
| * Make tooltips keyboard accessible | ||
| */ | ||
| function triggerTooltipOnFocus() { |
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.
We might want to refactor this into one or two separate functions. Do we always want to call this when we call .tooltipster()? If yes, we should take the selector as a parameter and do both .tooltipster() and the focus trigger fix in the helper function.
The second part of this function, the input thing, we might want to break it out into its own function, if we don't want to do this every time we call .tooltipster(). We should probably also make its selector more specific than input.
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.
Good point, done!
src/js/popup.js
Outdated
| $('.tooltip').on('focus', function() { | ||
| $(this).tooltipster('show'); | ||
| }); | ||
| $('.tooltip').on('blur', function() { |
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.
I was wondering why we can't instead set the trigger to "custom" and specify focus/blur as our open/close triggers but it looks that's not supported by tooltipster? Oh well, too bad.
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.
Yep! It would be nice if they supported "focus" and "blur" as custom triggers, but this is the approach they explicitly recommend for making tooltips appear on focus: https://calebjacob.github.io/tooltipster/#accessibility
| "description": "Domain slider list header tooltip." | ||
| }, | ||
| "popup_slider_label": { | ||
| "message": "Blocking options for $DOMAIN$", |
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.
(Waiting on addtl. feedback whether we need this but,) "blocking options" seems both too specific ("blocking") and too vague ("options"). I feel stronger about "options" than "blocking" though. Not sure about better name either.
We still use "slider" in a bunch of places but maybe we shouldn't? I've been calling these controls "toggles". What do other extensions say?
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.
What about "Settings for
Other blocking extensions don't have sliders (except for UBlock's "filtering mode") and/or are not very screenreader accessible
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.
Alternatively:
- Toggles for __
- Controls for __
I think we should wait to update the domain tooltips on the option page until #3133 is fixed (although I haven't been able to replicate on my computer). I moved the helper functions into a common location so that it's easy when the option page is ready. Do you mean that the "Sign in with google" warning should be tabbable? I added |
Yeah, exactly. Tabbing through the popup never focuses on the warning, so you can't dismiss it with the keyboard. You do kind of confusingly tab through elements hidden under the warning. |
Addresses:
role="radiogroup"and an aria-label to the slider group to provide more context when reading slider buttonsTested with the VoiceOver screenreader on MacOS
Test instructions: