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

VERIFIED FIXED in Firefox 63

Status

()

defect
P3
normal
VERIFIED FIXED
8 months ago
7 months ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

(Blocks 1 bug)

unspecified
Firefox 64
Points:
---

Firefox Tracking Flags

(firefox63 verified, firefox64 verified)

Details

Attachments

(2 attachments)

(Assignee)

Description

8 months ago
Posted image 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?
Flags: needinfo?(jhofmann)
(Assignee)

Comment 1

8 months ago
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.)
See Also: → 1488809
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...
Flags: needinfo?(jhofmann)
Priority: -- → P3
See Also: 1488809
(Assignee)

Comment 3

7 months ago
(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)...
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!
Flags: needinfo?(bbell)

Comment 5

7 months ago
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)
Flags: needinfo?(bbell)
Ok, thanks!
Priority: P3 → P5
(Assignee)

Updated

7 months ago
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Priority: P5 → P3
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.
Attachment #9009342 - Flags: review+

Comment 9

7 months ago
Pushed by eakhgari@mozilla.com:
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

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4d7e9850ad92
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
(Assignee)

Comment 11

7 months ago
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
Attachment #9009342 - Flags: approval-mozilla-beta?
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.
Attachment #9009342 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.