1435786 - "Request English versions…" checkbox in Preferences can't be unchecked after being checked

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[u607479]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20100101

Steps to reproduce:

1. Set the "privacy.resistFingerprinting" pref to true (needed for the relevant checkbox to be shown).
2. Open Preferences, and in General section under the Language heading, click the "Choose…" button.
3. Check the "Request English versions…" checkbox, then uncheck it, then click "OK" to close languages dialog.
4. Click the languages "Choose…" button again and notice the "Request English versions…" checkbox is checked - the uncheck in step 3 wasn't saved.

From this point on, the checkbox will always be checked when opening the languages dialog.

mozregression result: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d760cf06ca40056077e2a354b8b69350a5765ae3&tochange=0adedb70b7883ab39239b8531d07f1308ffc10c7

Comment 1

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
Regression in 59.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

This is probably due to:

>document.getElementById(...) is null
>  readSpoofEnglish chrome://browser/content/preferences/languages.js:322:24
>  readAcceptLanguages chrome://browser/content/preferences/languages.js:166:5
>  anonymous chrome://global/content/preferencesBindings.js%20line%20341%20%3E%20Function:3:8
>  setElementValue chrome://global/content/preferencesBindings.js:343:16
>  updateElements chrome://global/content/preferencesBindings.js:461:9
>  _add/< chrome://global/content/preferencesBindings.js:41:44

The fix is straightforward, but there aren't any tests for that checkbox (nor even the Languages subdialog generally), so I'll add some.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Attachment: Bug 1435786 - ensure Request English checkbox syncs to/from pref

Review commit: https://reviewboard.mozilla.org/r/217972/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217972/

Comment 4

[Myk Melez [:myk] [@mykmelez]]

The bug fix is just:

-    var spoofEnglish = document.getElementById("privacy.spoof_english").value;
+    var spoofEnglish = Preferences.get("privacy.spoof_english").value;

I also fixed a few typos I noticed in the area (lanauges -> languages).

And I saw some intermittent failures calling gLanguagesDialog.forceReflow() due to what appears to be a race condition between nsRefreshDriver::Tick and script loading/evaluation, which results in the onresize handler sometimes calling gLanguagesDialog.forceReflow() before gLanguagesDialog is defined.  I fixed that by moving the onresize handler (and onfocus handler, for good measure) to the end of languages.js to ensure gLanguagesDialog gets defined first.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=320faf5d2a4076dd01bae8a40a8a3b18688ecd0b

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8948553 [details]
Bug 1435786 - ensure Request English checkbox syncs to/from pref

https://reviewboard.mozilla.org/r/217972/#review223906

::: browser/components/preferences/in-content/tests/browser.ini:52
(Diff revision 1)
>  [browser_defaultbrowser_alwayscheck.js]
>  [browser_healthreport.js]
>  skip-if = true || !healthreport # Bug 1185403 for the "true"
>  [browser_homepages_filter_aboutpreferences.js]
>  [browser_extension_controlled.js]
> +[browser_languages_subdialog.js]

Thank you for the added test!

::: browser/components/preferences/in-content/tests/browser_languages_subdialog.js:22
(Diff revision 1)
> +  {
> +    is(dialogOverlay.style.visibility, "", "The dialog is invisible.");
> +    const win = await languagesSubdialogOpened();
> +    ok(win.document.getElementById("spoofEnglish").hidden, "The 'Request English' checkbox is hidden.");
> +    closeLanguagesSubdialog();
> +    is(dialogOverlay.style.visibility, "", "The dialog is invisible.");
> +  }

Why does `win` need to be a const here? It seems this is the only reason that a block was needed, but we can just declare `win` as a `let` variable and then overwrite it on line 36 below.

::: browser/components/preferences/in-content/tests/browser_languages_subdialog.js:35
(Diff revision 1)
> +  {
> +    let win = await languagesSubdialogOpened();

This doesn't look like it needs its own block.

::: browser/components/preferences/languages.js:322
(Diff revision 1)
>      if (!resistFingerprinting) {
>        checkbox.hidden = true;
>        return false;
>      }
>  
> -    var spoofEnglish = document.getElementById("privacy.spoof_english").value;
> +    var spoofEnglish = Preferences.get("privacy.spoof_english").value;

I searched the preferences directory for all other places we use `document.getElementById`. All other places are referring to DOM elements, so we're good now.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8948553 [details]
Bug 1435786 - ensure Request English checkbox syncs to/from pref

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217972/diff/1-2/

Comment 7

[Myk Melez [:myk] [@mykmelez]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)

> :::
> browser/components/preferences/in-content/tests/browser_languages_subdialog.
> js:22
> (Diff revision 1)
> > +  {
> > +    is(dialogOverlay.style.visibility, "", "The dialog is invisible.");
> > +    const win = await languagesSubdialogOpened();
> > +    ok(win.document.getElementById("spoofEnglish").hidden, "The 'Request English' checkbox is hidden.");
> > +    closeLanguagesSubdialog();
> > +    is(dialogOverlay.style.visibility, "", "The dialog is invisible.");
> > +  }
> 
> Why does `win` need to be a const here? It seems this is the only reason
> that a block was needed, but we can just declare `win` as a `let` variable
> and then overwrite it on line 36 below.
> 
> :::
> browser/components/preferences/in-content/tests/browser_languages_subdialog.
> js:35
> (Diff revision 1)
> > +  {
> > +    let win = await languagesSubdialogOpened();
> 
> This doesn't look like it needs its own block.

I originally wrote it this way because I was considering writing additional tests and wanted to isolate them from each other.  I agree that it doesn't make enough sense to isolate only two blocks of code, so I've removed the isolation.


> ::: browser/components/preferences/languages.js:322
> (Diff revision 1)
> >      if (!resistFingerprinting) {
> >        checkbox.hidden = true;
> >        return false;
> >      }
> >  
> > -    var spoofEnglish = document.getElementById("privacy.spoof_english").value;
> > +    var spoofEnglish = Preferences.get("privacy.spoof_english").value;
> 
> I searched the preferences directory for all other places we use
> `document.getElementById`. All other places are referring to DOM elements,
> so we're good now.

I've done that on a few occasions, only to be burned by a new instance that I somehow didn't notice.  I hope your search is more thorough than mine have been!

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8948553 [details]
Bug 1435786 - ensure Request English checkbox syncs to/from pref

https://reviewboard.mozilla.org/r/217972/#review224270

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/30d6a1c8f1e1
ensure Request English checkbox syncs to/from pref r=jaws

Comment 10

[Ryan VanderMeulen [:RyanVM]]

I don't think this needs to be tracked for the 59 release since it requires a non-default pref to be set in order to reproduce, but we'll still entertain the uplift request :)

Comment 11

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/30d6a1c8f1e1