985988 - event.defaultPrevented returns different value from other browsers (IE, Chrome)

Status: VERIFIED FIXED

(Core :: DOM: Events, defect)

Description

[Alice0775 White]

Attachment: testcase

When I test Bug 985463, I fond the following discrepancy between Firefox and other browser.

<a id="a"
   onclick="return false;" 
   href="https://bugzilla.mozilla.org/">link</a>

<script>
document.getElementById("a").addEventListener("click", log, false);
function log(event) {
  alert(event.defaultPrevented);
}
</script>

Steps To Reproduce:
1. open testcase
2. Click "link"

Actual Results:
Firefox28 and later returns false.
IE11 and Crome33 return true.

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/06b3a7aea2c0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131208154802
Bad:
http://hg.mozilla.org/mozilla-central/rev/551efcc4de6f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131208185601
Puahlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=06b3a7aea2c0&tochange=551efcc4de6f


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/046cf0c4f858
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131207204100
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2aaff66026ba
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131208075200
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=046cf0c4f858&tochange=2aaff66026ba

Regressed by:
Bug 930374 - Event.defaultPrevented shouldn't become true if preventDefault() was called by our internal handler for default action

Comment 1

[Olli Pettay [:smaug]]

Uh, need to fix nsJSEventListener::HandleEvent

Comment 2

[Olli Pettay [:smaug]]

Masayuki, want to look at this?

Thanks Alice!

Comment 3

[Olli Pettay [:smaug]]

Or patch coming. Need to write some tests

Comment 4

[Olli Pettay [:smaug]]

Attachment: patch + some tests

https://tbpl.mozilla.org/?tree=Try&rev=4b0e5f0d0310

beta patch will need some renaming, since there we have still nsDOMEvent

Comment 5

[Olli Pettay [:smaug]]

Attachment: maybe this compiles on osx

https://tbpl.mozilla.org/?tree=Try&rev=5fc0aa52fb33

Comment 6

[Olli Pettay [:smaug]]

Attachment: v3

Crossing fingers this more conventional approach compiles
https://tbpl.mozilla.org/?tree=Try&rev=fe69147db0df

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: 9/21~9/29)]

I'm sorry. Today is national holiday of Japan. And I'm going to go out soon. When I get back home and I have much time then, I'll check this.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: 9/21~9/29)]

Comment on attachment 8394440 [details] [diff] [review]
v3

Although, I don't understand well the computation code of isChromeHandler, but you're the expert around here. So, I trust your code.

The other concern is, I'm not sure if GetHandler() and GetHandler().Ptr() are always non-null when HandleEvent() is called. If it's no problem, r=masayuki.

Thank you for your work!

Comment 9

[Olli Pettay [:smaug]]

HasEventHandler() checks the Ptr isn't null.

Comment 10

[Olli Pettay [:smaug]]

Attachment: up-to-date for trunk (v4)

Comment 11

[Olli Pettay [:smaug]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eda84c1e8f2f

Comment 12

[Olli Pettay [:smaug]]

Comment on attachment 8394440 [details] [diff] [review]
v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 930374
User impact if declined: bug  	985463
Testing completed (on m-c, etc.): landed to m-i
Risk to taking this patch (and alternatives if risky): Shouldn't be too risky 
String or IDL/UUID changes made by this patch: NA

Comment 13

[Olli Pettay [:smaug]]

Attachment: for beta (v3)

[Approval Request Comment]
See for request for aurora

Comment 14

[Wes Kocher (:KWierso)]

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

Comment 15

[Ekanan Ketunuti]

verified fixed on nightly 31.0a1 20140322030204.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6a3cd07007aa
https://hg.mozilla.org/releases/mozilla-beta/rev/f46bf13e9b32

Comment 17

[Ekanan Ketunuti]

verified on aurora 30.0a2 20140324150430

Comment 18

[Ekanan Ketunuti]

I still can reproduce on 29.0-beta2.
buildID: 20140324101726
changeset fceb90d30601

Clicking `link` on attachment 8394205 [details] returns false.

Comment 19

[Ekanan Ketunuti]

oops, firefox 29 beta 2 came off changeset 662d9090988b which was prior to this landing (see hg graph) :(

Comment 20

[Ekanan Ketunuti]

verified on current tinderbox mozilla-beta changeset 33d1476deae6.