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

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: 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.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Animation video

Comment 2

[Brian Grinstead [:bgrins]]

I'll take a look at this

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: tp-shield-animate.patch

Stashing this here, waiting for Bug 1175689 to land

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: 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 5

[Brian Grinstead [:bgrins]]

Attachment: tp-shield.mov

screencast with patch applied

Comment 6

[Brian Grinstead [:bgrins]]

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 7

[Matthew N. [: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.

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: 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.

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: tp-no-animate-on-tab-switch.mov

screencast without the animation when switching between tabs

Comment 10

[Ash Grigas]

(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.

Comment 11

[Brian Grinstead [:bgrins]]

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

Comment 12

[Ash Grigas]

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...

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: tp-animate.mov

screencast of current animation

Comment 14

[Matthew N. [: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/#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.

Comment 15

[Brian Grinstead [:bgrins]]

(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.

Comment 16

[Brian Grinstead [:bgrins]]

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

Comment 17

[Brian Grinstead [:bgrins]]

Attachment: MozReview Request: Bug 1175858 - Style updates for browser tracking protection tests;r=MattN

Bug 1175858 - Style updates for browser tracking protection tests;r=MattN

Comment 18

[Matthew N. [:MattN]]

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.

Comment 19

[Matthew N. [: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/#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.

Comment 20

[Brian Grinstead [:bgrins]]

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

Comment 21

[Brian Grinstead [:bgrins]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/dba3e05341bd
remote:   https://hg.mozilla.org/integration/fx-team/rev/31be3956f347

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dba3e05341bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/31be3956f347

Comment 23

[Brian Grinstead [:bgrins]]

(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

Comment 24

[Matthew N. [:MattN]]

https://hg.mozilla.org/mozilla-central/rev/dba3e05341bd
https://hg.mozilla.org/mozilla-central/rev/31be3956f347

Comment 25

[Matt Wobensmith [:mwobensmith][:matt:]]

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.