Bug 164324 – When editing IME, `compositionend` events should fire after input events

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Wenson Hsieh]

When editing IME, `compositionend` events should fire after input events

[Wenson Hsieh]

<rdar://problem/29050438>

[Wenson Hsieh]

Created attachment 293666 [details]
Patch

[Wenson Hsieh]

Created attachment 293668 [details]
Patch

[Build Bot]

Comment on attachment 293668 [details]
Patch

Attachment 293668 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2451173

New failing tests:
fast/events/ime-composition-events-001.html

[Build Bot]

Created attachment 293675 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 293668 [details]
Patch

Attachment 293668 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2451172

New failing tests:
fast/events/ime-composition-events-001.html

[Build Bot]

Created attachment 293677 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Wenson Hsieh]

Created attachment 293686 [details]
Rebaselining

[Darin Adler]

Comment on attachment 293686 [details]
Rebaselining

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

My main concern about such code is reentrancy. What happens if the code that handles the compositionend event performs other editing operations. Is the work done after handling the event still OK if lots of things have happened and lots of things have changed? Maybe the compositionend event should be delivered even later, after things like the call to closeTyping and setIgnoreCompositionSelectionChange(false). We should think about how to write a test for that sort of thing.

> Source/WebCore/editing/Editor.cpp:1682
> +    // Dispatch a compositionend event to the focused node.

Comment isn’t great. Says exactly the same thing the code does. We should only write a comment if it says something that the code does not say.

> Source/WebCore/editing/Editor.cpp:1686
> +    if (Element* target = document().focusedElement()) {
> +        Ref<CompositionEvent> event = CompositionEvent::create(eventNames().compositionendEvent, document().domWindow(), text);
> +        target->dispatchEvent(event);
> +    }

I know you just moved this, and didn’t write it, but the code would read better without the local variable. And I would use auto:

    if (auto* focusedElement = document().focusedElement())
        focusedElement->dispatchEvent(CompositionEvent::create(eventNames().compositionendEvent, document().domWindow(), text));

[Wenson Hsieh]

(In reply to comment #9)
> Comment on attachment 293686 [details]
> Rebaselining
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=293686&action=review
> 
> My main concern about such code is reentrancy. What happens if the code that
> handles the compositionend event performs other editing operations. Is the
> work done after handling the event still OK if lots of things have happened
> and lots of things have changed? Maybe the compositionend event should be
> delivered even later, after things like the call to closeTyping and
> setIgnoreCompositionSelectionChange(false). We should think about how to
> write a test for that sort of thing.

I see. Perhaps we should enqueue this event via something like dispatchScopedEvent instead of directly calling dispatchEvent.

In WK2, Editor::setComposition, which dispatches the compositionend events, is invoked via a message from the UI process to setCompositionAsync, so I'm not sure reentrancy is a concern there. However, on WK1, I can imagine a situation where reentrancy could occur if -[WebHTMLView setMarkedText:selectionRange:] is directly called from within a compositionend handler in an app using a WebView.

> 
> > Source/WebCore/editing/Editor.cpp:1682
> > +    // Dispatch a compositionend event to the focused node.
> 
> Comment isn’t great. Says exactly the same thing the code does. We should
> only write a comment if it says something that the code does not say.

Good point -- removed the comment.

> 
> > Source/WebCore/editing/Editor.cpp:1686
> > +    if (Element* target = document().focusedElement()) {
> > +        Ref<CompositionEvent> event = CompositionEvent::create(eventNames().compositionendEvent, document().domWindow(), text);
> > +        target->dispatchEvent(event);
> > +    }
> 
> I know you just moved this, and didn’t write it, but the code would read
> better without the local variable. And I would use auto:
> 
>     if (auto* focusedElement = document().focusedElement())
>        
> focusedElement->dispatchEvent(CompositionEvent::create(eventNames().
> compositionendEvent, document().domWindow(), text));

Sounds good!

[Wenson Hsieh]

Created attachment 294061 [details]
Patch for landing

[Wenson Hsieh]

Created attachment 294245 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 294245 [details]
Patch for landing

Clearing flags on attachment: 294245

Committed r208462: <http://trac.webkit.org/changeset/208462>