Closed Bug 1495207 Opened 1 year ago Closed 1 year ago
Do not animate Content Blocking badge when already displayed for the same page
Bug 1495207 - Properly set and reset the animate attribute on the content blocking shield. r=ewright
47 bytes, text/x-phabricator-request
|Details | Review|
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:64.0) Gecko/20100101 Firefox/64.0 Build ID: 20180929100556 Steps to reproduce: - enable Content Blocking - open any BMO bug having at least one image as attachment e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1489581 - observe the Content Blocking badge animating and wait for it to finish (the shield at the left of the address bar, within the identity block) - click on any image in the "Attachments" section to trigger its display Actual results: the Content Blocking badge is animating again, despite being already displayed for the exact same page/url Expected results: nothing particular should append I understand that the displaying of an attachment triggers the blocking of a (new) resource not blocked during the page load, however I doubt any non-technical user comprehend this subtlety.
I can't reproduce this on Bugzilla because it doesn't load tracking scripts for me for some reason. Can you maybe reproduce this anywhere else?
I have not yet seen this behavior on Nightly with any other sites I use than Bugzilla. However, I realize that I do not use the default Content Blocking settings. In particular, I block all third-party cookies (as I have done since forever). Toggling the blocking of all third-party cookies is what make the animation replaying. This is confirmed by the following message on the console when I click the link to display the attached image: Request to access cookie or storage on “https://bug1489581.bmoattachments.org/attachment.cgi?id=9007301” was blocked because we are blocking all third-party storage access requests and content blocking is enabled. So, a most accurate STR would be: - using a new/clean profile - enable Content Blocking with the following settings * Slow Loading Trackers: ON * All Detected Trackers: ALWAYS * Third-Party Cookies: ALL - open any BMO bug having at least one image as attachment e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1489581 - observe the Content Blocking badge animating and wait for it to finish - click on any image in the "Attachments" section to trigger its display Note: you have to refresh the page each time you want to reproduce the bug. Hope this will help...
Ok, thanks for the details! This bug is really weird. First of all I can not reproduce it with my current profile, just with a fresh one. The cause for this is also quite clear but it doesn't make it any less mysterious. When onSecurityChange is called on clicking the image shortcut, the webProgress signals that there is a top-level navigation (https://searchfox.org/mozilla-central/rev/3c85ea2f8700ab17e38b82d77cd44644b4dae703/browser/base/content/browser-contentblocking.js#482). That causes our front-end to animate the shield again, because it thinks that a top-level navigation happened. This might need a deeper investigation.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [privacy-panel-64][triage] → [privacy-panel]
Ehsan, it seems like this may have come from bug 1493563, can you take a look?
(In reply to Johann Hofmann [:johannh] from comment #5) > Ehsan, it seems like this may have come from bug 1493563, can you take a > look? Another case where bug 1493563 was being incorrectly blamed. This bug was filed when the default cookie restrictions were enabled on Nightly (see bug 1492563 comment 18 where it got backed out). If you go to Content Blocking in Preferences and set Third-Party Cookies to Trackers, and open Web Console, you will see the following message logged to the console: Request to access cookie or storage on “https://bug1489581.bmoattachments.org/attachment.cgi?id=9007301” was blocked because we are blocking all third-party storage access requests and content blocking is enabled. The mozregression job that I was running helpfully found https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=54c5a8af9831a0a36ed39c6f4581101acdcae79b&tochange=498b35c45ec1e50d5f98bcc2ac49c90357c920a2 which points to bug 1487093 which started sending these notifications to the control centre. IOW, there is no regression! This is indeed working as intended: https://searchfox.org/mozilla-central/rev/c9272ef398954288525e37196eada1e5a93d93bf/browser/base/content/browser-contentblocking.js#516 is being taken because something new has been blocked. Our shield icon will animate every time something new gets blocked on the page, so this is working as designed, per the UX spec. (I agree that it's annoying personally, sorry about that!)
(In reply to :Ehsan Akhgari from comment #6) > Our shield icon will animate every time something new gets blocked on the page, so this is working as designed, per the UX spec. (I agree that it's annoying personally, sorry about that!) Then we have to turn off this feature. :/ This is unacceptable as it is distracting when watching videos. Broken per UX spec is not nice!
TP should stop and silence any tracking and not reminding you at *every video chunk* how bad the internet is.
[Tracking Requested - why for this release]: UX regression. Too much bling-bling. Distracting: comment 0 + bug 1497014.
(In reply to :Ehsan Akhgari from comment #6) > was being incorrectly blamed. > so this is working as designed, per the UX spec. (I agree that it's annoying personally, sorry about that!) The regression range doesn't necessarily mean that you would have made a programming error, but that a problem got visible. Did you wontfix only the perceived accusation of a technical problem because -in fact- you've implemented it correctly, or did you wontfix the whole TP UX regression? Would it be possible to reopen this bug and moving it over to the "Site Identity and Permission Panels" component? You, Franck and me already find it annoying and this would anyway cause further duplicate reports. ^^ I would suggest that a counter of blocked elements could be shown in the Site Information Panel (Control Center). Either replacing every "Blocked" with "14 Blocked" or just showing one common counter. We already have such a decent counter in Firefox Focus.
I don't make the UX decision here, nothing I can do about this, sorry.
(In reply to :Ehsan Akhgari from comment #11) > I don't make the UX decision here Thank you for clarifying. I de-duped bug 1497014 and added it to [ux-decision-needed].
FWIW I talked about this with our UX designers a bit today and they agreed that animating the icon after page load is too intensive. This behavior won't change for Firefox 64 but for 65 we're planning some major changes to the UX of the entire UI here and as part of that we'll try to look at improving this as well.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Thank you Ehsan for forwarding our feedback to the UX team.
We'll probably want this as part of 65 MVP
Whiteboard: [privacy-panel] → [privacy65][triage]
Priority: P3 → P2
Whiteboard: [privacy65][triage] → [privacy65]
Target Milestone: --- → Firefox 65
Previously this code was using webProgress.isTopLevel to set and reset the shield animation, which is just plain nonsense and was based on false assumptions about it being something like webProgress.isLoadingDocument. In reality this attribute just reflects whether the source of the event is the top-level window or a frame, not the load type. The new code uses the "blocking" state to set and reset the animation and uses the "active" attribute as a guard to ensure that we only set the "animate" attribute once per page. This works because the "active" attribute is guaranteed to be reset on a top-level document load.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/b9a24fd4b40b Properly set and reset the animate attribute on the content blocking shield. r=ewright
Attachment #9034193 - Flags: approval-mozilla-beta?
You need to log in before you can comment on or make changes to this bug.