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

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Florian Quèze [:florian]]

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 browser.translation.ui.show set to true, and to load a website in a non-English language (eg. http://pt.wikipedia.org).

Comment 1

[Hernán Rodriguez Colmeiro (:peregrino)]

Attachment: bug-1020245.diff

I added a check to only load the general pane on init only if no other pane is intended to be open.

Comment 2

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

Comment on attachment 8434330 [details] [diff] [review]
bug-1020245.diff

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 https://bugzilla.mozilla.org/show_bug.cgi?id=754304 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(event.target.value));
>  
> +  if (document.querySelector('.category[selected="true"]').id == "category-general"){

if (document.getElementById("category-general").selected)) {

Comment 3

[Hernán Rodriguez Colmeiro (:peregrino)]

Attachment: proposed fix

Comments Addressed

Comment 4

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

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?

Comment 5

[Hernán Rodriguez Colmeiro (:peregrino)]

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.

Comment 6

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

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.

Comment 7

[Hernán Rodriguez Colmeiro (:peregrino)]

Ok, I'll look into it tomorrow. The test should be along with the in-content preferences tests, right?

Comment 8

[Hernán Rodriguez Colmeiro (:peregrino)]

Attachment: WIP + test

Added tests

Comment 9

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

Attachment: WIP + test (updated to use yield)

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.

Comment 10

[Hernán Rodriguez Colmeiro (:peregrino)]

Yes, correct test should be using openPreferences, I will switch to that. Thanks for figuring out the strange promise behavior.

Comment 11

[Hernán Rodriguez Colmeiro (:peregrino)]

Attachment: proposed fix

Updated test to use openPreferences from utilityOverlay

Comment 12

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

Comment on attachment 8436359 [details] [diff] [review]
proposed fix

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

r=me with the test rename per https://mail.mozilla.org/pipermail/firefox-dev/2013-September/000957.html

::: 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

Comment 13

[Hernán Rodriguez Colmeiro (:peregrino)]

Attachment: nits addressed

Comment 14

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

https://tbpl.mozilla.org/?tree=Try&rev=7ec79d6fb6f9

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/156e9796cad4

Comment 16

[Hernán Rodriguez Colmeiro (:peregrino)]

Attachment: small comment

add a comment to preferences.js to clarify that category-general is selected by default

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/156e9796cad4

Comment 18

[Bogdan Maris, Desktop Test Engineering]

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?

Comment 19

[Hernán Rodriguez Colmeiro (:peregrino)]

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?

Comment 20

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

Yes, the way that preferences are opened to a specific category are being changed in bug 754304. That should fix this issue.

Comment 21

[Bogdan Maris, Desktop Test Engineering]

Thanks for clarification, I will mark this issue as Verified and reverify this when bug 754304 is fixed.

Comment 22

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

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.