Closed
Bug 861194
Opened 12 years ago
Closed 12 years ago
Crtl+clicking on a link should open it even if content is calling event.stopPropagation() in a click event handler (Port Firefox Bug 748740)
Categories
(SeaMonkey :: Tabbed Browser, defect)
SeaMonkey
Tabbed Browser
Tracking
(seamonkey2.19 affected, seamonkey2.20 affected)
RESOLVED
FIXED
seamonkey2.20
People
(Reporter: philip.chee, Assigned: philip.chee)
Details
(Whiteboard: [parity-IE][parity-chrome][parity-Firefox])
Attachments
(2 files, 2 obsolete files)
|
3.98 KB,
patch
|
philip.chee
:
review+
|
Details | Diff | Splinter Review |
|
2.44 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
q.v. FX Bug 748740 - New tab is not opening after "Ctrl/Cmd+Click" on a link if there is "event.stopPropagation()" in the content "click" handler
> User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:11.0) Gecko/20100101
> Firefox/11.0
> Build ID: 20120312181643
>
> Steps to reproduce:
>
> 1) Make test page with one link in it (<a href="http://google.com">Test
> link</a>)
> 2) Attach click handler to this link that stops propagation of the event:
> elem.addEventListener('click', function(e) {
> e.stopPropagation();
> }, false);
> 2) "Ctrl/Cmd+Click" on this link.
>
>
> Actual results:
>
> Nothing happens.
>
>
> Expected results:
>
> A new tab should be opened with "http://google.com" in it, because there is no
> "e.preventDefault()" in event handler.
> Middle-button click works just fine.
|
Assignee | |
Comment 1•12 years ago
|
||
Test Case:
https://bug748740.bugzilla.mozilla.org/attachment.cgi?id=618410
In Firefox Bug 748740 they used addSystemEventListener() from their browser.js:
https://hg.mozilla.org/mozilla-central/rev/eb01f41e077b
I thought I'd try using a XBL handler instead and it seems to work. I'm not sure which approach is better.
Attachment #736819 -
Flags: review?(neil)
|
|
Assignee | |
Updated•12 years ago
|
Whiteboard: [parity-IE][parity-chrome][parity-Firefox]
|
|
Assignee | |
Updated•12 years ago
|
status-seamonkey2.19:
--- → affected
status-seamonkey2.20:
--- → affected
|
||
Comment 2•12 years ago
|
||
Comment on attachment 736819 [details] [diff] [review]
Patch v1.0 use XBL handler.
>+ this.contentAreaClick.call(this, event);
This is the same as this.contentAreaClick(event) - did you mean to use window or null instead?
>+ ]]>
>+ </handler>
>+
Nit: unnecessary blank line.
|
|
||
Comment 3•12 years ago
|
||
Comment on attachment 736819 [details] [diff] [review]
Patch v1.0 use XBL handler.
>+ this.contentAreaClick.call(this, event);
window it is then, since that's strict-safe. r=me with those nits fixed.
Attachment #736819 -
Flags: review?(neil) → review+
|
|
||
Comment 4•12 years ago
|
||
(In reply to comment #3)
> (From update of attachment 736819 [details] [diff] [review])
> >+ this.contentAreaClick.call(this, event);
> window it is then, since that's strict-safe. r=me with those nits fixed.
Wait, I've confused myself here.
You're calling the JavaScript snippet "return contentAreaClick(event);" here, which *does* run in the context of the element itself. So this.contentAreaClick(event); is correct.
|
|
Assignee | |
Comment 5•12 years ago
|
||
> Nit: unnecessary blank line.
Fixed.
> You're calling the JavaScript snippet "return contentAreaClick(event);" here,
> which *does* run in the context of the element itself. So
> this.contentAreaClick(event); is correct.
Fixed.
Attachment #736819 -
Attachment is obsolete: true
Attachment #740086 -
Flags: review+
|
|
Assignee | |
Comment 6•12 years ago
|
||
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/3267f9ecf52c
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.20
|
|
||
Comment 7•12 years ago
|
||
(In reply to comment #4)
> You're calling the JavaScript snippet "return contentAreaClick(event);"
> here, which *does* run in the context of the element itself. So
> this.contentAreaClick(event); is correct.
Well, less incorrect.
If you wrote <body onclick="alert(onclick == this.onclick);"> then that will actually search for the onclick function as a property of the body (even though it doesn't say this.onclick) and therefore alert "true".
Fortunately we can't scope contentAreaClick to work in the same way, so it finds window.contentAreaClick, which is what we want, but maybe we should rename our property to oncontentareaclick to be less confusing...
|
|
Assignee | |
Updated•12 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
|
|
Assignee | |
Comment 8•12 years ago
|
||
>> You're calling the JavaScript snippet "return contentAreaClick(event);"
>> here, which *does* run in the context of the element itself. So
>> this.contentAreaClick(event); is correct.
> Well, less incorrect.
> If you wrote <body onclick="alert(onclick == this.onclick);"> then that will
> actually search for the onclick function as a property of the body (even
> though it doesn't say this.onclick) and therefore alert "true".
> Fortunately we can't scope contentAreaClick to work in the same way, so it
> finds window.contentAreaClick, which is what we want, but maybe we should
> rename our property to oncontentareaclick to be less confusing...
This patch renames the property to onContentAreaClick and calls it with "window" as the scope.
Attachment #740321 -
Flags: review?(neil)
|
|
||
Comment 9•12 years ago
|
||
Comment on attachment 740321 [details] [diff] [review]
Patch v2.0. fix issues raised in review.
No, I don't want to change the "this" object (as opposed to the scope, but we can't change that.)
If you still want to change the name of the property, then it should of course be something like "onContentClick" to match the attribute. Sorry for the confusion.
Attachment #740321 -
Flags: review?(neil) → review-
|
|
Assignee | |
Comment 10•12 years ago
|
||
> If you still want to change the name of the property, then it should of course
> be something like "onContentClick" to match the attribute. Sorry for the
> confusion.
Property renamed.
Attachment #740321 -
Attachment is obsolete: true
Attachment #741329 -
Flags: review?(neil)
|
|
||
Updated•12 years ago
|
Attachment #741329 -
Flags: review?(neil) → review+
|
|
Assignee | |
Comment 11•12 years ago
|
||
Pushed Part 2 to comm-central:
http://hg.mozilla.org/comm-central/rev/85e820e19cc8
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
|
|
Assignee | |
Updated•12 years ago
|
Attachment #741329 -
Attachment description: Patch v2.1 Rename the property to onContentClick to match the attribute → Patch v2.1 Rename the property to onContentClick to match the attribute [check-in comment 11]
You need to log in
before you can comment on or make changes to this bug.
Description
•