1489007 - Site Identity panel shows a red Disabled warning with the wrong tooltip in the Tracking Protection section on normal windows when there is a tracker detected

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P3)

Description

[(no longer active)]

Attachment: Screenshot

STR:

1. On Nightly, go to a site that has a tracker on it.
2. In order to simulate Beta, go to about:config and toggle browser.contentblocking.ui.enabled to false.

The content blocking panel looks like the screenshot.

The tooltip says: "You have disabled Content Blocking".

Johann, is this the intentional behavior of the UI?

Comment 1

[(no longer active)]

BTW we totally have tests for this and they fail!  I'm not sure if I'm supposed to update the expectations in the test or not...

(At the very least the tooltip is certainly wrong, but to me it appears the test is correct here, and we actually have a real UI bug.)

Comment 2

[Johann Hofmann [:johannh]]

Yeah, when we implemented the content blocking UI initially we said that we would cut some corners to avoid having to make maintaining the browser.contentblocking.ui pref incredibly complex. The idea wass that we're very confident that the CB UI is going to ship anyway :)

That's why I decided, for some of the less visible strings, to skip having two versions (you can see the same in the "report breakage" UI).

In this case that might not have been the right decision, I'm not sure.

We can definitely fix this, but I don't think it's worth uplifting these strings to 63 (regular users probably won't pay as much attention to the terminology as we do) and so at this point we might as well wait for the result of the shield study and see if we want to keep or drop the whole UI.

I don't know what this has to do with the failing test, though. We should probably chat about that on IRC...

Comment 3

[(no longer active)]

(In reply to Johann Hofmann [:johannh] from comment #2)
> Yeah, when we implemented the content blocking UI initially we said that we
> would cut some corners to avoid having to make maintaining the
> browser.contentblocking.ui pref incredibly complex. The idea wass that we're
> very confident that the CB UI is going to ship anyway :)

Huh.  So, on the off-chance that it doesn't, we're OK with shipping a giant red "Disabled" red warning in the control centre for all normal browsing windows for 63?  To me at least that seems like a regression, in that the user has _not_ disabled anything, and the warning seems too severe, as if something is seriously wrong.

> That's why I decided, for some of the less visible strings, to skip having
> two versions (you can see the same in the "report breakage" UI).
> 
> In this case that might not have been the right decision, I'm not sure.

Sure, but the tooltip is like 10% of what's wrong in this bug, let's focus on the 90% first.  :-)

> We can definitely fix this, but I don't think it's worth uplifting these
> strings to 63 (regular users probably won't pay as much attention to the
> terminology as we do) and so at this point we might as well wait for the
> result of the shield study and see if we want to keep or drop the whole UI.

I posit that regular users do pay much attention go giant red warnings that have never appeared anywhere else in our primary UI...

> I don't know what this has to do with the failing test, though. We should
> probably chat about that on IRC...

It wasn't immediately obvious to me what the root cause of the test failures were, since it's *not* just about the unblock button (e.g. note the failure on this check: <https://searchfox.org/mozilla-central/rev/a41fd8cb947266ea2e3f463fc6e31c88bfab9d41/browser/base/content/test/trackingUI/browser_trackingUI_state.js#182>) but now that I look at your patch I see where the logic mismatches the underlying code.

Except that I still think the underlying code is wrong (due to this bug)...

Comment 4

[Johann Hofmann [:johannh]]

Ehsan and I chatted on IRC and I explained a bit more about how the tracking protection UI we currently show on beta is really just a fallback for the content blocking UI and that bbell and pdol agreed to this plan.

I can, however, understand the concern about the big red warning icon. Bryan, can you give Beta a quick try and tell us your opinion about the current state? Is this something we could ship in the worst case or should we remove the red label in favor of a more subtle hint (e.g. crossing out the icon again)?

Thanks!

Comment 5

[bbell]

I don't think the strings, in this case, pose much of a worry, especially considering that the New-New-UI will make it to the public before this does.  I would say if you want to do anything about this red chicklet, you might only change the background to grey, so it looks informational and not like a significant warning. 

I would use Photon-Grey-40 #B1B1B3 (but only if you really want to)

Comment 6

[Johann Hofmann [:johannh]]

Ok, thanks!

Comment 7

[(no longer active)]

Attachment: Bug 1489007 - Turn the colour of the 'Disabled' warning when Content Blocking UI is turned off and tracking protection is off to grey; r=johannh

Comment 8

[Johann Hofmann [:johannh]]

Comment on attachment 9009342 [details]
Bug 1489007 - Turn the colour of the 'Disabled' warning when Content Blocking UI is turned off and tracking protection is off to grey; r=johannh

Johann Hofmann [:johannh] has approved the revision.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d7e9850ad92
Turn the colour of the 'Disabled' warning when Content Blocking UI is turned off and tracking protection is off to grey; r=johannh

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/4d7e9850ad92

Comment 11

[(no longer active)]

Comment on attachment 9009342 [details]
Bug 1489007 - Turn the colour of the 'Disabled' warning when Content Blocking UI is turned off and tracking protection is off to grey; r=johannh

Approval Request Comment
[Feature/Bug causing the regression]: Not sure, part of new UI
[User impact if declined]: Comment 0
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Locally
[Needs manual test from QE? If yes, steps to reproduce]: Sure, comment 0
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: changing the colour of some UI element
[String changes made/needed]: None

Comment 12

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9009342 [details]
Bug 1489007 - Turn the colour of the 'Disabled' warning when Content Blocking UI is turned off and tracking protection is off to grey; r=johannh

Short CSS fix for a visual bug on the content blockcking feature we have a shield study for in 63, on nightly for several days without reported regressions, Uplift approved for 63 beta 9, thanks.

Comment 13

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/79c5606108ec

Comment 14

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I have reproduced this issue using Firefox 64.0a1 (2018.09.05) on Ubuntu 18.04 x64.
I can confirm this issue is fixed, I verified using Firefox 64.0a1 (latest nightly) and 63.0b9 on Ubuntu 18.04 x64, Windows 10 x64 and Mac OS X 10.13.6.