Closed Bug 1268943 Opened 5 years ago Closed 5 years ago

Middle mouse click on links in settings does nothing (should open in new tab)

Categories

(Firefox :: Preferences, defect)

46 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 49
Tracking Status
firefox49 --- verified

People

(Reporter: nachtigall, Assigned: jaws)

References

Details

(Keywords: ux-consistency)

Attachments

(3 files, 2 obsolete files)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160425115337

Steps to reproduce:

1. Go to settings.
2. Hover over a link (shows no URL at left bottom to where I'll be directed)
3. Middle mouse click on a link (does nothing)

See screenshot.


Actual results:

Nothing.


Expected results:

Just do as is done for other links to:
- Hovering shows URL at left bottom
- Middle mouse click opens in new tab, currently only left mouse click opens it.
Currently not reproducing, which build do you use? Is it distribution-packaged Firefox on the one downloaded from Mozilla? In the first case, can you check if you reproduce with Mozilla's one?
Flags: needinfo?(nachtigall)
I can reproduce this on Firefox 46 and Nightly on both Windows and Ubuntu. 

This is *not* about mouse *left* click but about:
- mouse *middle* click on link does not open page in new tab (does nothing atm)
- no URL preview on left bottom of page when hovering link
- mouse pointer not changing from normal to pointer (the "hand") when hovering link

A link should behave like a link, regardless if its on the internet or within the (local) settings. Links within settings behave different, which is not a good UX
Flags: needinfo?(nachtigall) → needinfo?(clement.lefevre)
(In reply to Jens from comment #2)
> I can reproduce this on Firefox 46 and Nightly on both Windows and Ubuntu. 
> 
> This is *not* about mouse *left* click but about:
> - mouse *middle* click on link does not open page in new tab (does nothing
> atm)
> - no URL preview on left bottom of page when hovering link
> - mouse pointer not changing from normal to pointer (the "hand") when
> hovering link
> 
> A link should behave like a link, regardless if its on the internet or
> within the (local) settings. Links within settings behave different, which
> is not a good UX

Hum, ok. My bad, I accidentaly skipped the part telling it was about settings.
So, In both OS X and Archlinux, I can confirm that there are no URL preview and that the middle click doesn't work, but not for the missing hand: I currently get that hand whether I use version 46 or nightly.

However, maybe is this wished, I don't actually know about this.
Flags: needinfo?(clement.lefevre)
It is not an pure links, these link-buttons use the same appearance.

It is always open in a new tab, so middle-click is not obvious useful, but it should be supported.


I found right-click the "manage your Do Not Track settings" link have a context menu instead, that is not a rational design. The "clear your recent history" and "remove individual cookies" have the same problem, they use <a> tag instead.
Status: UNCONFIRMED → NEW
Component: Untriaged → Preferences
Ever confirmed: true
Keywords: ux-consistency
OS: Unspecified → All
Hardware: Unspecified → All
See Also: → 1268164
What are your ideas for a test for this? Ideally, I could add to /toolkit/content/tests/chrome/test_labelcontrol.xul but since this is in /toolkit I don't know if it would be accepted to add a dependency on a tabbed browser here?

Another route would be to add an in-content pref test that clicks on one of the text-links within the preferences, but I would like the test to outlive any preference redesign.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8751895 [details]
MozReview Request: Bug 1268943 - Middle mouse click on links in settings does nothing (should open in new tab). r?gijs

https://reviewboard.mozilla.org/r/52281/#review49199

::: browser/components/nsBrowserGlue.js:384
(Diff revision 1)
> +            let where = win.whereToOpenLink(data);
> +            win.openUILinkIn(data.href, where);

AFAICT can just do win.openUILink(data.href, data); ?

::: toolkit/content/widgets/text.xml:359
(Diff revision 1)
> +          let data = {
> +            href,
> +            shiftKey: aEvent.shiftKey,
> +            ctrlKey: aEvent.ctrlKey,
> +            metaKey: aEvent.metaKey,
> +            altKey: aEvent.altKey,
> +            button: aEvent.button,
> +          };

Nit: let {shiftKey, ctrlKey, metaKey, altKey, button} = aEvent;
let data = {shiftKey, ctrlKey, metaKey, altKey, button, href};
Attachment #8751895 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> What are your ideas for a test for this? Ideally, I could add to
> /toolkit/content/tests/chrome/test_labelcontrol.xul but since this is in
> /toolkit I don't know if it would be accepted to add a dependency on a
> tabbed browser here?

Dependency in what sense? There are browser_*.js tests in toolkit, including ones that open tabs etc., but we should probably have a separate test-only page to test with (ie not about:preferences itself). Does that make some degree of sense? It's a little bit evil that the functionality all depends on JS that lives in browser/ anyway, but I don't have suggestions on how to fix that.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #7)
> Comment on attachment 8751895 [details]
> MozReview Request: Bug 1268943 - Middle mouse click on links in settings
> does nothing (should open in new tab). r?gijs
> 
> https://reviewboard.mozilla.org/r/52281/#review49199
> 
> ::: browser/components/nsBrowserGlue.js:384
> (Diff revision 1)
> > +            let where = win.whereToOpenLink(data);
> > +            win.openUILinkIn(data.href, where);
> 
> AFAICT can just do win.openUILink(data.href, data); ?

openUILink will require the target and target.ownerDocument. I wasn't sure what we would want to put there, especially since we are sending this through observers where we use strings as their data.
Comment on attachment 8751895 [details]
MozReview Request: Bug 1268943 - Middle mouse click on links in settings does nothing (should open in new tab). r?gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52281/diff/1-2/
Comment on attachment 8751895 [details]
MozReview Request: Bug 1268943 - Middle mouse click on links in settings does nothing (should open in new tab). r?gijs

(mozreview-re-request-review-dance)

some slight behavior change in text.xml and also so you can look at the test.
Attachment #8751895 - Flags: review?(gijskruitbosch+bugs)
Attachment #8751895 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8751895 [details]
MozReview Request: Bug 1268943 - Middle mouse click on links in settings does nothing (should open in new tab). r?gijs

https://reviewboard.mozilla.org/r/52281/#review49387

::: toolkit/content/widgets/text.xml:361
(Diff revisions 1 - 2)
> -            shiftKey: aEvent.shiftKey,
> -            ctrlKey: aEvent.ctrlKey,
> -            metaKey: aEvent.metaKey,
> -            altKey: aEvent.altKey,
> +            // Preserve legacy behavior of non-modifier left-clicks
> +            // opening in a new selected tab.
> +            ctrlKey = true;
> +            metaKey = true;

Feels like this should only set one of these depending on what OS we're on.

::: toolkit/content/tests/browser/browser_label_textlink.js:16
(Diff revision 2)
> +    is(gBrowser.tabs.length, originalTabCount + 1, "the new tab should be opened");
> +    isnot(gBrowser.selectedBrowser, browser, "selected tab should be example page");

Maybe just:
```
let tabOpenPromise = BrowserTestUtils.waitForNewTab(gBrowser, "http://www.example.com/");
// click
let tab = yield tabOpenPromise;
is(gBrowser.selectedTab, tab, ...);
yield BrowserTestUtils.removeTab(tab);
```

?
Attached patch Patch for check-in (obsolete) — Splinter Review
Attachment #8751895 - Attachment is obsolete: true
Attachment #8752262 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/a356bf678567
https://hg.mozilla.org/mozilla-central/rev/b1ff014a973c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
First, thanks a lot for fixing this very fast! 

But this fixes only part of my bug report, see new attachment "Help links and internal links still not middle mouse clickable":

1. The help links at right top [?] still have mouse cursor as "default" where it should be "pointer" (the hand as used for links).

2. The help links are not middle mouse clickable

3. Internal links are not middle mouse clickable. That is, things like "manage your Do Not Track settings", "clear your recent history" or "remove individual cookies". These should also just open in new tabs or if this in inappropriate, e.g. if it opens a popup, then at least do act the same like left mouse click. For a user it is just strange, that middle mouse click does just nothing. At least please make this work like middle mouse click on the left side tabs like "General", "Search", "Content",... which just work.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Jens from comment #20)
> First, thanks a lot for fixing this very fast! 
> 
> But this fixes only part of my bug report, see new attachment "Help links
> and internal links still not middle mouse clickable":
> 
> 1. The help links at right top [?] still have mouse cursor as "default"
> where it should be "pointer" (the hand as used for links).

The help item is not a link, it is a button.

> 2. The help links are not middle mouse clickable

See above. This is platform consistent with help buttons on OS X and Windows.

> 3. Internal links are not middle mouse clickable. That is, things like
> "manage your Do Not Track settings",
Opens a dialog.

> "clear your recent history"
Opens a dialog.

> or "remove individual cookies".
Opens a dialog.

> These should also just open in new tabs or if this in
> inappropriate, e.g. if it opens a popup, then at least do act the same like
> left mouse click.

Please file a separate bug for this. This bug has landed code and so it shouldn't be reopened unless we back out the patch.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
See Also: → 1273515
Gijs, thanks, I didn't know how Fixed vs. Reopening applies to patches.

> The help item is not a link, it is a button.

I do not agree, maybe technically, but from the user perspective it is like an link. It's even an external link.

>> 2. The help links are not middle mouse clickable

> See above. This is platform consistent with help buttons on OS X and Windows.

I testet this on Windows 8.1 but failed to see a [?] help button anywhere within its settings pages. I'd also say that in-content browser settings are different than OS settings.

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1273517 for further discussion.

>> 3. Internal links are not middle mouse clickable...

> Please file a separate bug for this. This bug has landed code and so it shouldn't be reopened unless we back out the patch.

Done: https://bugzilla.mozilla.org/show_bug.cgi?id=1273515
See Also: → 1273517
The "Preserve legacy behavior" part in text.js#text-link prevents any one that observes "handle-xul-text-link" from getting the real event data.

>          if (!shiftKey && !altKey) {
>            // Preserve legacy behavior of non-modifier left-clicks
>            // opening in a new selected tab.
>            let {AppConstants} =
>              Components.utils.import("resource://gre/modules/AppConstants.jsm", {});
>            if (AppConstants.platform == "macosx") {
>              metaKey = true;
>            } else {
>              ctrlKey = true;
>            }
>          }

Tab mix add option to inverse the behavior of "browser.tabs.loadInBackground" when user middle-click on links, to one can click open the link and stay on the current page.

Is it possible to move the "Preserve legacy behavior" to "handle-xul-text-link" observer
>        if (!linkHandled.data) {
>          let win = RecentWindow.getMostRecentBrowserWindow();
>          if (win) {
>            data = JSON.parse(data);
>            let where = win.whereToOpenLink(data);
>+           // Preserve legacy behavior of non-modifier left-clicks
>+           // opening in a new selected tab.
>+           if (where == "current") {
>+             where = "tab";
>+           }
>            win.openUILinkIn(data.href, where);
>            linkHandled.data = true;
>          }
>        }
Blocks: 1274533
I have reproduced this bug with Nightly 49.0a1 (2016-04-29) on Windows 8.1, 64 bit!

This Bug's fix is verified on Latest Aurora 49.0a2 .

Build ID : 20160727004019
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0

[bugday-20160727]
Status: RESOLVED → VERIFIED
Depends on: 1300107
You need to log in before you can comment on or make changes to this bug.