Cristian Fogel [:cfogel] Reporter
	Description • 6 years ago

[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;

	(no longer active)
	Comment 1 • 6 years ago

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?

	Marco Castelluccio [:marco]
	Comment 2 • 6 years ago

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

	Cristian Fogel [:cfogel] Reporter
	Comment 3 • 6 years ago

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.

	:Gijs (he/him)
	Comment 4 • 6 years ago

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

	:Gijs (he/him)
	Comment 5 • 6 years ago

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.

	:Gijs (he/him)
	Comment 6 • 6 years ago

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.

	:Gijs (he/him)
	Comment 7 • 6 years ago

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

	(no longer active)
	Comment 8 • 6 years ago

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?

	Justin Dolske [:Dolske]
	Comment 10 • 6 years ago

(presuming you own this now ;)

	(no longer active)
	Comment 11 • 6 years ago

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

	(no longer active)
	Comment 12 • 6 years ago

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

	(no longer active)
	Comment 13 • 6 years ago

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

	Johann Hofmann [:johannh] Assignee
	Comment 14 • 6 years ago

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.

	Johann Hofmann [:johannh] Assignee
	Comment 15 • 6 years ago

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.

	Pulsebot
	Comment 17 • 6 years ago

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

	Dorel Luca [:dluca]
	Comment 18 • 6 years ago
bugherder

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

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 19 • 6 years ago

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

	Tanvi Vyas[:tanvi]
	Comment 20 • 6 years ago

Johann, can you request uplift on this bug?

	Cristian Fogel [:cfogel] Reporter
	Comment 21 • 6 years ago

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

	Johann Hofmann [:johannh] Assignee
	Comment 22 • 6 years ago

Absolutely!

	Johann Hofmann [:johannh] Assignee
	Comment 23 • 6 years ago

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

	Pascal Chevrel:pascalc (PTO until April 26)
	Comment 24 • 6 years ago

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.

	Johann Hofmann [:johannh] Assignee
	Comment 25 • 6 years ago

This is the patch for uplift to beta.

Thanks!

	Andreea Pavel [:apavel]
	Comment 26 • 6 years ago
bugherder uplift

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

	Andreea Pavel [:apavel]
	Comment 27 • 6 years ago

Thanks Johann!

	Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact mboldan@mozilla.com
	Comment 28 • 6 years ago

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.