Closed Bug 748740 Opened 8 years ago Closed 8 years ago

New tab is not opening after "Ctrl/Cmd+Click" on a link if there is "event.stopPropagation()" in the "click" handler

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 22

People

(Reporter: grunin.ya, Assigned: dao)

Details

(Keywords: testcase, Whiteboard: [parity-IE][parity-chrome])

Attachments

(2 files, 2 obsolete files)

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.
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.
Attached file Reporter's testcase
Attachment #618410 - Attachment mime type: text/plain → text/html
Component: Untriaged → Event Handling
Keywords: testcase
OS: Mac OS X → All
Product: Firefox → Core
QA Contact: untriaged → events
Hardware: x86 → All
Version: 11 Branch → Trunk
Please, fix this stuff
Component: Event Handling → Tabbed Browser
Product: Core → Firefox
QA Contact: events → tabbed.browser
How can I change the status? It is definitely CONFIRMED :)
Is there some known workarounds for the bug?
Status: UNCONFIRMED → NEW
Component: Tabbed Browser → General
Ever confirmed: true
QA Contact: tabbed.browser → general
Whiteboard: [parity-IE][parity-chrome]
Not a recent regression, no reason to track this.
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.
Component: General → Event Handling
Product: Firefox → Core
QA Contact: general → events
Sounds sad :( Navigation in my application is fully customized and based on handling this event. Work everywhere! ))) Even in IE and Opera ;)
Component: Event Handling → Tabbed Browser
Product: Core → Firefox
QA Contact: events → tabbed.browser
This is all about front-end code. It can use system event group.
Googling on "system event group" gave me no meaningful results. Could you provide me with some code snippet or other tip?
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
Ok, I can catch the event after the "stopPropagation", but how it can help me to keep the default "Ctrl+Click" functionality?
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → dao
Status: NEW → ASSIGNED
Component: Tabbed Browser → General
QA Contact: tabbed.browser → general
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
Any expectations when the issue will be resolved? =)
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #636265 - Attachment is obsolete: true
Attachment #726177 - Flags: feedback?(mak77)
Your answer didn't lit light on the issue at all for me ))
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?
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.
(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.
Attached patch patch v2Splinter Review
(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?
Attachment #726177 - Attachment is obsolete: true
Attachment #726177 - Flags: feedback?(mak77)
Attachment #726574 - Flags: feedback?(bugs)
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.
Attachment #726574 - Flags: feedback?(bugs) → feedback+
Attachment #726574 - Flags: review?(mak77)
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
Attachment #726574 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/eb01f41e077b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Resolved? Seriously? You are not kidding me? =) Thanks a lot guys! I'm awaiting next FF release as never!
You need to log in before you can comment on or make changes to this bug.