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

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Vladimir]

Attachment: 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!

Comment 1

[Andrew Overholt [:overholt]]

Olli, WDYT?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 3

[Ming-Chou Shih [:stone]]

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

Comment 4

[Ming-Chou Shih [:stone]]

Attachment: 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

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 6

[Ming-Chou Shih [:stone]]

Attachment: Instead of handling mouse up event, we handle mouse click event to trigger links.

Comment 7

[Ming-Chou Shih [:stone]]

(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?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 9

[Ming-Chou Shih [:stone]]

(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.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

up to you. Since we haven't supported explicit click dispatching before, that can be fixed in a separate bug.

Comment 11

[Ming-Chou Shih [:stone]]

Attachment: 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

Comment 12

[Ming-Chou Shih [:stone]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=80947f2c7c0ff8f1113ebfbc37c470b836b360d7

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fa0889256cd6