1493185 - Content blocking brought in focus on the position of Tracking protection after flipping browser.contentblocking.ui.enabled on

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Cristian Fogel [:cfogel]]

Attachment: trackProtection_browser.contentblocking.ui.enabled_preff_scrollsToOldPosition.gif

[Affected versions]:
- 63.0b7

[Affected platforms]:
- win10x64, macOS 10.13.6, Ubuntu 16.04

[Steps to reproduce]:
1. Launch firefox;
2. Access the about:config page;
3. Turn the following preff to true: browser.contentblocking.ui.enabled
4. Click on the 3line Menu button;
5. Click on the Content blocking option;

[Expected result]:
- about:preferences#privacy page is open with Content blocking section in focus

[Actual result]:
- focus is brought in the (old)position of the tracking protection 

[Regression range]:
- not a regression

[Additional notes]:
- attached screenshot with the issue;
- marked 64 as unaffected since the issue does not reproduce even after a restart/refresh for it;

Comment 1

[(no longer active)]

I can reproduce on beta.  Thanks for the report.

Cristian, is this a regression on beta?  Any chance you can bisect to see what broke it please?

Comment 2

[Marco Castelluccio [:marco]]

(In reply to :Ehsan Akhgari from comment #1)
> I can reproduce on beta.  Thanks for the report.
> 
> Cristian, is this a regression on beta?  Any chance you can bisect to see
> what broke it please?

I see Cristian said this is not a regression. Cristian, can you confirm?

Comment 3

[Cristian Fogel [:cfogel]]

Unfortunately mozregression doesn't want to run on beta channels.
Checked manually and 63.0b3 is the first beta version affected; 62.0b20 is fine.

Another thing is that, nightly builds don't seem to have this issue; have tried a few versions and to bisect it ... but this behavior did not reproduce.

Comment 4

[:Gijs (he/him)]

Looks like this was implemented in bug 1462470, so at least radaring there. Not clear yet why it's not working right...

Comment 5

[:Gijs (he/him)]

Beta is at least hitting https://searchfox.org/mozilla-central/rev/0640ea80fbc8d48f8b197cd363e2535c95a15eb3/toolkit/components/telemetry/core/TelemetryHistogram.cpp#1373-1378 for something when clicking that item... not sure if it's related.

Comment 6

[:Gijs (he/him)]

This doesn't reproduce if the preferences are already open. Without actually testing with a self-built instrumented version of beta, I suspect this is a race condition between the spotlight code scrolling the element into view, and the code in https://dxr.mozilla.org/mozilla-beta/source/browser/components/preferences/in-content/privacy.js#541-559 that reorders the content blocking UI.

I guess on nightly we're either winning this race or some APZ / scroll stuff that's enabled there but not beta is making the scrollIntoView code deal with the scrolled-into-view content moving from under it. Though on Nightly, I also don't see the 'spotlight' highlighting being applied, though that does get applied on beta.

Comment 7

[:Gijs (he/him)]

Ehsan/Johann, can one of you take it from here? (see comment #6)

Comment 8

[(no longer active)]

This is obviously a race between:

https://searchfox.org/mozilla-central/rev/0640ea80fbc8d48f8b197cd363e2535c95a15eb3/browser/components/preferences/in-content/preferences.js#84

and

https://searchfox.org/mozilla-central/rev/0640ea80fbc8d48f8b197cd363e2535c95a15eb3/browser/components/preferences/in-content/privacy.js#549

no doubt about that part.

What I'm not so sure about is how to fix it, given how far apart these two bits of code are.

Note that gotoPref() is responsible for lazily initializing the panes: https://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/preferences.js#160.  The cleanest way I can think of fixing this is to make init() return a promise which gotoPref() would wait on and resolve it after calling initSiteDataControls().

Thoughts?

Comment 9

[(no longer active)]

Attachment: Bug 1493185 - Make gotoPref correctly wait for the initialization of the privacy pane to finish

Comment 10

[Justin Dolske [:Dolske]]

(presuming you own this now ;)

Comment 11

[(no longer active)]

(In reply to Justin Dolske [:Dolske] from comment #10)
> (presuming you own this now ;)

It is unclear actually, this patch is just one example approach.

Comment 12

[(no longer active)]

This is a try push with my patch: https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=201866286&revision=2bcaf77d22d65d5f6527fdde379956df82f79227

the failing test I mentioned is: devtools/client/responsive.html/test/browser/browser_container_tab.js

Comment 13

[(no longer active)]

Attachment: Trial fix for the test

This small patch is what I had applied locally in my quest to fix this test failure.  It still isn't sufficient yet.

Comment 14

[Johann Hofmann [:johannh]]

Attachment: Bug 1493185 - Initialize Content Blocking prefs UI before moving it to the top. r=Gijs

We previously moved this section to the top in a timeout, to allow the other code that depends
on the XUL bindings of these elements to intialize first, but that ended up racing with the
spotlight feature. It's easier to just initialize the dependent code earlier.

This change also required cleaning up some usages of _updateTrackingProtectionUI.

Comment 15

[Johann Hofmann [:johannh]]

So this patch takes a different approach to fixing this. Instead of having even more stuff wait for the move of the Content Blocking section to the top, we make the moving "sync" again and initialize the everything else beforehand. To be honest I'm not very confident in this patch, that is I'm confident that it fixes the issue, but I'm a little nervous about other things breaking here.

On the other hand we have pretty good test coverage and I couldn't find any issues while testing it locally.

Comment 16

[Johann Hofmann [:johannh]]

Attachment: Bug 1493185 - Always put Content Blocking, TP and Site Data to the top of about:preferences. r=Gijs

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/24b0c3333b72
Always put Content Blocking, TP and Site Data to the top of about:preferences. r=Gijs

Comment 18

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/24b0c3333b72

Comment 19

[Joel Maher ( :jmaher ) (UTC -8)]

woohoo a perf win:
== Change summary for alert #16493 (as of Fri, 05 Oct 2018 12:39:36 GMT) ==

Improvements:

  2%  about_preferences_basic windows7-32 opt e10s stylo     142.96 -> 139.91

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=16493

Comment 20

[Tanvi Vyas[:tanvi]]

Johann, can you request uplift on this bug?

Comment 21

[Cristian Fogel [:cfogel]]

Fix Verified with 64.0a1 (2018-10-07) on the reported OS(s).

Comment 22

[Johann Hofmann [:johannh]]

Absolutely!

Comment 23

[Johann Hofmann [:johannh]]

Comment on attachment 9014574 [details]
Bug 1493185 - Always put Content Blocking, TP and Site Data to the top of about:preferences. r=Gijs

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1488013

User impact if declined: Users who click on "Content Blocking" in the main menu are lead to the wrong spot in about:preferences. It looks really amateurish.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

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: Low

Why is the change risky/not risky? (and alternatives if risky): Short version: Simply moving a bunch of static markup elements in a .xul file and removing a workaround that caused a bunch of issues and weird race conditions.

Long version:
So this is low-to-medium, somewhere in between. Touching this whole area of code has lead to a cascade of follow-up regressions in the past, so I had been afraid of landing a fix to this bug late in beta, BUT! what we came up with here is really the best outcome. We're able to eliminate a hack that dynamically moves elements when about:preferences is created and instead move them in the .xul file directly. This is possible because UX agreed that in the "old" scenario, the section we're moving can also be at the top.

String changes made/needed: None

Comment 24

[Pascal Chevrel:pascalc]

Comment on attachment 9014574 [details]
Bug 1493185 - Always put Content Blocking, TP and Site Data to the top of about:preferences. r=Gijs

P1, verified on nightly, uplift approved for 63 beta 13, thanks.

Comment 25

[Johann Hofmann [:johannh]]

Attachment: Patch for beta

This is the patch for uplift to beta.

Thanks!

Comment 26

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5517b5cb5a92

Comment 27

[Andreea Pavel [:apavel]]

Thanks Johann!

Comment 28

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this issue using Fx 63.0b3, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 63.0b12, on macOS 10.13.6, Ubuntu 16.04 LTS and Windows 10 x64.