Crtl+clicking on a link should open it even if content is calling event.stopPropagation() in a click event handler (Port Firefox Bug 748740)

RESOLVED FIXED in seamonkey2.20

Status

SeaMonkey
Tabbed Browser
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Philip Chee, Assigned: Philip Chee)

Tracking

Trunk
seamonkey2.20

SeaMonkey Tracking Flags

(seamonkey2.19 affected, seamonkey2.20 affected)

Details

(Whiteboard: [parity-IE][parity-chrome][parity-Firefox])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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

4 years ago
Created attachment 736819 [details] [diff] [review]
Patch v1.0 use XBL handler.

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

4 years ago
Whiteboard: [parity-IE][parity-chrome][parity-Firefox]
(Assignee)

Updated

4 years ago
status-seamonkey2.19: --- → affected
status-seamonkey2.20: --- → affected

Comment 2

4 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

4 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

4 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

4 years ago
Created attachment 740086 [details] [diff] [review]
Patch v1.1 r=Neil [check-in Comment 5].

> 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

4 years ago
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/3267f9ecf52c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.20

Comment 7

4 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

4 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 8

4 years ago
Created attachment 740321 [details] [diff] [review]
Patch v2.0. fix issues raised in review.

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

4 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

4 years ago
Created attachment 741329 [details] [diff] [review]
Patch v2.1 Rename the property to onContentClick to match the attribute [check-in comment 11]

> 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

4 years ago
Attachment #741329 - Flags: review?(neil) → review+
(Assignee)

Comment 11

4 years ago
Pushed Part 2 to comm-central:
http://hg.mozilla.org/comm-central/rev/85e820e19cc8
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

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