Bug 164324 - When editing IME, `compositionend` events should fire after input events
Summary: When editing IME, `compositionend` events should fire after input events
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks: 163112
  Show dependency treegraph
 
Reported: 2016-11-02 09:53 PDT by Wenson Hsieh
Modified: 2016-11-09 13:15 PST (History)
5 users (show)

See Also:


Attachments
Patch (13.21 KB, patch)
2016-11-02 10:06 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (10.58 KB, patch)
2016-11-02 10:09 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (1.08 MB, application/zip)
2016-11-02 11:09 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (1.72 MB, application/zip)
2016-11-02 11:16 PDT, Build Bot
no flags Details
Rebaselining (11.99 KB, patch)
2016-11-02 12:20 PDT, Wenson Hsieh
darin: review+
Details | Formatted Diff | Diff
Patch for landing (11.82 KB, patch)
2016-11-07 07:51 PST, Wenson Hsieh
wenson_hsieh: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (11.82 KB, patch)
2016-11-09 11:07 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2016-11-02 09:53:44 PDT
When editing IME, `compositionend` events should fire after input events
Comment 1 Wenson Hsieh 2016-11-02 09:54:29 PDT
<rdar://problem/29050438>
Comment 2 Wenson Hsieh 2016-11-02 10:06:58 PDT
Created attachment 293666 [details]
Patch
Comment 3 Wenson Hsieh 2016-11-02 10:09:43 PDT
Created attachment 293668 [details]
Patch
Comment 4 Build Bot 2016-11-02 11:09:24 PDT
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
Comment 5 Build Bot 2016-11-02 11:09:27 PDT
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
Comment 6 Build Bot 2016-11-02 11:16:25 PDT
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
Comment 7 Build Bot 2016-11-02 11:16:28 PDT
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
Comment 8 Wenson Hsieh 2016-11-02 12:20:30 PDT
Created attachment 293686 [details]
Rebaselining
Comment 9 Darin Adler 2016-11-05 15:49:34 PDT
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));
Comment 10 Wenson Hsieh 2016-11-05 16:11:59 PDT
(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!
Comment 11 Wenson Hsieh 2016-11-07 07:51:51 PST
Created attachment 294061 [details]
Patch for landing
Comment 12 Wenson Hsieh 2016-11-09 11:07:50 PST
Created attachment 294245 [details]
Patch for landing
Comment 13 WebKit Commit Bot 2016-11-09 12:09:05 PST
Comment on attachment 294245 [details]
Patch for landing

Clearing flags on attachment: 294245

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