Skip to content

Conversation

BeniRupp
Copy link

@BeniRupp BeniRupp commented Aug 5, 2025

Previously the tab order in the list of tracking domains was: domain, x-Button, toggle.

Now, the order is as expected: domain, toggle, x-Button. This was achieved by changing the order of the DOM nodes inside a tracking domain row. The layout was then fixed by replacing the float layout with some grid and flex layouts to have a more robust layout with fewer magic numbers.

Fixes #3107

This PR was part of the Hackergarten Stuttgart (Hack no. 427).
Co-authored by @itlinuxmaker

Previously the tab order in the list of tracking domains was: domain,
x-Button, toggle.

Now, the order is as expected: domain, toggle, x-Button. This was
achieved by changing the order of the DOM nodes inside a tracking domain
row. The layout was then fixed by replacing the float layout with some
grid and flex layouts to have a more robust layout with fewer magic
numbers.

Fixes EFForg#3107
@ghostwords
Copy link
Member

Thank you!! I will be able to review your contribution mid next week.

@ghostwords
Copy link
Member

A couple of our functional tests are failing. I wonder if it's because the tests need to be updated (probably because the selectors changed) or if they flagged a regression.

@BeniRupp
Copy link
Author

BeniRupp commented Aug 7, 2025

Oh.. yes, that could be a problem with the selectors. I can have a look on the tests next week.

@lenacohen
Copy link
Contributor

Thank you for working on this!

I noticed that your changes caused the sliders and their icons to shift left. We want to keep the layout of the popup the same.
Before:
Screenshot 2025-08-08 at 5 23 38 PM

After:
Screenshot 2025-08-08 at 5 23 49 PM

On the options page, the focused domain should have the default highlight styling, rather than this orange box:
Screenshot 2025-08-08 at 5 12 56 PM

@ghostwords
Copy link
Member

@BeniRupp I'm holding off looking further until you had a chance to respond to our initial feedback. Let us know if you have any questions please.

@lenacohen
Copy link
Contributor

Let us know if you have any questions about our feedback. Thanks again for contributing!

@itlinuxmaker

This comment was marked as off-topic.

@BeniRupp
Copy link
Author

@itlinuxmaker I think the requirement not to change the layout is an acceptable requirement for a UI application. Furthermore, the tests are also broken. So we definitly have to rework the PR.

@ghostwords @lenacohen Unfortunately, I don't have much time at the moment and won't be able to continue working on the ticket anytime soon. If it's okay with you, I'll be happy to take another look at it in 3-4 weeks.

@itlinuxmaker

This comment was marked as off-topic.

@ghostwords
Copy link
Member

@itlinuxmaker Providing, accepting and acting on feedback is a standard part of software development. Furthermore, please note that this project is governed by the EFF Public Projects Code of Conduct. Specifically:

Please be kind and courteous. Please take care not to be, or appear to be, mean or rude. Keeping this project a welcoming place is a shared responsibility involving each of us.

@ghostwords
Copy link
Member

@BeniRupp No problem, thanks for letting us know! We'll keep this open until the bug gets fixed here or elsewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wrong keyboard navigation order for toggles and "x" icons on options page

4 participants