-
-
Notifications
You must be signed in to change notification settings - Fork 387
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
Organize sliders into expandable summary, add first party protections section #2748
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
e92865d
to
7040294
Compare
Surely there's a more graceful approach to some of my css in this, but going ahead and marking ready for review now. 😅 |
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.
Thank you! Please see my first pass feedback below.
src/skin/popup.html
Outdated
@@ -128,11 +128,24 @@ <h2 id="title-name"><span class="i18n_name"></span></h2> | |||
<span class="i18n_popup_instructions_no_trackers" data-i18n_contents_placeholders="<a target='_blank' title='i18n_what_is_a_tracker' class='tooltip' href='https://privacybadger.org/#What-is-a-third-party-tracker'>"></span> | |||
<span id="no-third-parties" class="i18n_popup_blocked" style="display:none"></span> | |||
</span> | |||
<div id="toggle-blocked-resources-container"> | |||
<span id="expand-blocked-resources" class="ui-icon ui-icon-caret-d"></span> |
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.
This caret is visible on pages with no third parties (https://example.com
).
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 carets should get the hand cursor like other clickable elements.
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.
Should carets get a tooltip? "Click to show more information"?
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.
Whatever ends up being clickable to expand/collapse sections should be keyboard-accessible. Users should be able to Tab to visibly select each toggle element and Space (or whatever the standard key is) to activate.
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.
Resolved all these except the accessible activation via the keyboard bit. At its current working state, one can focus on the element from the keyboard, but I'm unable to get the activation bit working. I tried attaching on keydown
listeners and more tradition onClick
s to it, no luck. On the other hand, while testing this out I realized there are a few places that aren't accessible or end up "trapping" the user into some modals that they can't easily exit out of without a mouse action. Perhaps that's worth addressing in a follow up PR?
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.
<div>
or <span>
elements don't have built-in keyboard accessibility; <a>
and <button>
elements do.
To make a <span>
focusable and so able to receive keyboard events, you have to add a tabindex=0
. You then have to add a keydown
listener and filter for supported keys.
Make sure to add the tabindex
attribute to the same element that your keydown
listener is on.
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.
You'll also want to add proper ARIA attributes (such as role=button
perhaps) to your elements.
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.
You could instead use a <button>
or an <a href="" role="button">
element, which automatically supports keyboard navigation and will also trigger your click handler on Space/Enter presses. You will probably want to e.preventDefault()
inside the click handler (to avoid performing the unwanted default action of <a>
).
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.
Yes to improving keyboard navigation in a followup PR: #2629 (comment)
Friendlier to translators and easier to update the link in the future (avoids having to "retranslate" every message).
More logical for translators: in the UI the header comes first, before the "more info" text.
Also: - Link tracking protection moved above sliders - Keyboard support - Dark mode styles - RTL locale support
In the header.
The tweaks you left @ghostwords look good! I like the styling you added too, with the orange outline and expanding animation |
e2ac109
to
8cd7315
Compare
8cd7315
to
aa4fe37
Compare
Fixes #2482.
Should help with #2021, and with the problem of users not noticing the "Disable for this site" button.
adds the following sections to the popup:
each of the sections have the option to drop down expand into a more detailed informational or control panel for each:
EDIT: Widget section forthcoming in #2758