Closed Bug 1495207 Opened 1 year ago Closed 1 year ago

Do not animate Content Blocking badge when already displayed for the same page

Categories

(Firefox :: Protections UI, defect, P1)

Unspecified
All
defect

Tracking

()

VERIFIED FIXED
Firefox 66
Tracking Status
geckoview62 --- unaffected
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 --- unaffected
firefox64 --- wontfix
firefox65 --- verified
firefox66 --- verified

People

(Reporter: wip.the.gruik, Assigned: johannh)

Details

(Keywords: nightly-community, regression, ux-interruption, Whiteboard: [privacy65])

Attachments

(1 file)

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?
Flags: needinfo?(wip.the.gruik)
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...
Flags: needinfo?(wip.the.gruik)
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: [p
Whiteboard: [p → [privacy-panel-64][triage]
Priority: -- → P3
Whiteboard: [privacy-panel-64][triage] → [privacy-panel]
Duplicate of this bug: 1497014
Ehsan, it seems like this may have come from bug 1493563, can you take a look?
Blocks: 1493563
Flags: needinfo?(ehsan)
OS: Windows 7 → All
Hardware: x86_64 → Unspecified
See Also: → 1497555
(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!)
No longer blocks: privacy-ui, 1493563
Status: NEW → RESOLVED
Closed: 1 year ago
Flags: needinfo?(ehsan)
Keywords: regression
Resolution: --- → WONTFIX
See Also: 1497555
(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.
Flags: needinfo?(ehsan)
I don't make the UX decision here, nothing I can do about this, sorry.
Flags: needinfo?(ehsan)
(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
Target Milestone: Firefox 65 → Firefox 66
Assignee: nobody → jhofmann
Status: REOPENED → ASSIGNED
Priority: P2 → P1
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 jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b9a24fd4b40b
Properly set and reset the animate attribute on the content blocking shield. r=ewright
Status: ASSIGNED → RESOLVED
Closed: 1 year ago1 year ago
Resolution: --- → FIXED

Please nominate this for Beta approval when you get a chance.

Flags: qe-verify+
Flags: needinfo?(jhofmann)

Comment on attachment 9034193 [details]
Bug 1495207 - Properly set and reset the animate attribute on the content blocking shield. r=ewright

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Not sure

User impact if declined: In some edge cases, users may see the content blocking shield animation several times on the same page

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: See comment 0

List of other uplifts needed: None

Risk to taking this patch: Medium

Why is the change risky/not risky? (and alternatives if risky): We're changing some of the core logic of how we're showing the content blocking shield animation, so that might become buggy in case this patch is flawed, on the other hand no other functionality should be impacted.

String changes made/needed: None

Flags: needinfo?(jhofmann)
Attachment #9034193 - Flags: approval-mozilla-beta?

Verified on Nightly 66(20190109092644), that the issue is not reproducible with STR from Comment 0.

Flags: qe-verify+
Flags: qe-verify+

Comment on attachment 9034193 [details]
Bug 1495207 - Properly set and reset the animate attribute on the content blocking shield. r=ewright

[Triage Comment]
Fixes a possibly-annoying situation where the content blocking shield animation appears several times on the same page. Approved for 65.0b10.

Attachment #9034193 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Verified, that the issue is not reproducible on Release 65(20190124174741) and Beta 66(20190128143734)

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.