Closed Bug 1175239 Opened 7 years ago Closed 7 years ago

Replace tracking protection drop-down control with a button

Categories

(Firefox :: General, defect, P1)

40 Branch
defect
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 42
Iteration:
42.1 - Jul 13
Tracking Status
firefox42 --- verified

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

(Whiteboard: [fxprivacy] [campaign] [bugday-20150708])

Attachments

(4 files, 3 obsolete files)

Attached image 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).
Attached image proposed-button.png
Flags: firefox-backlog+
Depends on: 1175266
Priority: -- → P1
Bug 1175239 - Break current tracking protection test into 2 and move shared database functionality into head.js;r=MattN
Attachment #8623408 - Flags: review?(MattN+bmo)
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.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Rank: 3
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`.
Attachment #8623408 - Flags: review?(MattN+bmo)
Attachment #8623408 - Flags: review+
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!
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.
Depends on: 1175549
Attachment #8623408 - Attachment is obsolete: true
Attached patch tracking-protection-button.patch (obsolete) — Splinter Review
Includes test.  Waiting to request review since we will want to wait to proceed with this until 42 as discussed.
Points: --- → 3
Flags: qe-verify+
Iteration: --- → 42.1 - Jul 13
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
Attachment #8623784 - Attachment is obsolete: true
Comment on attachment 8627966 [details] [diff] [review]
tracking-protection-buttons.patch

Forgot review flag - see Comment 8
Attachment #8627966 - Flags: review?(ttaubert)
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.
Attachment #8627970 - Flags: review?(ttaubert)
Attachment #8627970 - Flags: review?(ttaubert) → review+
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?
Attachment #8627966 - Flags: review?(ttaubert) → review+
(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.
QA Contact: mwobensmith
Updated based on feedback in Comment 11.  Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee9c229ee29a
Attachment #8627966 - Attachment is obsolete: true
Attachment #8628369 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/90f85a1e2ac4
https://hg.mozilla.org/mozilla-central/rev/138bfb083317
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team][fxprivacy] → [fxprivacy]
Target Milestone: --- → Firefox 42
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?
Flags: needinfo?(ttaubert)
Flags: needinfo?(bgrinstead)
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
QA Whiteboard: [bugday-20150708]
Whiteboard: [fxprivacy] → [fxprivacy][bugday-20150708]
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
(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.
Flags: needinfo?(bgrinstead)
As per Comment 17 and Comment 18, Marking the bug as verified!
Status: RESOLVED → VERIFIED
(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.
Flags: needinfo?(bgrinstead)
(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
Flags: needinfo?(ttaubert)
Flags: needinfo?(bgrinstead)
Flags: needinfo?(agrigas)
(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.
Flags: needinfo?(agrigas)
(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.
Flags: needinfo?(agrigas)
(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...
Flags: needinfo?(agrigas)
(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..
Flags: needinfo?(agrigas)
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.
(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. :)
(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.
Flags: needinfo?(agrigas)
Depends on: 1182187
Whiteboard: [fxprivacy][bugday-20150708] → [fxprivacy] [campaign] [bugday-20150708]
You need to log in before you can comment on or make changes to this bug.