Bug 62518 – If an Event Listener is a Function, It Should be Called and Not Checked for handleEvent

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Rob Brackett]

Created attachment 96892 [details]
Layout Test for Event Listener Functions that Implement the EventListener Interface

When the listener passed to addEventListener() is a function, it should be called when the event occurs. The listener function should not be tested for adherence to the EventListener interface and have its handleEvent() method called if present.

The DOM Level 2 Events spec does not address this issue (in the ECMAScript interface, it only allows functions as listeners). The DOM Level 3 Events spec, though not yet a working draft, appears to indicate that the above described behavior is correct, though it is not extraordinarily clear. It says,

“The listener parameter must be either an object that implements the EventListener interface, or a function. If listener is a function then it must be used as the callback for the event; otherwise, if listener implements EventTarget, then its handleEvent method must be used as the callback.” (see the spec at http://www.w3.org/TR/2011/WD-DOM-Level-3-Events-20110531/#events-EventTarget-addEventListener)

In any case, all other major browsers (Firefox, Opera, and even Chrome) implement it this way, so standard WebKit with JavaScriptCore should probably strive for parity here.

As a note, I think the current behavior is much nicer than this, but interoperability is probably more important.

[Rob Brackett]

Created attachment 96893 [details]
Patch for Event Listener Functions that Implement the EventListener Interface

Adding a patch for this issue.

[Rob Brackett]

For some context, this issue was found while using Chrome to test some JS that had only previously been run in Safari. 

A JS "class" (which is actually a function) had handleEvent() implemented as a "class method" (a property on the function itself instead of its prototype) that would auto-construct instances of the class based on data found in the DOM. This worked great in Safari, but as soon as the code was run in any other browser, the constructor function would run instead of handleEvent()!

[Dominic Cooney]

Comment on attachment 96893 [details]
Patch for Event Listener Functions that Implement the EventListener Interface

View in context: https://bugs.webkit.org/attachment.cgi?id=96893&action=review

While I am not a reviewer, it would be nice if this patch included a ChangeLog and a layout test.

I think the layout test in a previous attachment could be much more succinct if it briefly (one sentence) described what it was testing and used the JS test framework many other layout tests use.

> Source/WebCore/bindings/js/JSEventListener.cpp:100
> +    CallType callType = jsFunction->getCallData(callData);

Is it safe to use this getCallData here, c.f. the two-argument helper in the original? I think that the checks are slightly different.

[Rob Brackett]

Created attachment 96923 [details]
[PATCH] Improved Fix for this Issue

Adding an improved patch.

I've switched to using the two-argument getCallData() helper in the change. The previous style seemed to be safe (it looks like all objects implement getCallData and jsFunction is guaranteed to be an object), but this certainly seems more foolproof.

For the layout test, I've removed a lot of the descriptive text that is specific to this bug but not necessarily to the test itself. I've also pulled all the setup and helpers out into separate files. Hopefully between these two, that should make the test more succinct and understandable.

The layout test doesn't behave exactly the same as the other DOM tests; I don't know what the right conventions are here since the "framework" appears to differ drastically across the various layout tests. Additionally, I was unsure whether I should use the selfhtml.js since that seems to be part of the official W3C tests, which this test is not. (As a side note, is this the right place for this test?)

I've also added a changelog. I haven't ever submitted a patch to WebKit before, so hopefully the formatting is correct.

[Rob Brackett]

Created attachment 96926 [details]
[PATCH] Improved Patch Following Conventions

I apologize for the churn. I read through the contributing guide and used the appropriate scripts to prepare the patch, so hopefully this should be more correct.

[Dominic Cooney]

Comment on attachment 96926 [details]
[PATCH] Improved Patch Following Conventions

View in context: https://bugs.webkit.org/attachment.cgi?id=96926&action=review

Again, I am not a reviewer and you will need to find a review for this. However here are some personal opinions about your patch.

> Source/WebCore/ChangeLog:5
> +        If an Event Listener is a Function, It Should be Called and Not Checked for handleEvent

Don't use title case here.

> Source/WebCore/ChangeLog:8
> +        If an event listener added via addEventListener() is a function, that 

You can probably delete this since it doesn't add much that isn't in line 5.

> LayoutTests/ChangeLog:12
> +        * dom/html/level3: Added.

I believe the tests under dom/html/level* are the W3C's published test suites. So you probably don't want to put this here. What about something under fast/events?

[Sam Weinig]

Comment on attachment 96926 [details]
[PATCH] Improved Patch Following Conventions

View in context: https://bugs.webkit.org/attachment.cgi?id=96926&action=review

I think we should probably audit all the other places where we handle the "handleEvent" case and ensure that we are being consistent.

>> LayoutTests/ChangeLog:12
>> +        * dom/html/level3: Added.
> 
> I believe the tests under dom/html/level* are the W3C's published test suites. So you probably don't want to put this here. What about something under fast/events?

Dominic is right, you should create a new test under fast/events.

[Rob Brackett]

Created attachment 114894 [details]
Patch with tests in fast/ and that addresses JSCallbacks

This patch moves the test to fast/events. It also simplifies the test further. I've also looked through the codebase to find any other uses of handleEvent. The only one seems to be in JSCallbackData, so I have included changes for that as well (and a test in fast/js).

[Rob Brackett]

This patch has been sitting for a week. Is anybody interested in reviewing it? Thanks!

[WebKit Review Bot]

Comment on attachment 114894 [details]
Patch with tests in fast/ and that addresses JSCallbacks

Clearing flags on attachment: 114894

Committed r100974: <http://trac.webkit.org/changeset/100974>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.