Open Bug 672618 Opened 13 years ago Updated 5 months ago

when user ctrl / shift clicks on a javascript url, ignore the modifier

Categories

(Core :: DOM: Core & HTML, defect, P3)

x86
All
defect
Points:
2

Tracking

()

People

(Reporter: beltzner, Unassigned)

References

(Blocks 2 open bugs, )

Details

(Keywords: parity-chrome, ux-error-prevention)

Attachments

(2 files)

Bug 251137 was originally opened with a simple purpose, which was to provide greater control to users and prevent what's becoming a common error scenario:

 - user sees a link on a page
 - user thinks "boy, I want to be sure that opens in a new tab/window"
 - user ctrl/shift clicks on the link

If the link was, in fact, a javascript URI, then what happens is a new tab is opened with that javascript URI unhelpfully in the locationBar. The user expectation was that it would open in a new tab.

Now, in most cases (I have no data to hand, but it is certainly true of virtually every case that I've personally run across) the javascript URI was either going to open that link in a new tab or a popup window. I am *not* advocating that we force popup windows into a new tab - that's apparently much harder, and someone's free to file a different bug for that issue.

What I *am* advocating is that a simple fix to get closer to user expectations would be to ignore the modifier when a user clicks on a javascript URI. There are a couple of corner cases which could annoy people:

 - user wanted to open in new window, and instead it opens a new tab, or
 - user wanted to open in a new tab, and instead it opens a popup
 - instead it does some JS thing on the page (ie: opens in lightbox)

But I would argue both of those are better than

 - it opens a useless tab
Attached patch patchSplinter Review
I think this is sufficient, but I don't particularly have time to look into tests at the moment.
Comment on attachment 546891 [details] [diff] [review]
patch

Asking for review (Dao, feel free to reassign, but I figure you know this area best) and ui-review (ohai, Alex!)

Also asking for feedback from bz (who is wise in the way of loading documents and such) and Jesse (who is wise in the way of security and how this might intersect with it, though I struggle to see that as an issue)
Attachment #546891 - Flags: ui-review?(faaborg)
Attachment #546891 - Flags: review?(dao)
Attachment #546891 - Flags: feedback?(jruderman)
Attachment #546891 - Flags: feedback?(bzbarsky)
Keywords: ux-control
Comment on attachment 546891 [details] [diff] [review]
patch

I have been accused of patch stealing; my bad. Mostly wanted to get the ui team's opinion.
Attachment #546891 - Flags: ui-review?(faaborg)
Attachment #546891 - Flags: review?(dao)
Attachment #546891 - Flags: feedback?(jruderman)
Attachment #546891 - Flags: feedback?(bzbarsky)
Seems like a reasonable thing to do until we can fix bug 55696.

Bug 151142 and bug 251137 may be related. Bug 138198 is the SeaMonkey equivalent.

Comments on the patch:

* It might be good for the comment to explain why we're doing this (by referring to bug 55696 and/or bug 335963).

* Please don't parse protocols out of URLs using regexps.

* We should make sure the patch does the right thing for middle-clicks.
Comment on attachment 546891 [details] [diff] [review]
patch

yep, better than a useless tab
Attachment #546891 - Flags: ui-review+
(In reply to comment #4)
> * Please don't parse protocols out of URLs using regexps.

Generally I would agree with this, but there's no scriptable way to access the link's existing underlying nsIURI, as far as I know, and creating another nsIURI manually just to parse the scheme is unnecessary overhead - the .href return value is nsIURI.spec, so we don't need to deal with arbitrary values, and this particular check isn't critical to get 100% right.
In an ideal world we'd have such a getter, btw... Worth filing a bug on.  Even more, we'd ideally be checking that this is a script-executing protocol instead of assuming that javascript: is the only such beastie.

But yeah, I think in this case this is probably alright.  Certainly the failure mode is not insecure or anything like that, which is the usual worry with manual parsing and scheme white/blacklisting.
The last comment was 2 years ago - what's the current status?  It seems to have been partly implemented.  This is actually a real annoyance.

When I middle-click a link, I want to open the linked-to page in a new tab.  Even if the JS implementation of the link is such that the browser can't open it in a new tab, opening it in the current tab (and consequently throwing me out of some Flash game or other interactive feature I am using on the page) without warning me first is the absolute last thing I want.

Other bugs I've found about the handling of middle-click on JavaScript URLs:

Bug 126862 - middle click on javascript links should act like normal click
Bug 138198 - "open link in new tab" (middle or ctrl click) doesn't work for javascript links (should act like normal click)
Bug 175836 - middle-click on javascript urls should... work
Bug 226497 - Middle clicking on javascript link or form button does not open new tab.
Whiteboard: p=0
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Whiteboard: p=0 → p=2
Points: --- → 2
Flags: qe-verify?
Whiteboard: p=2
Depends on: 55696
Blocks: 1462131
No longer depends on: 55696
Keywords: parity-chrome
Priority: -- → P3
See Also: → 55696
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED

(In reply to Thomas Wolff from comment #13)

The original report is "advocating", according to its own wording, so why was my comment hidden as "advocacy".
How to handle this is obviously controversial and discussion is important.
About your patch: please don't! See my "advocacy".

You added nothing to the discussion other than that you'd like bug 55696 to be fixed instead. That logic is flawed; not fixing this bug won't make it more likely that we'll fix bug 55696, as the last several years have shown.

Component: General → DOM: Core & HTML
Product: Firefox → Core
Blocks: 55696
See Also: 55696

I'd like to understand what the core DOM bit here is, exactly. Generally speaking, that part should all be covered by specs, so either we're not following the spec now (in what way?) or we're not following the spec after the changes (in what way and why?) or whatever the proposed change is is not covered by the spec (should probably be fixed)...

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #15)

I'd like to understand what the core DOM bit here is, exactly.

This code prevents default link handling when using a modifier: https://searchfox.org/mozilla-central/rev/040aa667f419932adf425d92c7438f03230ad96b/dom/base/Element.cpp#3095-3099

If changing this is a spec violation, then Chrome already violates the spec, and the spec should likely be updated...

Yes, I know what our existing code does. My question was what the proposed changes to that are.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #17)

Yes, I know what our existing code does. My question was what the proposed changes to that are.

Ignore the modifiers for js links, i.e. open the link as if no modifier was used... There's a patch attached where you could take a closer look.

Yes, I saw the patch. What I wanted to know is what the patch is trying to do as opposed to what it's doing, because one of the things a DOM reviewer will presumably want to check is whether what it's doing is what it's actually trying to do.

The comments about "current window" in the patch don't help, because there's no guarantee that a left click will load the link "in the current window" either (e.g. the <a> could have a target attribute), and that made the "what is the goal?" question even harder to divine from the code+comments. I guess smaug already commented about that...

I'm a little torn about whether it's better to handle this stuff in the core element code or in the UI code that normally handles modifier-clicks anyway. The UI code could just call .click() on the anchor in question if it wants to trigger it "normally", right?

target handling is an open question, I was going to investigate further Chrome's behavior.

Using click() is an interesting idea, I can give that a try.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #19)

The UI code could just call .click() on the anchor in question if it wants to trigger it "normally", right?

This sort of works but triggers another click event. So this:

data:text/html;,<a href="javascript:" onclick="alert('hi')">click me

... gives one alert for a middle click (which might be expected), but two alerts for e.g. shift+click which definitely isn't expected. I'm guessing we'd want to hide the original click event from the page. Can this be done from UI code in the child process? event.stopPropagation() in ClickHandlerChild.jsm doesn't seem to work. This is a capturing listener in the system group.

Flags: needinfo?(bzbarsky)

gives one alert for a middle click (which might be expected)

That's a bug. See bug 1533630. That said, for modifier+left-click this would in fact be an issue.

Maybe we should just add an API on anchors (chromeonly) to trigger the activation behavior directly. As a core change, I'd prefer that to adding javascript: special-casing in the click handling, I suspect, but I'd like to know what Olli thinks.

Flags: needinfo?(bzbarsky) → needinfo?(bugs)

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #22)

Maybe we should just add an API on anchors (chromeonly) to trigger the activation behavior directly. As a core change, I'd prefer that to adding javascript: special-casing in the click handling, I suspect, but I'd like to know what Olli thinks.

Hmm, wouldn't dispatching a DOMActivate event do the trick?

It would, for the moment, but that's also page-observable, of course. Maybe that's OK.

Middle click on data:text/html;,<a href="javascript:" onclick="alert('hi')">click me
shouldn't show any alert, since middle click doesn't produce click events on web, bug 1379466

DOMActivate without click is perhaps confusing.

A chromeonly method on anchors and other links to trigger activation behavior sounds good to me. But it should
still somehow deal with the link target. Should it just not care about target if scheme is javascript - is that what other browsers do?
(If so, that is something to get to the spec)

Flags: needinfo?(bugs)

Not a solution, but perhaps a partial workaround.

  1. With links, Firefox can already show a status panel in the lower-left corner.

  2. With some fake-links, Firefox can already NOT show a status panel in the lower left corner.

  3. With some fake-links, Firefox shows code such as javascript:; instead of a url.

The status panel, or its absence, is out of the way, hard to see, and therefore, not much use to users who don't expect to encounter this bug.

I don't know the code, but whatever cues Firefox uses to show or not show a status panel could also be used to show a link cursor or a fake-link cursor on mouseover, where 1. a status panel NOT including "javascript" in its location would get a link icon, and 2. a status panel including "javascript" in its location, or the lack of a status panel, or a status panel of length 0, would get a fake-link icon.

I posted a version of that as bug 1690165, which is currently WON'TFIX.

Severity: normal → S3

Changing qe-verify? to qe-verify+.

Flags: qe-verify? → qe-verify+
Assignee: dao+bmo → nobody
Status: ASSIGNED → NEW
See Also: → 1857466
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: