1194258 - Control Center shows 'no tracking elements' message even when it's preffed off

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Brian Grinstead [:bgrins]]

STR:

Open a normal window
Go to mozilla.org
Open control center

See "No tracking elements detected on this page"

If the TP feature isn't working the section should be hidden from the control center.

This is a regression since we have a test in place that's supposed to be confirming this.  The test is checking container.hidden="true" but that's being overridden in CSS by the  #identity-popup[connection="secure-ev"] [when-connection~="secure-ev"] selector.

Comment 1

[Brian Grinstead [:bgrins]]

I can take this

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: MozReview Request: Bug 1194258 - Make sure tracking protection section is hidden in Control Center when it's preffed off;r=paolo

Bug 1194258 - Make sure tracking protection section is hidden in Control Center when it's preffed off;r=paolo

Comment 3

[:Paolo Amadini]

Comment on attachment 8647555 [details]
MozReview Request: Bug 1194258 - Make sure tracking protection section is hidden in Control Center when it's preffed off;r=paolo

https://reviewboard.mozilla.org/r/16005/#review14291

::: browser/base/content/test/general/browser_trackingUI_2.js:27
(Diff revision 1)
> +function hidden(el) {
> +  let win = el.ownerDocument.defaultView;
> +  let display = win.getComputedStyle(el).getPropertyValue("display", null);
> +  let opacity = win.getComputedStyle(el).getPropertyValue("opacity", null);
> +
> +  return display === "none" || opacity === "0";
> +}

Did you copy this from somewhere else? I wonder if there's a version of this function already present in this context.

::: browser/base/content/test/general/browser_trackingUI_2.js:57
(Diff revision 1)
> +  gIdentityHandler._identityPopup.hidden = true;

Is this the correct way to close the popup?

::: browser/themes/shared/controlcenter/panel.inc.css:38
(Diff revision 1)
> +#identity-popup [hidden] {
> +  display: none !important;
> +}

Although it may be obvious from the position of the rule in the file, I'd add a comment on why we need to make this rule "\!important".

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to :Paolo Amadini from comment #3)
> Comment on attachment 8647555 [details]
> MozReview Request: Bug 1194258 - Make sure tracking protection section is
> hidden in Control Center when it's preffed off;r=paolo
> 
> https://reviewboard.mozilla.org/r/16005/#review14291
> 
> ::: browser/base/content/test/general/browser_trackingUI_2.js:27
> (Diff revision 1)
> > +function hidden(el) {
> > +  let win = el.ownerDocument.defaultView;
> > +  let display = win.getComputedStyle(el).getPropertyValue("display", null);
> > +  let opacity = win.getComputedStyle(el).getPropertyValue("opacity", null);
> > +
> > +  return display === "none" || opacity === "0";
> > +}
> 
> Did you copy this from somewhere else? I wonder if there's a version of this
> function already present in this context.
> 

Copied it from browser_trackingUI_1.js (with modifications to take in an element instead of a selector).  I'm not aware of an existing helper function for this, but there must be one somewhere.

> ::: browser/base/content/test/general/browser_trackingUI_2.js:57
> (Diff revision 1)
> > +  gIdentityHandler._identityPopup.hidden = true;
> 
> Is this the correct way to close the popup?

That's how I did it in Bug 1186925 when rewriting the MCB tests.  It seems to work fine but if we find a better way we should make sure to update usage across tests.

> 
> ::: browser/themes/shared/controlcenter/panel.inc.css:38
> (Diff revision 1)
> > +#identity-popup [hidden] {
> > +  display: none !important;
> > +}
> 
> Although it may be obvious from the position of the rule in the file, I'd
> add a comment on why we need to make this rule "\!important".

Alright, I'll add a comment

Comment 5

[Brian Grinstead [:bgrins]]

Sylvestre, when are Aurora builds going out?  This is a pretty critical problem with the new Tracking Protection UI that we'd like to get landed before the release if at all possible.  It's a CSS only change with a test and the patch will be ready to land today.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Tomorrow morning pacific time!

Comment 7

[Javaun Moradi [:javaun]]

Thanks Sylvestre. We can't release build 1 of DevEd without this fix, it's a critical one.

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8647555 [details]
MozReview Request: Bug 1194258 - Make sure tracking protection section is hidden in Control Center when it's preffed off;r=paolo

Bug 1194258 - Make sure tracking protection section is hidden in Control Center when it's preffed off;r=paolo

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> Comment on attachment 8647555 [details]
> MozReview Request: Bug 1194258 - Make sure tracking protection section is
> hidden in Control Center when it's preffed off;r=paolo
> 
> Bug 1194258 - Make sure tracking protection section is hidden in Control
> Center when it's preffed off;r=paolo

Added the comment in CSS.  Let's deal with any test refactoring from Comment 3 / 4 in another bug if needed.

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8647555 [details]
MozReview Request: Bug 1194258 - Make sure tracking protection section is hidden in Control Center when it's preffed off;r=paolo

Approval Request Comment
[Feature/regressing bug #]: 1175702
[User impact if declined]: We incorrectly show that no tracking elements are detected on pages that may have tracking elements inside of any non-private window
[Describe test coverage new/current, TreeHerder]: Updated the browser_trackingUI_2.js test to correctly catch this visual regression
[Risks and why]: Very low, just a CSS change that makes sure that elements set with the hidden=true attribute are actually hidden
[String/UUID change made/needed]: None

Comment 11

[Brian Grinstead [:bgrins]]

Try push on fx-team:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a8a740fec4d
Try push on aurora: https://treeherder.mozilla.org/#/jobs?repo=try&revision=18591ab1e878

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8647555 [details]
MozReview Request: Bug 1194258 - Make sure tracking protection section is hidden in Control Center when it's preffed off;r=paolo

Please land after the r+

Comment 13

[:Paolo Amadini]

Comment on attachment 8647555 [details]
MozReview Request: Bug 1194258 - Make sure tracking protection section is hidden in Control Center when it's preffed off;r=paolo

https://reviewboard.mozilla.org/r/16005/#review14297

Ship It!

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/96e619dc79f0

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/75baf2ad7d32

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/75baf2ad7d32

Comment 17

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:

FF 42 DevEd
Build Id: 20150813124640
OS: Win 7 x64, Mac OS X 10.10.4, Ubuntu 12.04 x86

Comment 18

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 43.0a1 (2015-08-26) Win 7, Ubuntu 14.04, OS X 10.10.