1175239 - Replace tracking protection drop-down control with a button

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Brian Grinstead [:bgrins]]

Attachment: current-dropdown.png

The functionality is otherwise identical: clicking the button always reverts tracking protection to its opposite state (enabled/disabled) and then reloads the page.

Currently this is a dropdown (see screenshot).

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: proposed-button.png

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: MozReview Request: Bug 1175239 - Break current tracking protection test into 2 and move shared database functionality into head.js;r=MattN

Bug 1175239 - Break current tracking protection test into 2 and move shared database functionality into head.js;r=MattN

Comment 3

[Brian Grinstead [:bgrins]]

This is just part 1, breaking up the test file into 2.  The first tests that the doorhanger is showing when not whitelisted, and the second is testing that the doorhanger never shows up when the feature is off.

I'll add a third test when I switch the control to a button, which will directly test the UI elements themselves.

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8623408 [details]
MozReview Request: Bug 1175239 - Break current tracking protection test into 2 and move shared database functionality into head.js;r=MattN

https://reviewboard.mozilla.org/r/11525/#review9945

::: browser/base/content/test/general/head.js:688
(Diff revision 1)
> +    QueryInterface: function(iid)
> +    {

Nit: wrapping is non-standard

::: browser/base/content/test/general/head.js:685
(Diff revision 1)
> +  let deferred = Promise.defer();

Nit: deferreds are deprecated. Feel free to leave it if you want.

::: browser/base/content/test/general/head.js:668
(Diff revision 1)
> +  var TABLE = "urlclassifier.trackingTable";
> +  Services.prefs.setCharPref(TABLE, "test-track-simple");

I wouldn't complain if you change the `var`s to `let`/`const` while you're changing blame anyways :)

You could also use the new method syntax on `listener`.

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8623408 [details]
MozReview Request: Bug 1175239 - Break current tracking protection test into 2 and move shared database functionality into head.js;r=MattN

https://reviewboard.mozilla.org/r/11525/#review9947

Ship It!

Comment 6

[Brian Grinstead [:bgrins]]

Thanks for the review - I've addressed the comments and am going to file a new bug to land these test changes so that we can keep the Target Milestone clean, since we may end up waiting to switch the button UI itself until 42.

The plan for the rest of this bug will be to switch to the button UI and add a third test that hits the UI directly.

Comment 7

[Brian Grinstead [:bgrins]]

Attachment: tracking-protection-button.patch

Includes test.  Waiting to request review since we will want to wait to proceed with this until 42 as discussed.

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: tracking-protection-buttons.patch

I copied the styling from the in-content buttons: https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#178

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8627966 [details] [diff] [review]
tracking-protection-buttons.patch

Forgot review flag - see Comment 8

Comment 10

[Brian Grinstead [:bgrins]]

Attachment: test-cleanup.patch

Test only - this is applied on top of the button changes.  Mostly whitespace and capitalization updates, just prepping so that when we add a third test things will be consistent and neat.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8627966 [details] [diff] [review]
tracking-protection-buttons.patch

Review of attachment 8627966 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great. Thanks :) r=me with the few issues fixed.

::: browser/base/content/test/general/browser_trackingUI_1.js
@@ +30,5 @@
> +function clickButton(sel) {
> +  let win = gBrowser.ownerGlobal;
> +  let evt = new win.Event("command");
> +  let el = win.document.querySelector(sel);
> +  el.dispatchEvent(evt);

Did you try button.click()? Or button.doCommand()?

::: browser/components/controlcenter/content/panel.inc.xul
@@ +55,5 @@
>  
> +          <button
> +            label="&trackingProtection.unblock.label;"
> +            class="identity-popup-button"
> +            accesskey="&trackingProtection.unblock.accesskey;"

Nit: Please format attributes like we did for all the other elements.

@@ +56,5 @@
> +          <button
> +            label="&trackingProtection.unblock.label;"
> +            class="identity-popup-button"
> +            accesskey="&trackingProtection.unblock.accesskey;"
> +            id="tracking-action-unblock"

Nit: Please move the id="" attribute behind <button .

@@ +62,5 @@
> +          <button
> +            label="&trackingProtection.block.label;"
> +            class="identity-popup-button"
> +            accesskey="&trackingProtection.block.accesskey;"
> +            id="tracking-action-block"

Nit: Please move the id="" attribute behind <button .

::: browser/themes/shared/controlcenter/panel.inc.css
@@ +77,5 @@
> +  --button-text-color: #333;
> +  --button-background: #fbfbfb;
> +  --button-border-color: #c1c1c1;
> +  --button-background-hover: #ebebeb;
> +  --button-background-active: #dadada;

Please replace the var(..) occurrences with the respective values, we don't really need CSS variables here.

@@ +90,5 @@
> +  border: 1px solid var(--button-border-color);
> +  -moz-border-top-colors: none !important;
> +  -moz-border-right-colors: none !important;
> +  -moz-border-bottom-colors: none !important;
> +  -moz-border-left-colors: none !important;

Do we really need those overrides? Looks like in the in-content prefs they're overriding non-button styles maybe?

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Tim Taubert [:ttaubert] from comment #11)
> Comment on attachment 8627966 [details] [diff] [review]
> tracking-protection-buttons.patch
> 
> Review of attachment 8627966 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks great. Thanks :) r=me with the few issues fixed.
> 
> ::: browser/base/content/test/general/browser_trackingUI_1.js
> @@ +30,5 @@
> > +function clickButton(sel) {
> > +  let win = gBrowser.ownerGlobal;
> > +  let evt = new win.Event("command");
> > +  let el = win.document.querySelector(sel);
> > +  el.dispatchEvent(evt);
> 
> Did you try button.click()? Or button.doCommand()?

Had tried click but not doCommand, which seems to work.  So updated to doCommand.

> @@ +90,5 @@
> > +  border: 1px solid var(--button-border-color);
> > +  -moz-border-top-colors: none !important;
> > +  -moz-border-right-colors: none !important;
> > +  -moz-border-bottom-colors: none !important;
> > +  -moz-border-left-colors: none !important;
> 
> Do we really need those overrides? Looks like in the in-content prefs
> they're overriding non-button styles maybe?

Yeah, not really sure why those were set.  I assumed it was needed for one of the platforms, but happy to remove them since it's a lot of extra noise.

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: tracking-protection-buttons.patch

Updated based on feedback in Comment 11.  Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee9c229ee29a

Comment 14

[Brian Grinstead [:bgrins]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/90f85a1e2ac4
remote:   https://hg.mozilla.org/integration/fx-team/rev/138bfb083317

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/90f85a1e2ac4
https://hg.mozilla.org/mozilla-central/rev/138bfb083317

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 8628369 [details] [diff] [review]
tracking-protection-buttons.patch

>+/* IN-CONTENT-PREF STYLE BUTTONS */

Why are we doing this? This isn't in-content UI.

>+.identity-popup-button {
>+  -moz-appearance: none;
>+  margin: 5px 0;
>+  height: 30px;
>+  color: #333;
>+  line-height: 20px;
>+  border: 1px solid #c1c1c1;
>+  border-radius: 2px;
>+  background-color: #fbfbfb;
>+}

Has this been this tested with high contrast themes?

Comment 17

[Hossain Al Ikram [:ikram] (QA Contact)]

I have seen the bug by activating Tracking Protection (By reading the SuMo KB - https://support.mozilla.org/en-US/kb/tracking-protection-firefox ) on Windows 7 64 bit with Nightly 41.0a1.

The bug is fixed (or feature is implemented) on latest Nightly 42.0a1!

Build ID : 20150708030204
User Agent : Mozilla/5.0 (Windows NT 6.1; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0

Comment 18

[Ashickur Rahman]

I have seen the bug by activating Tracking Protection (By reading the SuMo KB - https://support.mozilla.org/en-US/kb/tracking-protection-firefox ) on Mac OS X with Nightly 41.0a1.

The feature is implemented on latest Nightly 42.0a1, Build ID: 20150708030204 , User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:42.0) Gecko/20100101 Firefox/42.0

Comment 19

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [:dao] from comment #16)
> Comment on attachment 8628369 [details] [diff] [review]
> tracking-protection-buttons.patch
> 
> >+/* IN-CONTENT-PREF STYLE BUTTONS */
> 
> Why are we doing this? This isn't in-content UI.
> 
> >+.identity-popup-button {
> >+  -moz-appearance: none;
> >+  margin: 5px 0;
> >+  height: 30px;
> >+  color: #333;
> >+  line-height: 20px;
> >+  border: 1px solid #c1c1c1;
> >+  border-radius: 2px;
> >+  background-color: #fbfbfb;
> >+}
> 
> Has this been this tested with high contrast themes?

It's copying over the styles to match the mockup, which uses in-content style buttons: https://bugzilla.mozilla.org/attachment.cgi?id=8623220.  Figured it's easier to reuse something already implemented and has hover/active/disabled states sorted out than to do it from scratch.  The entire CC could probably use a pass-through for high contrast themes - I filed Bug 1181803 for that.

Comment 20

[Hossain Al Ikram [:ikram] (QA Contact)]

As per Comment 17 and Comment 18, Marking the bug as verified!

Comment 21

[Dão Gottwald [:dao]]

(In reply to Brian Grinstead [:bgrins] from comment #19)
> (In reply to Dão Gottwald [:dao] from comment #16)
> > >+/* IN-CONTENT-PREF STYLE BUTTONS */
> > 
> > Why are we doing this? This isn't in-content UI.
> 
> It's copying over the styles to match the mockup, which uses in-content
> style buttons: https://bugzilla.mozilla.org/attachment.cgi?id=8623220.

Why does the mockup do that then? We don't use this anywhere else in the primary UI.

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [:dao] from comment #21)
> (In reply to Brian Grinstead [:bgrins] from comment #19)
> > (In reply to Dão Gottwald [:dao] from comment #16)
> > > >+/* IN-CONTENT-PREF STYLE BUTTONS */
> > > 
> > > Why are we doing this? This isn't in-content UI.
> > 
> > It's copying over the styles to match the mockup, which uses in-content
> > style buttons: https://bugzilla.mozilla.org/attachment.cgi?id=8623220.
> 
> Why does the mockup do that then? We don't use this anywhere else in the
> primary UI.

Forwarding question to Aislinn

Comment 23

[Ash Grigas]

(In reply to Brian Grinstead [:bgrins] from comment #22)
> (In reply to Dão Gottwald [:dao] from comment #21)
> > (In reply to Brian Grinstead [:bgrins] from comment #19)
> > > (In reply to Dão Gottwald [:dao] from comment #16)
> > > > >+/* IN-CONTENT-PREF STYLE BUTTONS */
> > > > 
> > > > Why are we doing this? This isn't in-content UI.
> > > 
> > > It's copying over the styles to match the mockup, which uses in-content
> > > style buttons: https://bugzilla.mozilla.org/attachment.cgi?id=8623220.
> > 
> > Why does the mockup do that then? We don't use this anywhere else in the
> > primary UI.
> 
> Forwarding question to Aislinn

We're using a button because its very important to provide on click disabling for this feature. This isn't primary ui - its disclosed under a menu and is part of a new design of the entire control center.

Comment 24

[Dão Gottwald [:dao]]

(In reply to agrigas from comment #23)
> (In reply to Brian Grinstead [:bgrins] from comment #22)
> > (In reply to Dão Gottwald [:dao] from comment #21)
> > > (In reply to Brian Grinstead [:bgrins] from comment #19)
> > > > (In reply to Dão Gottwald [:dao] from comment #16)
> > > > > >+/* IN-CONTENT-PREF STYLE BUTTONS */
> > > > > 
> > > > > Why are we doing this? This isn't in-content UI.
> > > > 
> > > > It's copying over the styles to match the mockup, which uses in-content
> > > > style buttons: https://bugzilla.mozilla.org/attachment.cgi?id=8623220.
> > > 
> > > Why does the mockup do that then? We don't use this anywhere else in the
> > > primary UI.
> > 
> > Forwarding question to Aislinn
> 
> We're using a button because its very important to provide on click
> disabling for this feature. This isn't primary ui - its disclosed under a
> menu and is part of a new design of the entire control center.

That wasn't at all my question. My question is why that button is replicating in-content styling that we only use for buttons in in-content UI such as about:preferences and about:addons.

Comment 25

[Ash Grigas]

(In reply to Dão Gottwald [:dao] from comment #24)
> (In reply to agrigas from comment #23)
> > (In reply to Brian Grinstead [:bgrins] from comment #22)
> > > (In reply to Dão Gottwald [:dao] from comment #21)
> > > > (In reply to Brian Grinstead [:bgrins] from comment #19)
> > > > > (In reply to Dão Gottwald [:dao] from comment #16)
> > > > > > >+/* IN-CONTENT-PREF STYLE BUTTONS */
> > > > > > 
> > > > > > Why are we doing this? This isn't in-content UI.
> > > > > 
> > > > > It's copying over the styles to match the mockup, which uses in-content
> > > > > style buttons: https://bugzilla.mozilla.org/attachment.cgi?id=8623220.
> > > > 
> > > > Why does the mockup do that then? We don't use this anywhere else in the
> > > > primary UI.
> > > 
> > > Forwarding question to Aislinn
> > 
> > We're using a button because its very important to provide on click
> > disabling for this feature. This isn't primary ui - its disclosed under a
> > menu and is part of a new design of the entire control center.
> 
> That wasn't at all my question. My question is why that button is
> replicating in-content styling that we only use for buttons in in-content UI
> such as about:preferences and about:addons.

They are design to be the same as this https://www.dropbox.com/s/mi22sljoldvp7no/Screenshot%202015-07-09%2011.56.19.png?dl=0

in preferneces...

Comment 26

[Dão Gottwald [:dao]]

(In reply to agrigas from comment #25)
> > That wasn't at all my question. My question is why that button is
> > replicating in-content styling that we only use for buttons in in-content UI
> > such as about:preferences and about:addons.
> 
> They are design to be the same as this
> https://www.dropbox.com/s/mi22sljoldvp7no/Screenshot%202015-07-09%2011.56.19.
> png?dl=0
> 
> in preferneces...

I know that. My question is why. We are not using that design for any other button outside of about:preferences, about:addons & Co..

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

I can't speak for UX but if we want to start using the "in-content" design for buttons as well for chrome in certain places then we'd have to start somewhere. I also think this discussion would be much more efficient if it took place on IRC. We can always file new bugs based on the outcome.

Comment 28

[Dão Gottwald [:dao]]

(In reply to Tim Taubert [:ttaubert] from comment #27)
> I can't speak for UX but if we want to start using the "in-content" design
> for buttons as well for chrome in certain places then we'd have to start
> somewhere.

This isn't the way to start. It gets us inconsistency from a user's point of view and maintenance issues since this kind of one-off styling isn't reusable. The right way to go about this would be to put that styling in a central place. But at that point we're beyond this bug's scope. Within this bug's scope, we should just have followed existing related UI. I guess I'll file a new bug on this.

> I also think this discussion would be much more efficient if it
> took place on IRC.

Not for me, async communication allows me to prepare dinner and such. :)

Comment 29

[Ash Grigas]

(In reply to Dão Gottwald [:dao] from comment #28)
> (In reply to Tim Taubert [:ttaubert] from comment #27)
> > I can't speak for UX but if we want to start using the "in-content" design
> > for buttons as well for chrome in certain places then we'd have to start
> > somewhere.
> 
> This isn't the way to start. It gets us inconsistency from a user's point of
> view and maintenance issues since this kind of one-off styling isn't
> reusable. The right way to go about this would be to put that styling in a
> central place. But at that point we're beyond this bug's scope. Within this
> bug's scope, we should just have followed existing related UI. I guess I'll
> file a new bug on this.
> 
> > I also think this discussion would be much more efficient if it
> > took place on IRC.
> 
> Not for me, async communication allows me to prepare dinner and such. :)

The re-styling of this entire panel - internally now 'the control center' is part of a larger design initiative to update older styles to better match the design changes we've done in other parts of the in-content ui. We're making things more consistent over time by leveraging new styles and making them more universal.