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)

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
Whiteboard: [fxprivacy] [campaign] → [fixed-in-fx-team][fxprivacy][campaign]
Status: ASSIGNED → RESOLVED
Closed: 10 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.

Attachment

General

Created:
Updated:
Size: