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

Status: VERIFIED FIXED

(Firefox :: Protections UI, defect, P1)

Description

[Franck (Wip)]

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.

Comment 1

[Johann Hofmann [:johannh]]

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?

Comment 2

[Franck (Wip)]

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

Comment 3

[Johann Hofmann [:johannh]]

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.

Comment 5

[Johann Hofmann [:johannh]]

Ehsan, it seems like this may have come from bug 1493563, can you take a look?

Comment 6

[(no longer active)]

(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!)

Comment 7

[Darkspirit]

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

Comment 8

[Darkspirit]

TP should stop and silence any tracking and not reminding you at *every video chunk* how bad the internet is.

Comment 9

[Darkspirit]

[Tracking Requested - why for this release]: UX regression. Too much bling-bling. Distracting: comment 0 + bug 1497014.

Comment 10

[Darkspirit]

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

Comment 11

[(no longer active)]

I don't make the UX decision here, nothing I can do about this, sorry.

Comment 12

[Darkspirit]

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

Comment 13

[(no longer active)]

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.

Comment 14

[Franck (Wip)]

Thank you Ehsan for forwarding our feedback to the UX team.

Comment 15

[Johann Hofmann [:johannh]]

We'll probably want this as part of 65 MVP

Comment 16

[Johann Hofmann [:johannh]]

Attachment: Bug 1495207 - Properly set and reset the animate attribute on the content blocking shield. r=ewright

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.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b9a24fd4b40b
Properly set and reset the animate attribute on the content blocking shield. r=ewright

Comment 18

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/b9a24fd4b40b

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Johann Hofmann [:johannh]]

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

Comment 21

[Gyula Palko[:gpalko]]

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c777b4b5cd07

Comment 24

[Gyula Palko[:gpalko]]

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