-
-
Notifications
You must be signed in to change notification settings - Fork 414
fix: change tab order on tracking domains page #3152
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?
fix: change tab order on tracking domains page #3152
Conversation
|
Hello and thank you for the PR! I have a few initial observations below. Let me know if anything is unclear.
|
|
Also, when keyboard navigating on the options page, the focus outlines around the "x" and the domain names are larger/longer than they should be. |
|
Thanks for the feedback, I will work on cleaning those things up. I think for the alignment issues I will see if using CSS grid instead of flexbox helps, it should be easier to keep things in structured columns. Shouldn't be much trouble. The RTL mode things also make sense. |
08ea30c to
becfc36
Compare
|
I updated the PR. I thought about it some more and I don't think fixing this requires adding any flex layout stuff. Really the main thing is swapping the order we define the html elements in and that would resolve the tab order. Which eliminates all of the changes which affected the alignment of the toggles. I did adjust the css for the "undo" icon a bit, because I noticed that the "remove" icon wraps whenever the "undo" icon is displayed. Also I moved the DNT icon to the right of the fqdn because it is focusable element and the tab flow of focusing on the fqdn, then going left to the DNT icon, then right again to the toggle seemed awkward. I am wondering if that even stood out to you as a problem to begin with, but also I want to make sure it's ok to move the icon. If it needs to stay on the left I can put it back and either leave the tabbing behavior as-is or adjust it a different way. Finally, for the RTL support I can work on all of that as part of #3103. This updated PR at least preserves the existing functionality in that regard. |
This comment was marked as outdated.
This comment was marked as outdated.
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.
Nice, this seems close to a solution for #3107.
It looks like the "x"s are no longer aligned to the toggles and domain names the way they are now.
Let me know if anything is unclear and thanks again for working on this.
| height: 40px; | ||
| overflow: hidden; | ||
| line-height: normal; | ||
| contain: layout; |
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 does this do?
| } | ||
|
|
||
| .dnt-compliant { | ||
| float: left; |
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.
Also I moved the DNT icon to the right of the fqdn because it is focusable element and the tab flow of focusing on the fqdn, then going left to the DNT icon, then right again to the toggle seemed awkward. I am wondering if that even stood out to you as a problem to begin with, but also I want to make sure it's ok to move the icon. If it needs to stay on the left I can put it back and either leave the tabbing behavior as-is or adjust it a different way.
I think we should leave the DNT icon where it is for now, but fix its tab order.
| pkg | ||
| release-utils/config.sh | ||
|
|
||
| # development tooling |
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 precisely produces .devcontainer/? "development tooling" is generic; lots of entries above are also "development tooling".
Also, suggest placing this next to related entries, like maybe following the Vim swap files entry?
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 for contributing to Privacy Badger! In addition to Alexei's feedback, could you double check if your CSS changes also need to be applied inside media queries for different screen sizes?
| left: 340px; | ||
| bottom: 15px; | ||
| position: fixed; | ||
| left: 345px; |
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 fixes #3107
I used PR #3122 as a starting point, and made sure the tests pass and the UI was not affected (too badly). Used flexbox on the tracking domain rows so that tabbing works implicitly with the HTML elements.
Attempted to knock out #3103 also since they are related, but I wasn't able to find a quick, clean way to make the toggle-switch direction-agnostic so I'll leave that for another day.