Last Comment Bug 861194 - Crtl+clicking on a link should open it even if content is calling event.stopPropagation() in a click event handler (Port Firefox Bug 748740)
: Crtl+clicking on a link should open it even if content is calling event.stopP...
Status: RESOLVED FIXED
[parity-IE][parity-chrome][parity-Fir...
:
Product: SeaMonkey
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.20
Assigned To: Philip Chee
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-12 07:44 PDT by Philip Chee
Modified: 2013-04-27 21:15 PDT (History)
0 users
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
affected
affected


Attachments
Patch v1.0 use XBL handler. (4.50 KB, patch)
2013-04-12 07:51 PDT, Philip Chee
neil: review+
Details | Diff | Splinter Review
Patch v1.1 r=Neil [check-in Comment 5]. (3.98 KB, patch)
2013-04-21 06:24 PDT, Philip Chee
philip.chee: review+
Details | Diff | Splinter Review
Patch v2.0. fix issues raised in review. (2.47 KB, patch)
2013-04-22 09:29 PDT, Philip Chee
neil: review-
Details | Diff | Splinter Review
Patch v2.1 Rename the property to onContentClick to match the attribute [check-in comment 11] (2.44 KB, patch)
2013-04-24 08:05 PDT, Philip Chee
neil: review+
Details | Diff | Splinter Review

Description Philip Chee 2013-04-12 07:44:36 PDT
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.
Comment 1 Philip Chee 2013-04-12 07:51:28 PDT
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.
Comment 2 neil@parkwaycc.co.uk 2013-04-20 17:02:40 PDT
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 neil@parkwaycc.co.uk 2013-04-21 03:12:06 PDT
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.
Comment 4 neil@parkwaycc.co.uk 2013-04-21 03:26:35 PDT
(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.
Comment 5 Philip Chee 2013-04-21 06:24:08 PDT
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.
Comment 6 Philip Chee 2013-04-21 06:26:06 PDT
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/3267f9ecf52c
Comment 7 neil@parkwaycc.co.uk 2013-04-21 13:53:52 PDT
(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...
Comment 8 Philip Chee 2013-04-22 09:29:17 PDT
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.
Comment 9 neil@parkwaycc.co.uk 2013-04-22 09:52:38 PDT
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.
Comment 10 Philip Chee 2013-04-24 08:05:43 PDT
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.
Comment 11 Philip Chee 2013-04-27 21:14:10 PDT
Pushed Part 2 to comm-central:
http://hg.mozilla.org/comm-central/rev/85e820e19cc8

Note You need to log in before you can comment on or make changes to this bug.