Closed Bug 1020245 Opened 10 years ago Closed 10 years ago

calling openPreferences from utilityOverlay.js with a paneID doesn't work for in-content preferences if the preference tab isn't opened yet


(Firefox :: Settings UI, defect)

Not set



Firefox 33


(Reporter: florian, Assigned: peregrino)



(Whiteboard: [translation] p=0 s=33.1 [qa!])


(2 files, 5 obsolete files)

I noticed this when clicking the "Translation Preferences" menuitem of the "Options" menubutton of the translation infobar.

If about:preferences is already open in a tab, switching to the 'content' pane works correctly.

However, if there was no about:preferences tab yet, the tab is loaded, "Content" is selected in the list on the left side, but the "General" pane is shown on the right side.

To make the translation infobar appear on a trunk build, you currently need to have the preferences browser.translation.detectLanguage and set to true, and to load a website in a non-English language (eg.
Flags: firefox-backlog+
Attached patch bug-1020245.diff (obsolete) — Splinter Review
I added a check to only load the general pane on init only if no other pane is intended to be open.
Assignee: nobody → colmeiro
Attachment #8434330 - Flags: review?(jaws)
Comment on attachment 8434330 [details] [diff] [review]

Review of attachment 8434330 [details] [diff] [review]:

This looks to be a race condition between DOMContentLoaded and the advanced-pane-loaded observer notification. Whichever fires last wins. In the case of the tab already being opened, there is no race condition, and the code in utilityOverlay.js will switch to the correct pane. When the preferences aren't already open, then the race condition is present.

The longer term fix for this is to fix so we don't have to do any second guessing about which category to show.

::: browser/components/preferences/in-content/preferences.js
@@ +32,5 @@
>    let categories = document.getElementById("categories");
>    categories.addEventListener("select", event => gotoPref(;
> +  if (document.querySelector('.category[selected="true"]').id == "category-general"){

if (document.getElementById("category-general").selected)) {
Attachment #8434330 - Flags: review?(jaws) → review+
Attached patch proposed fix (obsolete) — Splinter Review
Comments Addressed
Attachment #8434330 - Attachment is obsolete: true
Actually, we should have a test that covers the case of comment #0 so it doesn't regress again. Hernan, can you add a test for this?
The problem is that there's already a test for a similar use case on bug 881576, but it doesn't fail. How long should the test wait for something to unexpectedly change the active tab? Feels weird.
Well, DOMContentLoaded is when the general pane was being opened, so if you wait until "load" then there shouldn't be anymore changes after that.
Ok, I'll look into it tomorrow. The test should be along with the in-content preferences tests, right?
Attached patch WIP + test (obsolete) — Splinter Review
Added tests
Attachment #8434469 - Attachment is obsolete: true
Had to switch to using add_task to use yield. Note that the current test is insufficient as the test shouldn't be calling selectCategory.
Attachment #8436054 - Attachment is obsolete: true
Yes, correct test should be using openPreferences, I will switch to that. Thanks for figuring out the strange promise behavior.
Attached patch proposed fix (obsolete) — Splinter Review
Updated test to use openPreferences from utilityOverlay
Attachment #8436081 - Attachment is obsolete: true
Attachment #8436359 - Flags: review?(jaws)
Comment on attachment 8436359 [details] [diff] [review]
proposed fix

Review of attachment 8436359 [details] [diff] [review]:

r=me with the test rename per

::: browser/components/preferences/in-content/tests/browser.ini
@@ +5,5 @@
>  [browser_advanced_update.js]
>  [browser_bug410900.js]
>  [browser_bug731866.js]
> +[browser_bug1020245.js]

Please change the test name to browser_bug1020245_openPreferences_to_paneContent.js
Attachment #8436359 - Flags: review?(jaws) → review+
Attached patch nits addressedSplinter Review
Attachment #8436359 - Attachment is obsolete: true
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Attached patch small commentSplinter Review
add a comment to preferences.js to clarify that category-general is selected by default
Attachment #8439377 - Flags: review?(jaws)
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Whiteboard: p=0 s=33.1 [qa?]
QA Contact: bogdan.maris
Whiteboard: p=0 s=33.1 [qa?] → p=0 s=33.1 [qa+]
Tested on Ubuntu 13.04 64bit and Mac OS X 10.9.3 using latest Nightly.
If 'Translation Preferences' is clicked and about:preferences is not opened, a new tab is opened and 'Content' is selected in the left pane and right pane.

I noticed that if about:preferences is opened in a new window with focus on 'General' for example, and 'Translation Preferences' is clicked, the focus jumps to the about:preferences window but 'General' is focused. 
Same scenario but with about:preferences in a new tab, will switch focus to 'Content'. I think the scenario with new window should behave like new tab. 
Is this intended behavior?
Flags: needinfo?(colmeiro)
Uhm. I think this is an issue related to how we are opening certain panels in about:preferences. If I run openPreferences from the browser console having an about:preferences tab in another window, I get this error:

> openPreferences('paneContent')
>>> TypeError: browser.contentWindow.selectCategory is not a function

Yesterday I was talking to jaws about the way openPreferences works right now, and he told me he was working on a different bug that would provide a better and more reliable implementation. Maybe we should wait for that one?
Flags: needinfo?(colmeiro) → needinfo?(jaws)
Yes, the way that preferences are opened to a specific category are being changed in bug 754304. That should fix this issue.
Flags: needinfo?(jaws)
Thanks for clarification, I will mark this issue as Verified and reverify this when bug 754304 is fixed.
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: p=0 s=33.1 [qa+] → p=0 s=33.1 [qa!]
Comment on attachment 8439377 [details] [diff] [review]
small comment

This patch won't be necessary once bug 754304 is fixed, which looks to be on track.
Attachment #8439377 - Flags: review?(jaws)
Whiteboard: p=0 s=33.1 [qa!] → [translation] p=0 s=33.1 [qa!]
Blocks: 1042300
You need to log in before you can comment on or make changes to this bug.