Last Comment Bug 748740 - New tab is not opening after "Ctrl/Cmd+Click" on a link if there is "event.stopPropagation()" in the "click" handler
: New tab is not opening after "Ctrl/Cmd+Click" on a link if there is "event.st...
Status: RESOLVED FIXED
[parity-IE][parity-chrome]
: testcase
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: Firefox 22
Assigned To: Dão Gottwald [:dao]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-25 06:07 PDT by Grunin Yuriy
Modified: 2013-03-21 11:16 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Reporter's testcase (177 bytes, text/html)
2012-04-25 13:10 PDT, mjh563
no flags Details
patch (2.28 KB, patch)
2012-06-25 03:47 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Review
patch (2.21 KB, patch)
2013-03-18 08:10 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Review
patch v2 (2.21 KB, patch)
2013-03-19 04:19 PDT, Dão Gottwald [:dao]
mak77: review+
bugs: feedback+
Details | Diff | Review

Description Grunin Yuriy 2012-04-25 06:07:10 PDT
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 mjh563 2012-04-25 13:03:22 PDT
Confirmed on Mozilla/5.0 (X11; Linux i686; rv:12.0) Gecko/20100101 Firefox/12.0

Middle click works OK
Context menu "open link in new tab" works OK
Ctrl-click does nothing.
Comment 2 mjh563 2012-04-25 13:10:00 PDT
Created attachment 618410 [details]
Reporter's testcase
Comment 3 pavel.shkleinik 2012-06-22 13:50:07 PDT
Please, fix this stuff
Comment 4 pavel.shkleinik 2012-06-23 05:14:01 PDT
How can I change the status? It is definitely CONFIRMED :)
Comment 5 pavel.shkleinik 2012-06-24 03:58:08 PDT
Is there some known workarounds for the bug?
Comment 6 Dão Gottwald [:dao] 2012-06-24 10:34:49 PDT
Not a recent regression, no reason to track this.
Comment 7 Dão Gottwald [:dao] 2012-06-24 16:00:26 PDT
It seems that front-end code can't do much about this. It could use a capturing click event listener, but then it wouldn't honor event.defaultPrevented.
Comment 8 pavel.shkleinik 2012-06-25 01:47:01 PDT
Sounds sad :( Navigation in my application is fully customized and based on handling this event. Work everywhere! ))) Even in IE and Opera ;)
Comment 9 Olli Pettay [:smaug] 2012-06-25 02:30:21 PDT
This is all about front-end code. It can use system event group.
Comment 10 pavel.shkleinik 2012-06-25 03:24:52 PDT
Googling on "system event group" gave me no meaningful results. Could you provide me with some code snippet or other tip?
Comment 11 Olli Pettay [:smaug] 2012-06-25 03:36:05 PDT
Firefox UI code can use system event group, not a web page.
As an example 
http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChild.js?mark=120-127#119
Comment 12 pavel.shkleinik 2012-06-25 03:46:28 PDT
Ok, I can catch the event after the "stopPropagation", but how it can help me to keep the default "Ctrl+Click" functionality?
Comment 13 Dão Gottwald [:dao] 2012-06-25 03:47:16 PDT
Created attachment 636265 [details] [diff] [review]
patch
Comment 14 Dão Gottwald [:dao] 2012-06-25 05:59:38 PDT
I get this failure with that patch:
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_markPageAsFollowedLink.js | User activated visits should get a FRAMED_LINK transition. - Got null, expected 8
Comment 15 pavel.shkleinik 2012-07-05 23:29:44 PDT
Any expectations when the issue will be resolved? =)
Comment 16 Dão Gottwald [:dao] 2013-03-18 08:10:44 PDT
Created attachment 726177 [details] [diff] [review]
patch

I don't know what to do about the browser_markPageAsFollowedLink.js failure. It seems that places history requires markPageAsFollowedLink to be called at a certain (wrong, by this bug's measure) point in time during the click event propagation.
Comment 17 pavel.shkleinik 2013-03-18 14:47:33 PDT
Your answer didn't lit light on the issue at all for me ))
Comment 18 Marco Bonardo [::mak] 2013-03-19 01:09:00 PDT
Places just collects calls to markPageAsFollowedLink and applies the last followed flag to the next visit.
so markPageAsFollowedLink -> stored into an hash -> AddVisit uses the info
There is no specific requirement about when it happens, could be the test is too picky for when it checks?
Comment 19 Marco Bonardo [::mak] 2013-03-19 01:15:22 PDT
Did you verify, by chance, if we still reach the markPageAsFollowedLink call with the change? I wonder if some event change causes us to bail out earlier.
Comment 20 Dão Gottwald [:dao] 2013-03-19 04:03:04 PDT
(In reply to Marco Bonardo [:mak] from comment #19)
> Did you verify, by chance, if we still reach the markPageAsFollowedLink call
> with the change? I wonder if some event change causes us to bail out earlier.

No I didn't! Apparently event.defaultPrevented is true, causing contentAreaClick to return early. Thanks for the hint.
Comment 21 Dão Gottwald [:dao] 2013-03-19 04:19:45 PDT
Created attachment 726574 [details] [diff] [review]
patch v2

(In reply to Dão Gottwald [:dao] from comment #20)
> Apparently event.defaultPrevented is true, causing contentAreaClick to return early.

Making the event listener a capturing one makes browser_markPageAsFollowedLink.js pass. I assume core code was consuming the event before us. Does this make sense?
Comment 22 Olli Pettay [:smaug] 2013-03-19 05:58:33 PDT
Comment on attachment 726574 [details] [diff] [review]
patch v2

Hmm, using system group doesn't affect to preventDefault() handling, but
stopPropagation() is per group, so yes, this makes sense to me.
Comment 23 Marco Bonardo [::mak] 2013-03-20 01:41:28 PDT
Comment on attachment 726574 [details] [diff] [review]
patch v2

Review of attachment 726574 [details] [diff] [review]:
-----------------------------------------------------------------

sort of scary, since involves all clicks in browser, but easy to backout in case
Comment 25 Ed Morley [:emorley] 2013-03-21 06:07:34 PDT
https://hg.mozilla.org/mozilla-central/rev/eb01f41e077b
Comment 26 pavel.shkleinik 2013-03-21 11:16:14 PDT
Resolved? Seriously? You are not kidding me? =) Thanks a lot guys! I'm awaiting next FF release as never!

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