when user ctrl / shift clicks on a javascript url, ignore the modifier
Categories
(Core :: DOM: Core & HTML, defect, P3)
Tracking
()
People
(Reporter: beltzner, Assigned: dao)
References
(Blocks 2 open bugs, )
Details
(Keywords: parity-chrome, ux-error-prevention)
Attachments
(2 files)
(deleted),
patch
|
faaborg
:
ui-review+
|
Details | Diff | Splinter Review |
(deleted),
text/x-phabricator-request
|
Details |
Comment 1•13 years ago
|
||
Reporter | ||
Comment 2•13 years ago
|
||
Updated•13 years ago
|
Reporter | ||
Comment 3•13 years ago
|
||
Comment 4•13 years ago
|
||
Comment 5•13 years ago
|
||
Comment 6•13 years ago
|
||
Comment 7•13 years ago
|
||
Comment 8•11 years ago
|
||
Updated•11 years ago
|
Updated•11 years ago
|
Updated•11 years ago
|
Updated•10 years ago
|
Updated•10 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Comment hidden (advocacy) |
Assignee | ||
Comment 12•5 years ago
|
||
Comment hidden (off-topic) |
Assignee | ||
Comment 14•5 years ago
|
||
(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.
Assignee | ||
Updated•5 years ago
|
Comment 15•5 years ago
|
||
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)...
Assignee | ||
Comment 16•5 years ago
|
||
(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...
Comment 17•5 years ago
|
||
Yes, I know what our existing code does. My question was what the proposed changes to that are.
Assignee | ||
Comment 18•5 years ago
|
||
(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.
Comment 19•5 years ago
|
||
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?
Assignee | ||
Comment 20•5 years ago
|
||
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.
Assignee | ||
Comment 21•5 years ago
|
||
(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.
Comment 22•5 years ago
|
||
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.
Assignee | ||
Comment 23•5 years ago
|
||
(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?
Comment 24•5 years ago
|
||
It would, for the moment, but that's also page-observable, of course. Maybe that's OK.
Comment 25•5 years ago
|
||
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)
Comment 26•4 years ago
|
||
Not a solution, but perhaps a partial workaround.
-
With links, Firefox can already show a status panel in the lower-left corner.
-
With some fake-links, Firefox can already NOT show a status panel in the lower left corner.
-
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.
Updated•2 years ago
|
Description
•