Closed
Bug 1175858
Opened 10 years ago
Closed 10 years ago
TP shield should be animated when TP has blocked content on the page
Categories
(Firefox :: General, defect, P1)
Firefox
General
Tracking
()
VERIFIED
FIXED
Iteration:
42.2 - Jul 27
People
(Reporter: past, Assigned: bgrins)
References
Details
(Whiteboard: [fxprivacy][campaign])
Attachments
(7 files, 2 obsolete files)
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?
Updated•10 years ago
|
Flags: firefox-backlog? → firefox-backlog+
Reporter | ||
Comment 1•10 years ago
|
||
Updated•10 years ago
|
Rank: 1
Priority: -- → P1
Updated•10 years ago
|
Points: --- → 5
Flags: qe-verify+
Assignee | ||
Comment 2•10 years ago
|
||
I'll take a look at this
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Updated•10 years ago
|
Iteration: --- → 42.1 - Jul 13
Updated•10 years ago
|
QA Contact: mwobensmith
Assignee | ||
Comment 3•10 years ago
|
||
Stashing this here, waiting for Bug 1175689 to land
Updated•10 years ago
|
Iteration: 42.1 - Jul 13 → 42.2 - Jul 27
Assignee | ||
Comment 4•10 years ago
|
||
Bug 1175858 - TP shield should be animated when TP has blocked content on the page;r=MattN
Attachment #8634365 -
Flags: review?(MattN+bmo)
Assignee | ||
Comment 5•10 years ago
|
||
screencast with patch applied
Assignee | ||
Comment 6•10 years ago
|
||
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•10 years ago
|
||
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)
Assignee | ||
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
screencast without the animation when switching between tabs
Assignee | ||
Updated•10 years ago
|
Attachment #8633034 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8634392 -
Attachment is obsolete: true
Comment 10•10 years ago
|
||
(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)
Assignee | ||
Comment 11•10 years ago
|
||
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)
Comment 12•10 years ago
|
||
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...
Assignee | ||
Comment 13•10 years ago
|
||
screencast of current animation
Updated•10 years ago
|
Whiteboard: [fxprivacy] → [fxprivacy] [campaign]
Comment 14•10 years ago
|
||
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)
Assignee | ||
Comment 15•10 years ago
|
||
(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.
Assignee | ||
Updated•10 years ago
|
Attachment #8634365 -
Flags: review?(MattN+bmo)
Assignee | ||
Comment 16•10 years ago
|
||
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
Assignee | ||
Comment 17•10 years ago
|
||
Bug 1175858 - Style updates for browser tracking protection tests;r=MattN
Attachment #8637506 -
Flags: review?(MattN+bmo)
Comment 18•10 years ago
|
||
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 19•10 years ago
|
||
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+
Assignee | ||
Comment 20•10 years ago
|
||
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
Assignee | ||
Comment 21•10 years ago
|
||
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]
Comment 22•10 years ago
|
||
Assignee | ||
Comment 23•10 years ago
|
||
(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: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•10 years ago
|
Whiteboard: [fixed-in-fx-team][fxprivacy][campaign] → [fxprivacy][campaign]
Comment 24•10 years ago
|
||
Comment 25•10 years ago
|
||
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.
Description
•