Closed Bug 1493185 Opened Last year Closed Last year

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

Categories

(Firefox :: Preferences, defect, P1)

63 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 64
Tracking Status
firefox62 --- unaffected
firefox63 + verified
firefox64 + verified

People

(Reporter: cfogel, Assigned: johannh)

References

Details

(Whiteboard: [privacy-panel-64][uplift])

Attachments

(3 files, 3 obsolete files)

[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;
QA Contact: comorasu.cristian → cristian.comorasu
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?
(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?
Flags: needinfo?(cristian.fogel)
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.
Flags: needinfo?(cristian.fogel)
Looks like this was implemented in bug 1462470, so at least radaring there. Not clear yet why it's not working right...
Blocks: 1462470
Priority: -- → P1
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.
Ehsan/Johann, can one of you take it from here? (see comment #6)
Flags: needinfo?(jhofmann)
Flags: needinfo?(ehsan)
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?
Flags: needinfo?(ehsan)
Whiteboard: [privacy-panel-64][uplift]
Target Milestone: --- → Firefox 63
(presuming you own this now ;)
Assignee: nobody → ehsan
(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.
Assignee: ehsan → nobody
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
Attached patch Trial fix for the test (obsolete) — Splinter Review
This small patch is what I had applied locally in my quest to fix this test failure.  It still isn't sufficient yet.
Blocks: 1488013
No longer blocks: 1462470
Flags: needinfo?(jhofmann)
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
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.
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.
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
Attachment #9013431 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/24b0c3333b72
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
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
Johann, can you request uplift on this bug?
Target Milestone: Firefox 63 → Firefox 64
Fix Verified with 64.0a1 (2018-10-07) on the reported OS(s).
Status: RESOLVED → VERIFIED
Flags: needinfo?(jhofmann)
Absolutely!
Flags: needinfo?(jhofmann)
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
Attachment #9014574 - Flags: approval-mozilla-beta?
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.
Attachment #9014574 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Attachment #9012679 - Attachment is obsolete: true
Attachment #9012635 - Attachment is obsolete: true
Attached patch Patch for betaSplinter Review
This is the patch for uplift to beta.

Thanks!
Flags: needinfo?(apavel)
Thanks Johann!
Flags: needinfo?(apavel)
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.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.