Bug 62518 - If an Event Listener is a Function, It Should be Called and Not Checked for handleEvent
: If an Event Listener is a Function, It Should be Called and Not Checked for h...
Status: RESOLVED FIXED
: WebKit
WebCore JavaScript
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-06-12 15:03 PST by
Modified: 2011-11-21 18:59 PST (History)


Attachments
Layout Test for Event Listener Functions that Implement the EventListener Interface (4.39 KB, patch)
2011-06-12 15:03 PST, Rob Brackett
no flags Review Patch | Details | Formatted Diff | Diff
Patch for Event Listener Functions that Implement the EventListener Interface (2.42 KB, patch)
2011-06-12 15:06 PST, Rob Brackett
no flags Review Patch | Details | Formatted Diff | Diff
[PATCH] Improved Fix for this Issue (8.96 KB, patch)
2011-06-12 23:27 PST, Rob Brackett
no flags Review Patch | Details | Formatted Diff | Diff
[PATCH] Improved Patch Following Conventions (10.10 KB, patch)
2011-06-12 23:45 PST, Rob Brackett
sam: review-
Review Patch | Details | Formatted Diff | Diff
Patch with tests in fast/ and that addresses JSCallbacks (9.89 KB, patch)
2011-11-14 00:11 PST, Rob Brackett
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-06-12 15:03:48 PST
Created an attachment (id=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.
------- Comment #1 From 2011-06-12 15:06:22 PST -------
Created an attachment (id=96893) [details]
Patch for Event Listener Functions that Implement the EventListener Interface

Adding a patch for this issue.
------- Comment #2 From 2011-06-12 15:25:32 PST -------
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()!
------- Comment #3 From 2011-06-12 18:29:36 PST -------
(From update of attachment 96893 [details])
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.
------- Comment #4 From 2011-06-12 23:27:34 PST -------
Created an attachment (id=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.
------- Comment #5 From 2011-06-12 23:45:17 PST -------
Created an attachment (id=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.
------- Comment #6 From 2011-06-13 00:02:51 PST -------
(From update of attachment 96926 [details])
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?
------- Comment #7 From 2011-06-13 17:29:11 PST -------
(From update of attachment 96926 [details])
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.
------- Comment #8 From 2011-11-14 00:11:39 PST -------
Created an attachment (id=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).
------- Comment #9 From 2011-11-21 12:45:28 PST -------
This patch has been sitting for a week. Is anybody interested in reviewing it? Thanks!
------- Comment #10 From 2011-11-21 18:59:18 PST -------
(From update of attachment 114894 [details])
Clearing flags on attachment: 114894

Committed r100974: <http://trac.webkit.org/changeset/100974>
------- Comment #11 From 2011-11-21 18:59:24 PST -------
All reviewed patches have been landed.  Closing bug.