Closed Bug 863082 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 23

People

(Reporter: mconnor, Assigned: mconnor)

Details

Attachments

(1 file, 1 obsolete file)

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.
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?
Status: NEW → ASSIGNED
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)
Attached patch v2Splinter Review
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.
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+
https://hg.mozilla.org/mozilla-central/rev/a0dace7878d9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services]
Target Milestone: --- → Firefox 23
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.