Bug 9181 – Complete DOMUIEvent Obj-C API to reflect UIEvent

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Jonas Witt]

DOMUIEvent does not allow full access to the underlying UIEvent's properties (most important, charCode()). I've added these accessors. Patch follows.

[Jonas Witt]

Created attachment 8616 [details]
Patch to add accessors.

[Jonas Witt]

Created attachment 8620 [details]
Patch to add DOMUIEventWithKeyState, DOMKeyboardEvent, DOMMouseRelatedEvent

The whole WebKit event API (subclasses of DOMEvent) seems to be out of date. This patch should put everything that's a subclass of DOMUIEvent roughly in sync with their C++ equivalents.

The classes ClipboardEvent, BeforeUnloadEvent and Clipboard remain missing from the ObjC API.

[Geoffrey Garen]

Comment on attachment 8620 [details]
Patch to add DOMUIEventWithKeyState, DOMKeyboardEvent, DOMMouseRelatedEvent

Patch looks great.

A few things you need before landing:
1) Please add your copyright info to the files you've modified
2) Please add a ChangeLog entry (the prepare-ChangeLog script will help you).
3) How did you test? Can you add testing functionality to DumpRenderTree?

[Darin Adler]

I'm not entirely sure about this patch. Currently our Objective-C bindings for the DOM cover only what's in the DOM specification and extensions are put into separate categories/files.

This patch would be the first one to include all sorts of non-standard methods in the Objective-C bindings. While I think we want to go that way eventually, I think perhaps the way to do that would be to start generating the Objective-C bindings rather than expanding them by hand.

Also, since this is public API it needs to go through an API review process before it can be submitted on the Apple side, so we may have to figure out how to handle that.

[Jonas Witt]

Much of what I added is actually part of the DOM Level 3 Events specs. I'm all for standards, so maybe the goal could be to have an API that covers the DOM {2,3} Events specs completely (and nothing else)?

It is probably also unnecessary to expose (DOM)UIEventWithKeyState in the API - it's used in the C++ implementation, but if we want to model the API after the DOM specs, then it would be confusing.

Regarding generating the API: The C++ implementation does not closely follow the specs, so generating the ObjC API from that would expose unnecessary classes like UIEventWithKeyState. Writing the ObjC wrapper is pretty straightforward and it won't need updating very often, so I think it would be OK to do that manually.

[Darin Adler]

(In reply to comment #5)
> Much of what I added is actually part of the DOM Level 3 Events specs. I'm all
> for standards, so maybe the goal could be to have an API that covers the DOM
> {2,3} Events specs completely (and nothing else)?

Sounds OK.

> It is probably also unnecessary to expose (DOM)UIEventWithKeyState in the API -
> it's used in the C++ implementation, but if we want to model the API after the
> DOM specs, then it would be confusing.

The way this works is that we use .idl files to define what we want to expose publicly. This doesn't have to exactly match our C++.

> Regarding generating the API: The C++ implementation does not closely follow
> the specs, so generating the ObjC API from that would expose unnecessary
> classes like UIEventWithKeyState. Writing the ObjC wrapper is pretty
> straightforward and it won't need updating very often, so I think it would be
> OK to do that manually.
> 

(In reply to comment #5)
> Much of what I added is actually part of the DOM Level 3 Events specs. I'm all
> for standards, so maybe the goal could be to have an API that covers the DOM
> {2,3} Events specs completely (and nothing else)?
> 
> It is probably also unnecessary to expose (DOM)UIEventWithKeyState in the API -
> it's used in the C++ implementation, but if we want to model the API after the
> DOM specs, then it would be confusing.
> 
> Regarding generating the API: The C++ implementation does not closely follow
> the specs, so generating the ObjC API from that would expose unnecessary
> classes like UIEventWithKeyState. Writing the ObjC wrapper is pretty
> straightforward and it won't need updating very often, so I think it would be
> OK to do that manually.

As I said, we do this from .idl files, not the C++ code. We're currently using this for the JavaScript bindings.

[Jonas Witt]

Created attachment 8639 [details]
Patch to add DOMKeyboardEvent, DOMWheelEvent

This patch adds DOMKeyboardEvent (DOM Level 3 Events-compliant). It also adds DOMWheelEvent as a non-standard event in a seperate file and two non-standard methods of DOMKeyboardEvent (keyCode and charCode). I've also added the copyright notice and Changelog entries, as Geoffrey said. I've removed the classes from the second patch that don't need to be exposed.

Regarding testing: I've done testing of all properties using a simple WebView + test HTML file, an event listener set up to listen to all events and manual checking. Everything works fine for me. I couldn't figure out how/where to add something to DumpRenderTree, sorry. It doesn't even compile (unmodified), so I guess I just don't understand it. If someone can give a hint or two, I may be able to do this, though.

Regarding public API/review process: Maybe one could also add documentation, as everything DOMEvent-related is missing from the ADC Reference Library. I don't know who would (be able to) do that.

Regarding .idl files: I see that the generation is done in Perl. I don't speak Perl and generally have no experience with such things, so I probably can't do this. But it doesn't hurt when the manually written DOMKeyboardEvent from my patch is included now, IMO.

[Alexey Proskuryakov]

(In reply to comment #7)
> I couldn't figure out how/where to add
> something to DumpRenderTree, sorry. It doesn't even compile (unmodified)

Does it build and run when you invoke run-webkit-tests from the command line? To make an automated test, you may need to use (extend?) EventSendingController to fake user events; see how eventSender is used in LayoutTests/fast/events tests.

[Jonas Witt]

(In reply to comment #8)
> Does it build and run when you invoke run-webkit-tests from the command line?

Yes, it does. I wrongly tried to build it from the DumpRenderTree.xcodeproj, but run-webkit-tests compiles & runs for me.

> To make an automated test, you may need to use (extend?) EventSendingController
> to fake user events; see how eventSender is used in LayoutTests/fast/events
> tests.

Are there any tests that test the currently available DOMEvent API? I think that testing this API would be a bit different from the JS-based testing that's done in LayoutTests/fast/events. I don't really know where to start... maybe one could add an event listener from ObjC code (but I don't know where), then load a HTML page that fires several events from JS and then check if the events reach the ObjC event handler correctly and that their properties match the expected values. I don't know enough of the inner workings of these tests.

[Alexey Proskuryakov]

(In reply to comment #9)

Yes, this is exactly the approach I was thinking about: add an enableDOMUIEventLogging() method to eventSender (WebKitTools/DumpRenderTree/EventSendingController.*), which would install listeners and dump the coming events to stdout. The output is compared to the *-expected.txt file which is auto-generated when the test is executed for the first time, so any changes in behavior get caught.

This automated test will not work from Safari, of course - only DumpRenderTree has these debugging extensions to DOM.

[Jonas Witt]

Created attachment 8784 [details]
Testcase for the ObjC DOMEvent API

OK, here's what I've got so far. I've added enableDOMUIEventLogging: that lets the eventSender register itself as an event lister for pretty much all event types (with the exception of DOMMutationEvents) on the node that's been given as an argument.

The event handler is supposed to take the event and dump it's properties in the result <div> on the HTML page that called enableDOMUIEventLogging: - however, as you see in the output of the first small testcase I created, I always get 0 for the coordinates of DOMMouseEvents, for example. Since this is working in the app I'm actually working on, I suppose it's related to the way that DumpRenderTree simulates and handles events. Any comments?

[Alexey Proskuryakov]

Comment on attachment 8784 [details]
Testcase for the ObjC DOMEvent API

Looks really good! A few nitpicks:
 - there are some tabs in the source - please convert them to spaces;
 - I think that printing to stdout (like editing delegates print their events) would make the output more readable, as the RenderBlocks and RenderTexts pollute the output.
- why not use [DOMNode ownerDocument]?
- An explicit check that the document is a DOMHTMLDocument would be safer.

I think that the reason for incorrect coordinates printout is that the format is wrong: int variables are printed as %f; and there's an extra [event type].

[Jonas Witt]

Created attachment 8820 [details]
Testcase for the ObjC DOMEvent API

OK, this one is better. It tests all properties of all DOMEvent (sub)classes that are currently defined in the WebKit API. Exceptions are [DOMMutationEvent relatedNode], [DOMMouseEvent relatedTarget] and [DOMUIEvent view], which return pointers to objects in the DOM tree. I don't know how they could be displayed in a useful way; calling [NSObject description] would certainly be useless because of the included memory address. Maybe [DOMNode nodeName]?

The included test (objc-event-api.html) currently tests mouse and key events only. I'm unsure how WheelEvents and MutationEvents could be generated from JS (For the MutationEvents, I've tried changing properties of DOM nodes via JS or adding child nodes from JS, but neither worked).

[Jonas Witt]

Created attachment 8821 [details]
Update WebCore.exp with DOMKeyboardEvent and DOMWheelEvent

This adds the new classes DOMKeyboardEvent and DOMWheelEvent to the exported symbols list of WebCore.framework

[Alexey Proskuryakov]

(In reply to comment #13)
> calling [NSObject description] would certainly be useless
> because of the included memory address. Maybe [DOMNode nodeName]?

  That, or maybe you could filter out the addresses with a regexp.

>  I'm unsure how WheelEvents and MutationEvents could be generated from JS

  I don't know about MutationEvents, but for WheelEvents, you could probably consider extending EventSendingController to send NSScrollWheel events. Actually, might make sense to do this separately, in another bug.

[Jonas Witt]

Created attachment 8828 [details]
Testcase for the ObjC DOMEvent API

- for [DOMUIEvent view], just check if view != nil and [view document] != nil.
- for [DOMMouseEvent relatedTarget] and [DOMMutationEvent relatedNode], print out the class name and node name of the related target/node, if it's != nil

Do we need anything else before we can land this?

[Geoffrey Garen]

There's still the question of API review at Apple, since this is a public header change. Not sure exactly what to do about that, since I've never done it before. Darin?

[Geoffrey Garen]

I think the answer to the API question is that we need to land this change as private "pending public" code. You can see examples of this in DOMPrivate.h. Basically, you declare your methods in private categories on the API class, so that they're not visible in the public headers. 

Could you attach a new patch that does that, and includes all the previous patches you've attached here?

[Jonas Witt]

Created attachment 8864 [details]
public-pending DOMKeyboardEvent, DOMWheelEvent plus testcase

I'm not sure about DOMEvent's _eventWith() function. ATM we're returning a non-public DOMWheelEvent in this public function. I don't know if that's OK. On the other hand, if we don't include DOMWheelEvent there we don't need it at all.

[Geoffrey Garen]

Comment on attachment 8864 [details]
public-pending DOMKeyboardEvent, DOMWheelEvent plus testcase

Good work.

I don't think returning the non-public object from the public method is a big deal. From the public API standpoint, it will just be a DOMUIEvent.

There's no need to use a separate file for non-W3C additions. I don't think that's how we handle other private-pending-public API. Also, the Objc bindings will be autogenerated eventually, and our IDL files probably won't distinguish W3C and non-W3C interfaces.

+				EXPORTED_SYMBOLS_FILE = "";

How did this line sneak into the project diff? Is it an XCode version quirk, or will it nuke WebCore's .exp file? I don't *think* it will, since we specify the .exp file through OTHER_LDFLAGS, not EXPORTED_SYMBOLS_FILE.

Anyway, addressing the comments above is optional.  r=me

Whoever lands this should make sure that WebCore still builds with its .exp file.

[Jonas Witt]

(In reply to comment #20)
> There's no need to use a separate file for non-W3C additions.

That's what Darin suggested in comment #4. Since all its header declarations have gone to DOMPrivate.h, it looks a bit awkward, that's right.

> +                               EXPORTED_SYMBOLS_FILE = "";
> 
> How did this line sneak into the project diff? Is it an XCode version quirk, or
> will it nuke WebCore's .exp file? I don't *think* it will, since we specify the
> .exp file through OTHER_LDFLAGS, not EXPORTED_SYMBOLS_FILE.

I first didn't know it was handled via OTHER_LDFLAGS and edited EXPORTED_SYMBOLS_FILE. It does build for me with this empty declaration, but maybe it's safer to delete that.

[Alexey Proskuryakov]

Committed revision 15008.

Many y coordinates in test results were different for me. Either something has changed in mouse handling, or the results are non-deterministic - we'll learn about that from BuildBot soon... Also changed the test to be text-only (call dumpAsText()).