1102961 - Cannot navigate without closing the Options window

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Petruta Horea [:phorea]]

Reproduces in Firefox 34 beta 11 under Win 7 64-bit.

STR:
1. Select "Change Search Settings" from search toolbar arrow - Options window with Search tab selected is opened
2. Click on "Add more search providers..." link - https://addons.mozilla.org/en-US/firefox/search/?atype=4 page opens
3. Select a new search provider from the AMO list and add it to Firefox

ER: User can navigate to the AMO page (such as in Ubuntu and Mac OSX)

AR: The Options window remains in view and it must be closed before navigating to AMO page

Comment 1

[Florian Quèze [:florian]]

Note: this problem goes away with in-content preferences.

Comment 2

[Lawrence Mandel [:lmandel] (use needinfo)]

This isn't a blocker for 34. I've marked 34 as wontfix and 35+ as affected. I don't think this needs to track.

Comment 3

[Florian Quèze [:florian]]

Here is how the old engine manager dialog handled this: http://hg.mozilla.org/mozilla-central/annotate/cb8ad2251c09/browser/components/search/content/engineManager.js#l74

Comment 4

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 1 - closing the options window

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 8540846 [details] [diff] [review]
rev 1 - closing the options window

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

Thanks for the patch, and sorry for the delay in reviewing this!

::: browser/components/preferences/search.js
@@ +64,5 @@
>      Services.search.currentEngine =
>        document.getElementById("defaultEngine").selectedItem.engine;
>      this.displayOneClickEnginesList();
> +  },
> +  

Nit: please avoid adding trailing whitespace.

@@ +67,5 @@
> +  },
> +  
> +  loadAddEngines: function () {
> +    window.opener.BrowserSearch.loadAddEngines();
> +    window.close();

This will not save any change the user made. To save the changes and close the window, you can use instead:
  window.document.documentElement.acceptDialog();

::: browser/components/preferences/search.xul
@@ +50,5 @@
>        <label>The search bar lets you search alternate engines directly. Choose which ones to display.</label>
>  
>        <richlistbox id="oneClickProvidersList"/>
>        <hbox pack="end" style="margin-bottom: 1em">
>          <label id="addEngines" class="text-link" value="Add more search providers…"

I think this patch was based on an old revision of the code, strings are no longer hardcoded.

Comment 6

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 2 - closing the options window

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 8544543 [details] [diff] [review]
rev 2 - closing the options window

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

Thanks for updating the patch! :-)

::: browser/components/preferences/search.js
@@ +183,5 @@
> +  },
> +
> +  loadAddEngines: function () {
> +    window.opener.BrowserSearch.loadAddEngines();
> +    window.document.documentElement.acceptDialog();

Note: this won't close the dialog on Mac (due to http://hg.mozilla.org/mozilla-central/annotate/2a193b7f395c/toolkit/content/widgets/preferences.xml#l637) but I don't think that's a problem. The bug we are trying to fix here is really that on Windows it's currently not possible to browse the AMO page after clicking the link; closing the preferences dialog is not a goal in itself.

Comment 8

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/4f0b4498fae5

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4f0b4498fae5

Comment 10

[Florian Quèze [:florian]]

Comment on attachment 8544543 [details] [diff] [review]
rev 2 - closing the options window

Approval Request Comment
[Feature/regressing bug #]: bug 1088660
[User impact if declined]: Clicking the "add more engine" link in the Search prefpane looks broken on Windows because the modal pref dialog stays above the new tab that's opened.
[Describe test coverage new/current, TBPL]: currently on m-c, will be verified by QA.
[Risks and why]: low, self contained change.
[String/UUID change made/needed]: none.

Comment 11

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fixed on Windows 7 using latest Aurora 37.0a2 (buildID: 20150113004007).

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5ac62d0df17e

Comment 13

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fixed on Windows 7 64bit and 32bit using Firefox 36 Beta 2 (buildID: 20150120155007).