863082 - Prefpane links should open in a new window if prefwindow is modal

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Mike Connor [:mconnor]]

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sync/utils.js#12 is an example of how this should work.

Not a blocker, but annoying and should get fixed.

Comment 1

[Mike Connor [:mconnor]]

Attachment: open in new window if instantApply is false

Comment 2

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

Comment on attachment 739302 [details] [diff] [review]
open in new window if instantApply is false

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

::: browser/components/preferences/advanced.js
@@ +150,5 @@
> +   */
> +  openTextLink: function (evt) {
> +    let where = Services.prefs.getBoolPref("browser.preferences.instantApply") ? "tab" : "window";
> +    openUILinkIn(evt.target.getAttribute("targetUrl"), where);
> +    return false;

Please use evt.preventDefault() here.

@@ +162,5 @@
>      let url = Services.prefs.getCharPref(pref);
>      let el = document.getElementById(element);
>  
>      if (url) {
> +      el.setAttribute("targetUrl", url);

Why does this attribute need to be changed from 'href' to 'targetURL'?

::: browser/components/preferences/advanced.xul
@@ +207,5 @@
>                <spacer flex="1"/>
>                <label id="telemetryLearnMore"
>                       class="text-link"
> +                     value="&telemetryLearnMore.label;"
> +                     onclick="gAdvancedPane.openTextLink(event)"/>

Does this get fired if the user uses the keyboard to access the link?

Comment 3

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

Comment on attachment 739302 [details] [diff] [review]
open in new window if instantApply is false

Please reflag for review after addressing the questions or uploading a new version.

Comment 4

[Mike Connor [:mconnor]]

Attachment: v2

Pressing enter while the link is focused will open the link.

addressed other comments/

Comment 5

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

Actually, I'm not sure why this patch is necessary. I tested the links on the Advanced pane and they open up in tabs of the most recent window already (1 May 2013 Nightly on Win7).

The only links that have issues are the ones for the Sync pane, but those have an onclick handler that calls event.stopPropagation and then some JS to open the links. The Sync labels can probably be cleaned up to not work in their unusual way.

Comment 6

[Mike Connor [:mconnor]]

They open in tabs behind a modal window,  which is poor UX if the idea is to read that link as a part of making a decision. The Help button follows the same logic as this patch for that same reason.

Comment 7

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

Comment on attachment 744327 [details] [diff] [review]
v2

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

We really should just either fix text-link or openUILinkIn to cause the link to be opened in a new window if the browser window is being blocked by a modal dialog. Can you file a followup for this? It may be a good candidate for a mentored bug.

::: browser/components/preferences/advanced.js
@@ +151,5 @@
> +  openTextLink: function (evt) {
> +    let where = Services.prefs.getBoolPref("browser.preferences.instantApply") ? "tab" : "window";
> +    openUILinkIn(evt.target.getAttribute("href"), where);
> +    evt.preventDefault()
> +    return false;

We can remove the |return false;| here since preventDefault is being called now.

Comment 8

[Mike Connor [:mconnor]]

https://hg.mozilla.org/services/services-central/rev/a0dace7878d9

Comment 9

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/a0dace7878d9