Closed Bug 1435786 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Preferences, defect, P1)

59 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 - wontfix
firefox60 --- fixed

People

(Reporter: u607479, Assigned: myk)

References

Details

(Keywords: regression)

Attachments

(1 file)

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
Blocks: 1379338
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Preferences
Flags: needinfo?(myk)
Keywords: regression
[Tracking Requested - why for this release]:
Regression in 59.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
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.
Assignee: nobody → myk
Status: NEW → ASSIGNED
Flags: needinfo?(myk)
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 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.
Attachment #8948553 - Flags: review?(jaws) → review+
(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 on attachment 8948553 [details]
Bug 1435786 - ensure Request English checkbox syncs to/from pref

https://reviewboard.mozilla.org/r/217972/#review224270
Attachment #8948553 - Flags: review+
Pushed by myk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/30d6a1c8f1e1
ensure Request English checkbox syncs to/from pref r=jaws
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 :)
https://hg.mozilla.org/mozilla-central/rev/30d6a1c8f1e1
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Flags: in-qa-testsuite+
You need to log in before you can comment on or make changes to this bug.