Closed Bug 1175858 Opened 5 years ago Closed 5 years ago

TP shield should be animated when TP has blocked content on the page

Categories

(Firefox :: General, defect, P1)

defect
Points:
5

Tracking

()

VERIFIED FIXED
Iteration:
42.2 - Jul 27

People

(Reporter: past, Assigned: bgrins)

References

Details

(Whiteboard: [fxprivacy][campaign])

Attachments

(7 files, 2 obsolete files)

Attached image Screenshot
The TP shield should animate in order to call attention to the URL bar when elements on the page have been blocked by TP.
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
See Also: → 1175689
Rank: 1
Priority: -- → P1
Points: --- → 5
Flags: qe-verify+
Depends on: 1175689
I'll take a look at this
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Iteration: --- → 42.1 - Jul 13
QA Contact: mwobensmith
Attached patch tp-shield-animate.patch (obsolete) — Splinter Review
Stashing this here, waiting for Bug 1175689 to land
Iteration: 42.1 - Jul 13 → 42.2 - Jul 27
Bug 1175858 - TP shield should be animated when TP has blocked content on the page;r=MattN
Attachment #8634365 - Flags: review?(MattN+bmo)
Attached video tp-shield.mov (obsolete) —
screencast with patch applied
Comment on attachment 8634365 [details]
MozReview Request: Bug 1175858 - Tracking Protection shield should be animated in when content is blocked on the page;r=MattN

Bug 1175858 - TP shield should be animated when TP has blocked content on the page;r=MattN
Comment on attachment 8634365 [details]
MozReview Request: Bug 1175858 - Tracking Protection shield should be animated in when content is blocked on the page;r=MattN

https://reviewboard.mozilla.org/r/13385/#review12029

::: browser/themes/shared/identity-block/identity-block.inc.css:86
(Diff revision 2)
> +   /* Only animate the shield *in*, when it disappears hide it immediately. */

Nit: outdent this by 1 character

::: browser/themes/shared/identity-block/identity-block.inc.css:82
(Diff revision 2)
>  #tracking-protection-icon:not([state]) {
> -  display: none;
> +  margin-left: -16px;
> +  pointer-events: none;

With this patch we re-animate the icon in whenever you switch back to the tab which seems too distracting. We should see what EME is doing for its animation anchor icon.
Attachment #8634365 - Flags: review?(MattN+bmo)
Aislinn, do you think that we should animate the shield when switching tabs from one without the shield to one that does have it?  I don't think that we do that anywhere else in the UI.  I'll upload a screencast next of what it looks like without the animation.
Flags: needinfo?(agrigas)
screencast without the animation when switching between tabs
Attachment #8633034 - Attachment is obsolete: true
Attachment #8634392 - Attachment is obsolete: true
(In reply to Brian Grinstead [:bgrins] from comment #8)
> Created attachment 8634998 [details]
> tp-animate-on-tab-switch.mov
> 
> Aislinn, do you think that we should animate the shield when switching tabs
> from one without the shield to one that does have it?  I don't think that we
> do that anywhere else in the UI.  I'll upload a screencast next of what it
> looks like without the animation.

No animation needed when switching between tabs.
Flags: needinfo?(agrigas)
Comment on attachment 8634365 [details]
MozReview Request: Bug 1175858 - Tracking Protection shield should be animated in when content is blocked on the page;r=MattN

Bug 1175858 - Tracking Protection shield should be animated in when content is blocked on the page;r=MattN
Attachment #8634365 - Attachment description: MozReview Request: Bug 1175858 - TP shield should be animated when TP has blocked content on the page;r=MattN → MozReview Request: Bug 1175858 - Tracking Protection shield should be animated in when content is blocked on the page;r=MattN
Attachment #8634365 - Flags: review?(MattN+bmo)
Brian - ca you post a screen recording of the final animation with old shield hidden if possible so I can share it wit the ux team - or let me know when its in nightly and i'll grab one...
Attached video tp-animate.mov
screencast of current animation
Whiteboard: [fxprivacy] → [fxprivacy] [campaign]
Comment on attachment 8634365 [details]
MozReview Request: Bug 1175858 - Tracking Protection shield should be animated in when content is blocked on the page;r=MattN

https://reviewboard.mozilla.org/r/13385/#review12349

::: browser/base/content/test/general/browser_trackingUI_4.js:5
(Diff revision 3)
> +// Test that the Tracking Protection icon is properly animated in the identity
> +// block when loading tabs and switching between tabs.
> +// See also Bug 1175858.

This should be in /* … */ (one asterisk on the starting line) so it shows up on https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/

::: browser/base/content/test/general/browser_trackingUI_4.js:53
(Diff revision 3)
> +  ok (!TrackingProtection.icon.hasAttribute("state"), "icon: no state");
> +  ok (TrackingProtection.icon.hasAttribute("animate"), "icon: animate");
> +  is (TrackingProtection._loadingTab, benignTab, "Tab request detected");

Why the spaces after the function names?

::: browser/base/content/test/general/browser_trackingUI_4.js:94
(Diff revision 3)
> +  browser = gBrowser;

Nope. This adds to confusion about gBrowser. It's not a <browser>, it's a <tabbrowser>. Why do we even need to alias this? It should be passed as an argument to `testTrackingProtectionForBrowser` (where I also don't think "ForBrowser" is appropriate). If you do keep a gBrowser alias, it should be called tabbrowser.

::: browser/base/content/test/general/browser_trackingUI_4.js:50
(Diff revision 3)
> +  let benignTab = browser.selectedTab = browser.addTab();
> +  yield promiseTabLoadEvent(benignTab, BENIGN_PAGE);

BrowserTestUtils.openNewForegroundTab(…)

::: browser/base/content/test/general/browser_trackingUI_4.js:58
(Diff revision 3)
> +  let trackingTab = browser.selectedTab = browser.addTab();
> +  yield promiseTabLoadEvent(trackingTab, TRACKING_PAGE);

BrowserTestUtils.openNewForegroundTab(…)

::: browser/base/content/test/general/browser_trackingUI_4.js:30
(Diff revision 3)
> +  let def = Promise.defer();

Promise.defer is deprecated so we don't use it in new code. Use the Promise constructor instead.

::: browser/base/content/test/general/browser_trackingUI_4.js:1
(Diff revision 3)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Tests are normally PD and get that my default now so no header is needed.

::: browser/base/content/browser-trackingprotection.js:69
(Diff revision 3)
> +    if (request) {
> +      this._loadingTab = tab;
> +      this.icon.setAttribute("animate", "true");
> +    } else if (tab != this._loadingTab) {
> +      this.icon.removeAttribute("animate");
> +    }

The \_loadingTab state seems fragile to me as the onSecurityChange notification ordering could change. I think we should look into doing this without additional state by determining the initial load via progress listener arguments and/or passing extra info in callers who don't go through the docshell but call this method directly.
Attachment #8634365 - Flags: review?(MattN+bmo)
(In reply to Matthew N. [:MattN] from comment #14)
> ::: browser/base/content/test/general/browser_trackingUI_4.js:94
> (Diff revision 3)
> > +  browser = gBrowser;
> 
> Nope. This adds to confusion about gBrowser. It's not a <browser>, it's a
> <tabbrowser>. Why do we even need to alias this? It should be passed as an
> argument to `testTrackingProtectionForBrowser` (where I also don't think
> "ForBrowser" is appropriate). If you do keep a gBrowser alias, it should be
> called tabbrowser.

This is aliased to be consistent with other TP tests so that we can run the same functionality in a private and non-private window (it assigns browser = privateWin.gBrowser in the next test).  In this test it'd be easy to pass browser as an argument but in others it's just more convenient to store it in a variable accessible from utility functions and the like.  I'll update the variable name and fix the formatting issues across all the TP tests in a second patch.
Attachment #8634365 - Flags: review?(MattN+bmo)
Comment on attachment 8634365 [details]
MozReview Request: Bug 1175858 - Tracking Protection shield should be animated in when content is blocked on the page;r=MattN

Bug 1175858 - Tracking Protection shield should be animated in when content is blocked on the page;r=MattN
Bug 1175858 - Style updates for browser tracking protection tests;r=MattN
Attachment #8637506 - Flags: review?(MattN+bmo)
Comment on attachment 8637506 [details]
MozReview Request: Bug 1175858 - Style updates for browser tracking protection tests;r=MattN

https://reviewboard.mozilla.org/r/13901/#review12465

::: browser/base/content/test/general/browser_trackingUI_4.js:13
(Diff revision 1)
> -let browser = null;
> +let tabbrowser = null;

I still think this global should just be passed as an argument as it makes it easier to understand where things are coming from to reason about a function and it makes it easier to refactor out helpers. I don't think consistency with other tests that use this bad pattern is that good of a reason to leave it but I'll leave it up to you.
Attachment #8637506 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8634365 [details]
MozReview Request: Bug 1175858 - Tracking Protection shield should be animated in when content is blocked on the page;r=MattN

https://reviewboard.mozilla.org/r/13385/#review12461

::: browser/base/content/browser-trackingprotection.js:61
(Diff revision 4)
> +    // Only animate the shield if the event was not fired directly from
> +    // the tabbrowser (due to a browser change).
> +    if (isSimulated) {
> +      this.icon.removeAttribute("animate");
> +    } else {
> +      this.icon.setAttribute("animate", "true");
> +    }

Much cleaner IMO, thanks!

::: browser/base/content/browser.js:4365
(Diff revision 4)
> +  onSecurityChange: function (aWebProgress, aRequest, aState, aIsSimulated) {
>      // Don't need to do anything if the data we use to update the UI hasn't

To make it more obvious that this code should be updated if the onSecurityChange signature changes, I think we should consider checking that `typeof(aIsSimulated) === "boolean" || typeof(aIsSimulated) === "undefined"`. Of course that won't help if the API adds a boolean argument but it's better than nothing. I think we should throw if the argument isn't boolean or undefined.
Attachment #8634365 - Flags: review?(MattN+bmo) → review+
Thanks for the review - I added in the assertion for aIsSimulated and am also passing in true for that param when it's called in XULBrowserWindow.init. Try push looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=53cc47bfc3a9
remote:   https://hg.mozilla.org/integration/fx-team/rev/dba3e05341bd
remote:   https://hg.mozilla.org/integration/fx-team/rev/31be3956f347
Whiteboard: [fxprivacy] [campaign] → [fixed-in-fx-team][fxprivacy][campaign]
(In reply to Carsten Book [:Tomcat] from comment #22)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/dba3e05341bd
> https://hg.mozilla.org/integration/mozilla-inbound/rev/31be3956f347

I think this was meant to link to the revs in m-c (instead of inbound):

https://hg.mozilla.org/mozilla-central/rev/dba3e05341bd
https://hg.mozilla.org/mozilla-central/rev/31be3956f347
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team][fxprivacy][campaign] → [fxprivacy][campaign]
Verified Nightly Fx42.0a1, 2015-07-27. 

I checked the normal case, as well as switching tabs to verify no animation happens on return to original tab.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.