identity.fxaccounts.enabled = false breaks search layout

VERIFIED FIXED in Firefox 67

Status

()

defect
P1
normal
VERIFIED FIXED
4 months ago
4 months ago

People

(Reporter: ke5trel, Assigned: bgrins)

Tracking

({regression})

67 Branch
Firefox 68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox66 unaffected, firefox67+ verified, firefox68 verified)

Details

Attachments

(1 attachment)

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.

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

Flags: needinfo?(bgrinstead)

Tracked regression -> P1.

Priority: -- → P1

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.

Flags: needinfo?(bgrinstead)

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

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

Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attachment #9052949 - Attachment description: Bug 1536752 - WIP - fix search in prefs when identity.fxaccounts.enabled is false → Bug 1536752 - Remove sync pane from about:preferences when identity.fxaccounts.enabled is false
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/240085d386be
Remove sync pane from about:preferences when identity.fxaccounts.enabled is false r=Gijs
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68

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

Flags: needinfo?(ke5trel)

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
Attachment #9052949 - Flags: approval-mozilla-beta?
Flags: qe-verify?

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

Status: RESOLVED → VERIFIED
Flags: needinfo?(ke5trel)
QA Whiteboard: [qa-triaged]

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.

Attachment #9052949 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify? → qe-verify+

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.

Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.