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

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Jens]

Attachment: Links in settings do not behave like links

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.

Comment 1

[Clément Lefèvre]

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?

Comment 2

[Jens]

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

Comment 3

[Clément Lefèvre]

(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.

Comment 4

[YF (Yang)]

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.

Comment 5

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

Attachment: MozReview Request: Bug 1268943 - Middle mouse click on links in settings does nothing (should open in new tab). r?gijs

Review commit: https://reviewboard.mozilla.org/r/52281/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52281/

Comment 6

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

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.

Comment 7

[:Gijs (he/him)]

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};

Comment 8

[:Gijs (he/him)]

(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.

Comment 9

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

(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 10

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

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 11

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f215048a527

Comment 12

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

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.

Comment 13

[:Gijs (he/him)]

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);
```

?

Comment 14

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

Attachment: Patch for check-in

Comment 15

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

Attachment: Patch for check-in (qref'd)

Comment 16

[Pulsebot]

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

Comment 17

[Pulsebot]

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

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a356bf678567
https://hg.mozilla.org/mozilla-central/rev/b1ff014a973c

Comment 19

[Jens]

Attachment: Help links and internal links still not middle mouse clickable

Comment 20

[Jens]

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.

Comment 21

[:Gijs (he/him)]

(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.

Comment 22

[Jens]

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

Comment 23

[tabmix.onemen]

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;
>          }
>        }

Comment 24

[Azmina]

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]