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

RESOLVED FIXED in Firefox 22

Status

()

Firefox
General
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Grunin Yuriy, Assigned: dao)

Tracking

({testcase})

Trunk
Firefox 22
testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

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

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

5 years ago
Created attachment 618410 [details]
Reporter's testcase

Updated

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

Comment 3

5 years ago
Please, fix this stuff

Updated

5 years ago
Component: Event Handling → Tabbed Browser
Product: Core → Firefox
QA Contact: events → tabbed.browser

Comment 4

5 years ago
How can I change the status? It is definitely CONFIRMED :)

Comment 5

5 years ago
Is there some known workarounds for the bug?
(Assignee)

Updated

5 years ago
Status: UNCONFIRMED → NEW
Component: Tabbed Browser → General
Ever confirmed: true
QA Contact: tabbed.browser → general
Whiteboard: [parity-IE][parity-chrome]

Updated

5 years ago
tracking-firefox13: --- → ?
tracking-firefox14: --- → ?
tracking-firefox15: --- → ?
tracking-firefox16: --- → ?
(Assignee)

Comment 6

5 years ago
Not a recent regression, no reason to track this.
tracking-firefox13: ? → ---
tracking-firefox14: ? → ---
tracking-firefox15: ? → ---
tracking-firefox16: ? → ---
(Assignee)

Comment 7

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

Comment 8

5 years ago
Sounds sad :( Navigation in my application is fully customized and based on handling this event. Work everywhere! ))) Even in IE and Opera ;)

Updated

5 years ago
Component: Event Handling → Tabbed Browser
Product: Core → Firefox
QA Contact: events → tabbed.browser

Comment 9

5 years ago
This is all about front-end code. It can use system event group.

Comment 10

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

Comment 12

5 years ago
Ok, I can catch the event after the "stopPropagation", but how it can help me to keep the default "Ctrl+Click" functionality?
(Assignee)

Comment 13

5 years ago
Created attachment 636265 [details] [diff] [review]
patch
Assignee: nobody → dao
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Component: Tabbed Browser → General
QA Contact: tabbed.browser → general
(Assignee)

Comment 14

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

5 years ago
Any expectations when the issue will be resolved? =)
(Assignee)

Comment 16

4 years ago
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.
Attachment #636265 - Attachment is obsolete: true
Attachment #726177 - Flags: feedback?(mak77)

Comment 17

4 years ago
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.
(Assignee)

Comment 20

4 years ago
(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.
(Assignee)

Comment 21

4 years ago
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?
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+
(Assignee)

Updated

4 years ago
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+
(Assignee)

Comment 24

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb01f41e077b
https://hg.mozilla.org/mozilla-central/rev/eb01f41e077b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22

Comment 26

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