1536752 - identity.fxaccounts.enabled = false breaks search layout

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Kestrel]

Set identity.fxaccounts.enabled = false and go to about:preferences and search for "pa".

Page width expands to ~39,000px width to accomodate visible code in template-paneSync, leaving search box and buttons well outside window.

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=43a795c023259b1388e18c3660e8a2266ee3f0b1&tochange=41e11bb52568ce7b22584f113271e4a0bc058781

Regressed by Bug 1520350.

Comment 1

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
UI regression that will affect enterprises that turn off sync and the like.

Brian, is this something you or someone else on the de-XBL team can pick up?

Comment 2

[:Gijs (he/him)]

Tracked regression -> P1.

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: Bug 1536752 - Remove sync pane from about:preferences when identity.fxaccounts.enabled is false

Comment 4

[Brian Grinstead [:bgrins]]

The problem is that it's matching the textContent of the CDATA markup in findInPage. The simplest fix looks to be removing template-paneSync from the DOM when the pref is false. I wonder what the pre-Bug-1520350 behavior was - I'm guessing we were incorrectly including stuff inside the pane in search results.

Comment 5

[:Gijs (he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #4)

The problem is that it's matching the textContent of the CDATA markup in findInPage. The simplest fix looks to be removing template-paneSync from the DOM when the pref is false. I wonder what the pre-Bug-1520350 behavior was - I'm guessing we were incorrectly including stuff inside the pane in search results.

Could we just make the findInPage code skip elements that contain only a single CDATA child?

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to :Gijs (he/him) from comment #5)

(In reply to Brian Grinstead [:bgrins] from comment #4)

The problem is that it's matching the textContent of the CDATA markup in findInPage. The simplest fix looks to be removing template-paneSync from the DOM when the pref is false. I wonder what the pre-Bug-1520350 behavior was - I'm guessing we were incorrectly including stuff inside the pane in search results.

Could we just make the findInPage code skip elements that contain only a single CDATA child?

I was also looking into this as an option, and when inspecting template-paneSync it appears to only have a single textNode child (the CDATA isn't detectable, at least when poking around in the inspector inside about:preferences).

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/240085d386be
Remove sync pane from about:preferences when identity.fxaccounts.enabled is false r=Gijs

Comment 8

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/240085d386be

Comment 9

[Brian Grinstead [:bgrins]]

Kestrel, can you verify the fix on Nightly? I'll request uplift so we can get this onto 67.

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 9052949 [details]
Bug 1536752 - Remove sync pane from about:preferences when identity.fxaccounts.enabled is false

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1520350
• User impact if declined: Search UI on about:preferences is glitchy (extremely long horizontal scroll) when identity.fxaccounts.enabled is set to false (default value for the pref is true).
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: - Set identity.fxaccounts.enabled = false

• Open about:preferences
• Search for "sync"

Expected: no results
Previous buggy behavior: horizontal scrollbar appears along with some XML markup returned as a result

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It's a change for a non-default pref value, only affects about:preferences (and specifically searching within it). There's also an automated test for it.
• String changes made/needed: None

Comment 11

[Kestrel]

(In reply to Brian Grinstead [:bgrins] from comment #9)

Kestrel, can you verify the fix on Nightly? I'll request uplift so we can get this onto 67.

Yes, it's fixed for me on latest Nightly 68.0a1 (2019-03-25) build ID 20190325095153.

Comment 12

[Pascal Chevrel:pascalc]

Comment on attachment 9052949 [details]
Bug 1536752 - Remove sync pane from about:preferences when identity.fxaccounts.enabled is false

Fix for a P1 regression in 67, with tests and verified on Nightly, approved for 67 beta 6, thanks.

Comment 13

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/releases/mozilla-beta/rev/32cf4d4997be

Comment 14

[Rares Doghi, Desktop QA]

I managed to reproduce this issue in older versions of Firefox ie Firefox Beta 67.0b5 but it no longer occurs in Fx67.0b6 on Windows 10, Mac OsX and Ubuntu systems. I will mark this issue accordingly.