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

RESOLVED FIXED in Firefox 23

Status

Firefox Health Report
Client: Desktop
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mconnor, Assigned: mconnor)

Tracking

unspecified
Firefox 23
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 739302 [details] [diff] [review]
open in new window if instantApply is false
Attachment #739302 - Flags: review?(jaws)
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 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.
Attachment #739302 - Flags: review?(jaws)
(Assignee)

Comment 4

5 years ago
Created attachment 744327 [details] [diff] [review]
v2

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

addressed other comments/
Attachment #739302 - Attachment is obsolete: true
Attachment #744327 - Flags: review?(jaws)
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.
(Assignee)

Comment 6

5 years ago
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 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.
Attachment #744327 - Flags: review?(jaws) → review+

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/a0dace7878d9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services]
Target Milestone: --- → Firefox 23
You need to log in before you can comment on or make changes to this bug.