Shields v2 GA version

[cezaraugusto]

npm run test-unit

closes brave/brave-browser#1339
closes brave/brave-browser#771
closes brave/brave-browser#507
closes brave/brave-browser#471

[srirambv]

• Dropdown text is not visible until on hover
• Site icon is too small

• Cookie blocked count isn't shown
  

• Site icon and total block count could use some more padding to right

• Global Shield settings should open chrome://settings/shields but currently only goes to chrome://settings/

• Would be better to have the following renamed keeping it similar to Allow all scripts and whats there on Browser-laptop

  • All device recognition blocked to Block all device recognition
  • All device recognition allowed to Allow all device recognition
  • All script blocked to Block all script
  • All cookies allowed to Allow all cookies
  • All cookies blocked to Block all cookies

• Connections encrypted (HTTPSE) doesn't have a switch to disable? Can only be disabled from global settings?

• Down could use the same red colour
  

cc: @rebron for text changes

[bsclifton]

@srirambv I had these notes about your feedback

• chrome://settings/shields doesn't seem routable; I think we would need to change the settings page to make that jump to the appropriate place. If you wanted to capture an issue for that, it might be a good first bug for a contributor 😄
• The color for Down matches the spec perfectly. I don't think we should change this
• ++ from me too on preferring the site icon to be a little bigger / having some padding. Good eye 👁 👍

@rebron some notes for you

• I agree with @srirambv about the text looking unusual for cookies / device recognition (formally known as fingerprinting protection). Having Allow or Block at the beginning makes things easier at a glance. @cezaraugusto matched the spec exactly though, so if we prefer the existing text, I'm OK with it too 😄 cc: @karenkliu
• as @srirambv mentioned, we lose the ability to toggle HTTPSE for this specific site with this new design. I want to confirm that we're OK with this (from a design perspective and from a security/privacy perspective too). If this is expected, let me know and I'll create a security review ticket so that @diracdeltas or team can review 😄

@cezaraugusto Great work on this one! 😄

Trying it out and it looks and feels MUCH better

• I got into a weird situation where I set scripts to All scripts blocked and wasn't able to change back to 3rd party scripts blocked. This makes sense because we currently just have on/off for scripts. Should we remove the 3rd party scripts blocked item for now? (since it doesn't work)
• I noticed that the cookie blocking didn't show the count also. It seemed to work good though! Can we fix this?

Besides the two issues above (and possibly any changes @rebron might want to make), this gets an approval from me 😄 I'll save the official approve for once we resolve those items

[srirambv]

chrome://settings/shields doesn't seem routable;

This is mentioned in the spec that it should be open that. Currently direct link to open Global shields settings isn't implemented so this should be changed after thats done.

[srirambv]

Also noticed there is not details shown of items that are blocked. Is this being worked on a different issue?

[bsclifton]

@srirambv I believe that will be covered in brave/brave-browser#1196 (and it may show under advanced). Maybe @rebron or @karenkliu can confirm?

[rebron]

per @srirambv and @bsclifton notes @cezaraugusto

• Let's fix the drop down text to make visible.
  Site icon size can be fixed later.
  Let's get the cookie block count fixed.
  Link to main settings page is fine for now.

• Let's go with sri's text changes
  All device recognition blocked to 'Block all device recognition'
  All device recognition allowed to 'Allow all device recognition'
  All script blocked to 'Block all script'
  All cookies allowed to 'Allow all cookies'
  All cookies blocked to 'Block all cookies'

The https toggle is out by design and cleared by security team. We're showing connections encrypted and have it as global setting instead.

[rebron]

per discussion with @cezaraugusto and @bsclifton we'll also display ads and trackers blocked when a user selects Ad and Trackers row. Content should be the same as in current shields.

[karenkliu]

Just catching up here; I agree with most of the comments above except:

• The dropdown menu isn't matching spec; It should look like this:
  
  The text should be visible even without hover, also it needs more padding on all sides.

• Regarding the naming:
  The design rationale behind the grammatically inconsistent labels is because _____ blocked, is easier for the user to recognize rather than scanning a long list of Blocked ____, Blocked ____, etc.
  However I don't want to hold this up anymore than it is so just leave it for now and we can review the labels at a later point.

• "Down" being this color is correct:

It is grey instead of red to reinforce the association that Shields is in a disabled state. Do not change this to red.

Lastly, I feel that for next round (1.0) when we're shipping shields again, it would be helpful for everyone including QA to be aware of what's in the design, influence it, and voice concerns/feedback, before it gets to dev-QA and causes a time-crunch at last minute. My apologies for not being aware that more people needed to have eyes on the design before @rebron and I put the Shields design into Github. I wasn't aware of all the stakeholders who should have been part of the design process. We'll do better next time! A retro might be a good thing to schedule after all this craziness is over so we can figure out how to make this process smoother for everyone.

[cezaraugusto]

closing in favor of #78