Event preventDefault in JavaScript doesn't work if there is an image with attribute "ismap" inside link

RESOLVED FIXED in Firefox 50

Status

()

Core
DOM: Events
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Vladimir, Assigned: stone)

Tracking

(Blocks: 1 bug)

38 Branch
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: btpp-active, [tw-dom])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8736299 [details]
server-map.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20151027170520

Steps to reproduce:

I wrote an example of how working "ismap" attribute. JavaScript should stop following by URL and display coordinates of the image after "onclick" event.


Actual results:

Method "event.preventDefault();" didn't stop following. If I remove attribute "ismap" from the tag "img" then I works correctly.


Expected results:

Method "event.preventDefault();" should work!

Updated

2 years ago
Component: Untriaged → JavaScript Engine
Product: Firefox → Core

Updated

2 years ago
Component: JavaScript Engine → DOM: Events
Olli, WDYT?
Flags: needinfo?(bugs)
Whiteboard: btpp-followup-2016-04-29
If I read the code right, this is some truly ancient code in http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsImageFrame.cpp?rev=d155d4cc4312#1944
From commits:
Sat Jan 9 00:13:19 1999 UTC (17 years, 3 months ago) 
http://52.25.115.98/viewvc/main/mozilla/layout/generic/nsImageFrame.cpp?revision=1.44&view=markup
and
Tue Sep 8 22:34:39 1998 UTC (17 years, 7 months ago)
http://52.25.115.98/viewvc/main/mozilla/layout/generic/nsImageFrame.cpp?annotate=1.1

Nothing in that code seems to check if preventDefault() was called.

The feature is clearly very very rarely used, but this looks like a valid bug. No idea what other browsers do though.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(bugs)
Whiteboard: btpp-followup-2016-04-29 → btpp-followup-2016-04-29, [tw-dom]
Whiteboard: btpp-followup-2016-04-29, [tw-dom] → btpp-backlog, [tw-dom]
(Assignee)

Updated

2 years ago
Assignee: nobody → sshih

Updated

2 years ago
Whiteboard: btpp-backlog, [tw-dom] → btpp-active, [tw-dom]
(Assignee)

Comment 3

2 years ago
Edge, Safari, and Chrome do handle preventDefault in this test case.

Looks like there are two causes of this bug.
1. nsImageFrame doesn't handle preventDefault
2. nsImageFrame triggers link when receiving 'mouseup' event while the test case invoke preventDefault in the event listener to 'click' event
(Assignee)

Comment 4

2 years ago
Created attachment 8765752 [details] [diff] [review]
Instead of handling mouse up event, we handle mouse click event to trigger links

1.Set mMultipleActionsPrevented in HTMLImageElement::PreHandleEvent to prevent click event is handled more than once
2.Instead of handling mouse up event, we handle mouse click event in nsImageFrame::HandleEvent to trigger links
Attachment #8765752 - Flags: review?(bugs)
Comment on attachment 8765752 [details] [diff] [review]
Instead of handling mouse up event, we handle mouse click event to trigger links


> HTMLImageElement::PreHandleEvent(EventChainPreVisitor& aVisitor)
> {
>-  // If we are a map and get a mouse click, don't let it be handled by
>-  // the Generic Element as this could cause a click event to fire
>-  // twice, once by the image frame for the map and once by the Anchor
>-  // element. (bug 39723)
>+  // We handle image element with attribute ismap in its corresponding frame
>+  // element. Set mMultipleActionsPrevented here to prevent the click event
>+  // trigger the behaviors in Element::PostHandleEventForLinks
>   WidgetMouseEvent* mouseEvent = aVisitor.mEvent->AsMouseEvent();
>-  if (mouseEvent && mouseEvent->IsLeftClickEvent() && IsMap()) {
>-    aVisitor.mEventStatus = nsEventStatus_eConsumeNoDefault;
>+  if (mouseEvent && aVisitor.mEvent->mMessage == eMouseClick &&
>+      mouseEvent->IsLeftClickEvent() && IsMap()) {
IsLeftClickEvent() is enough, no need to have mMessage == eMouseClick

Not really about this bug, but related, does dispatching click event from script trigger the ismap
link on other browsers? I mean using 'new MouseEvent("click", ...)'
If so, we may need to move the relevant event handling from ns*Frame implementation to HTMLImageElement.




>+      ok(params[testIdx]["map"] == (url.substring(url.length - 4, url.length) != "html"), url)
This is super hard to understand. url should be always bug1260704_iframe_empty.html here (since that is where the message event is coming from), but we're doing some odd looking
substring comparisong with "html" and comparing whether it matches with 'map' boolean property.
Could you clarify this a bit.

>+</div>
>+<iframe id="test_iframe" src="bug1260704_iframe_empty.html" width="400" height="400">
>+</iframe>
this all might be a bit easier to read if this started by loading about:blank to the iframe
Attachment #8765752 - Flags: review?(bugs) → review+
(Assignee)

Comment 6

2 years ago
Created attachment 8766186 [details] [diff] [review]
Instead of handling mouse up event, we handle mouse click event to trigger links.
Attachment #8765752 - Attachment is obsolete: true
(Assignee)

Comment 7

2 years ago
(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #5)
> Not really about this bug, but related, does dispatching click event from
> script trigger the ismap
> link on other browsers? I mean using 'new MouseEvent("click", ...)'
> If so, we may need to move the relevant event handling from ns*Frame
> implementation to HTMLImageElement.
Click event from script do trigger link on Edge and Chrome but doesn't on Safari. Should we follow Chrome and Edge?

> This is super hard to understand. url should be always
> bug1260704_iframe_empty.html here (since that is where the message event is
> coming from), but we're doing some odd looking
> substring comparisong with "html" and comparing whether it matches with
> 'map' boolean property.
> Could you clarify this a bit.
Sorry for that. It's try to make sure that the redirected url contains coordinates. I refined my patch and would you mind to review it if it's better to understand?
(In reply to Ming-Chou Shih [:stone] from comment #7)
> (In reply to Olli Pettay [:smaug] (high review load, please consider other
> reviewers) from comment #5)
> > Not really about this bug, but related, does dispatching click event from
> > script trigger the ismap
> > link on other browsers? I mean using 'new MouseEvent("click", ...)'
> > If so, we may need to move the relevant event handling from ns*Frame
> > implementation to HTMLImageElement.
> Click event from script do trigger link on Edge and Chrome but doesn't on
> Safari. Should we follow Chrome and Edge?
I think we should, given that firing click event explicitly does usually trigger links.
Like with <a> element.


> Sorry for that. It's try to make sure that the redirected url contains
> coordinates. I refined my patch and would you mind to review it if it's
> better to understand?
Ahaa, it was about coordinates! I totally missed that.
But r+ for the change in the test.
(Assignee)

Comment 9

2 years ago
(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #8)
> (In reply to Ming-Chou Shih [:stone] from comment #7)
> > (In reply to Olli Pettay [:smaug] (high review load, please consider other
> > reviewers) from comment #5)
> > > Not really about this bug, but related, does dispatching click event from
> > > script trigger the ismap
> > > link on other browsers? I mean using 'new MouseEvent("click", ...)'
> > > If so, we may need to move the relevant event handling from ns*Frame
> > > implementation to HTMLImageElement.
> > Click event from script do trigger link on Edge and Chrome but doesn't on
> > Safari. Should we follow Chrome and Edge?
> I think we should, given that firing click event explicitly does usually
> trigger links.
> Like with <a> element.
Should I submit this patch first or merge it with the patch to move the relevant event handling to HTMLImageElement? I quickly trace related implementation and looks like the implementation is bound to nsXXX stuffs (such as nsImageMap). I have to figure it out first.
up to you. Since we haven't supported explicit click dispatching before, that can be fixed in a separate bug.
(Assignee)

Updated

2 years ago
Blocks: 1283347
(Assignee)

Comment 11

2 years ago
Created attachment 8766672 [details] [diff] [review]
Instead of handling mouse up event, we handle mouse click event to trigger links. r=smaug

Follow reviewer's comments to update the patch and update patch summary
Attachment #8766186 - Attachment is obsolete: true
Attachment #8766672 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 13

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa0889256cd6
Instead of handling mouse up event, we handle mouse click event to trigger links. r=smaug
Keywords: checkin-needed

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fa0889256cd6
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1285082
You need to log in before you can comment on or make changes to this bug.