Skip to content
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

several shields panel improvements #1797

Merged
merged 16 commits into from
Mar 13, 2019
Merged

several shields panel improvements #1797

merged 16 commits into from
Mar 13, 2019

Conversation

cezaraugusto
Copy link
Contributor

@cezaraugusto cezaraugusto commented Feb 27, 2019

Submitter Checklist:

fix brave/brave-browser#1894
fix brave/brave-browser#1892
fix brave/brave-browser#2488
fix brave/brave-browser#2565

Test Plan:

  1. Ensure all features on shields work
  1. Ensure shields is accessible via keyboard using tab for navigation, space for actions. enter should work for buttons as well.

  2. Ensure the general (not scripts) blocked list (accessed via arrow down icon) can be scrolled both horizontally and vertically

  3. Issue Click on All scripts allowed shield setting, displays blank drop down for few seconds before loading drop down options brave-browser#2488 is being set as fixed as scripts now use a toggle instead of a select element

  4. Shields should look like the original spec available here

  5. Ensure that once a given resource stat reach 0, the option to show resources blocked should have no interactions other than toggling switch on/off

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

@cezaraugusto cezaraugusto force-pushed the shields-new branch 3 times, most recently from eb20768 to 55c26c0 Compare March 8, 2019 01:06
@cezaraugusto cezaraugusto marked this pull request as ready for review March 8, 2019 02:52
@bsclifton
Copy link
Member

@cezaraugusto does 3rd party device recognition blocking work? (should it be included?)
Screen Shot 2019-03-11 at 4 14 59 PM

@bsclifton
Copy link
Member

bsclifton commented Mar 11, 2019

When going into a details screen (ex: in the above screenshot, you can expand 2 to see the list of 3rd-party trackers blocked), there isn't really an immediate keyboard shortcut (that I found) for going "back" to the general shields screen.

  • If you hit escape, it closes the shields menu completely
  • pushing enter didn't engage the button for me
  • if you manually choose the Go Back button and hit space, it works as expected
  • also if you hit tab at least once and then hit enter

@cezaraugusto
Copy link
Contributor Author

@bsclifton re third-party scripts this is something I'm investigating but it has the same behavior in current release so if that's a bug it isn't new.

re keyboard shortcut there's no immediate closing but you can do it via space bar in the arrow's row or by space/enter in the bottom button. esc closes the extension but I think this is the default behavior

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments I had addressed by @cezaraugusto in follow up comment

I went through test plan, worked great; keyboard worked especially great. This is a SUPER nice upgrade 😄 Well done! 🥇

@cezaraugusto
Copy link
Contributor Author

re fingerprinting I created brave/brave-browser#3698 to keep track.

since test plan works, PR is reviewed and both product and design agree with changes applied here I'm going to hit merge. if for some reason other reviewers still have feedback to give please comment and I can come with a follow-up. thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants