When editing IME, `compositionend` events should fire after input events
<rdar://problem/29050438>
Created attachment 293666 [details] Patch
Created attachment 293668 [details] Patch
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
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 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
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
Created attachment 293686 [details] Rebaselining
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));
(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!
Created attachment 294061 [details] Patch for landing
Created attachment 294245 [details] Patch for landing
Comment on attachment 294245 [details] Patch for landing Clearing flags on attachment: 294245 Committed r208462: <http://trac.webkit.org/changeset/208462>