1487300 - Unchecking "All Detected Trackers" always makes "Only in private windows" selected

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P2)

Description

[(no longer active)]

STR:

1. Open about:preferences#privacy-trackingprotection.
2. Ensure All Detected Trackers is checked and under it, Always is checked.
3. Uncheck the checkbox.

Actual results: "Only in private windows" gets selected.

Expected results: the radio button selection shouldn't change.

Comment 1

[(no longer active)]

Tanvi, how much do we care about this?  It's kinda edge casey, seems like I missed it while testing my patch...

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Setting to P3 while Tanvi and Ehsan evaluate the importance of this.

Comment 3

[Tanvi Vyas[:tanvi]]

Lets make it a P2, do it in 64 timeframe with potential to uplift.

Next week we are going to meet to figure out how to track these; in the meantime I'm creating a whiteboard flag "[privacy-panel-64]" for this.

Comment 4

[(no longer active)]

The problem here is that when we uncheck the checkbox, we set both the TP pref and the TP PBM pref to false, and we don't persist the state of the UI anywhere after this point.  From here on, when we restore the state of the radio group, we set its value to "never", but that value doesn't appear in any of the radio children so we fall back to the first radio child (Only in private windows).  And when the user clicks the checkbox again to turn All Detected Trackers back on, we now have the radiogroup still in the same state, hence the bug.

In order to fix this, we need a "backup preference" to write the state of the prefs before entering the "never" state, which we can use to restore the state of the radiogroup properly when needed.

Comment 5

[(no longer active)]

Attachment: Bug 1487300 - Restore the state of the tracking protection menu when All Detected Trackers is checked after being previously unchecked; r=johannh

Comment 6

[Martin Thomson [:mt:]]

I'm seeing an issue that might be related: the checkbox isn't visibly toggling.  The radio buttons are activated or not, as is the pref, but the checkbox doesn't show a check mark.

Comment 7

[(no longer active)]

(In reply to Martin Thomson [:mt:] from comment #6)
> I'm seeing an issue that might be related: the checkbox isn't visibly
> toggling.  The radio buttons are activated or not, as is the pref, but the
> checkbox doesn't show a check mark.

What build is that?  That sounds bug 1488013 which is fixed in the latest Nightly.  (This UI is under heavy development, so you really want to be on the *latest* Nightly for testing.)

Comment 8

[Martin Thomson [:mt:]]

Ahh, that is it.  Thanks.  I always check before doing the long search and filing process, but it must have taken too long for the build to make it through.

FWIW, you now have two "Change block list" links, and the second is non-functional.  (64.0a1 (2018-09-04) (64-bit))

Comment 9

[Johann Hofmann [:johannh]]

(In reply to Martin Thomson [:mt:] from comment #8)
> Ahh, that is it.  Thanks.  I always check before doing the long search and
> filing process, but it must have taken too long for the build to make it
> through.
> 
> FWIW, you now have two "Change block list" links, and the second is
> non-functional.  (64.0a1 (2018-09-04) (64-bit))

Bug 1488436 :)

Comment 10

[Johann Hofmann [:johannh]]

Comment on attachment 9006409 [details]
Bug 1487300 - Restore the state of the tracking protection menu when All Detected Trackers is checked after being previously unchecked; r=johannh

Johann Hofmann [:johannh] has approved the revision.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd88ac1e5391
Restore the state of the tracking protection menu when All Detected Trackers is checked after being previously unchecked; r=johannh

Comment 12

[(no longer active)]

Comment on attachment 9006409 [details]
Bug 1487300 - Restore the state of the tracking protection menu when All Detected Trackers is checked after being previously unchecked; r=johannh

Approval Request Comment
[Feature/Bug causing the regression]: This isn't a regression, but an improvement to a new UI in 63.
[User impact if declined]: comment 0.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Landed now.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: The code is rather trivial and disabled by default.
[String changes made/needed]: None.

Comment 13

[Arthur Iakab [arthur_iakab]]

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

Comment 14

[Pascal Chevrel:pascalc]

Comment on attachment 9006409 [details]
Bug 1487300 - Restore the state of the tracking protection menu when All Detected Trackers is checked after being previously unchecked; r=johannh

Uplift approved for 63 beta 5

Comment 15

[Eliza Balazs [:ebalazs_]]

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

Comment 16

[Daniel Bodea [:danibodea]]

I have verified this issue in the latest Nightly v64.0a1 (2018-09-10). 
Also, this issue does not appear as valid for the Beta version of the browser (v63.0b3 / v63.0b4) because the design of the changeable options from the tracking protection section does not allow for the issue to reproduce. 
Please see the attachment below. Am I missing anything?

Comment 17

[Daniel Bodea [:danibodea]]

Attachment: Issue is not present on the BETA version.png

Demonstration for comment 16.

Comment 18

[(no longer active)]

(In reply to Bodea Daniel [:danibodea] from comment #16)
> I have verified this issue in the latest Nightly v64.0a1 (2018-09-10). 
> Also, this issue does not appear as valid for the Beta version of the
> browser (v63.0b3 / v63.0b4) because the design of the changeable options
> from the tracking protection section does not allow for the issue to
> reproduce. 
> Please see the attachment below. Am I missing anything?

You need to set the browser.contentblocking.ui.enabled pref to true in order to see the new version of the UI.

Comment 19

[Daniel Bodea [:danibodea]]

This issue is now verified in v63.0b5. 
Uplift successful.