Make "compositionstart" event cancelable (issue 311053008)

[yo...@chromium.org]

Reviewers: Yuta Kitamura, yoichio, Takayoshi Kochi,

Message:
Could you review this patch?
Thanks in advance.

Description:
Make "compositionstart" event cancelable

This patch makes "compositionstart" event cancelable as DOM Level 3 event
specification[1].

[1] http://www.w3.org/TR/DOM-Level-3-Events/#events-compositionevents
BUG=228440
TEST=Source/core/editing/InputMethodControllerTest.cpp


Please review this at https://codereview.chromium.org/311053008/

SVN Base: https://chromium.googlesource.com/chromium/blink.git@master

Affected files (+76, -1 lines):
M Source/core/core.gypi
M Source/core/editing/InputMethodController.cpp
A Source/core/editing/InputMethodControllerTest.cpp


Index: Source/core/core.gypi
diff --git a/Source/core/core.gypi b/Source/core/core.gypi
index
5b8c58f59f6e12fa711694e4b76c10a14c4234af..439f3e967fe20a2f4e79b9a62af4fb9476d424a1
100644
--- a/Source/core/core.gypi
+++ b/Source/core/core.gypi
@@ -3385,6 +3385,7 @@
'dom/MainThreadTaskRunnerTest.cpp',
'dom/RangeTest.cpp',
'dom/TreeScopeTest.cpp',
+ 'editing/InputMethodControllerTest.cpp',
'editing/TextIteratorTest.cpp',
'editing/VisibleSelectionTest.cpp',
'fetch/CachingCorrectnessTest.cpp',
Index: Source/core/editing/InputMethodController.cpp
diff --git a/Source/core/editing/InputMethodController.cpp
b/Source/core/editing/InputMethodController.cpp
index
3609e75405069a27fc42c957cd5e295daa8802a6..f50dc088a4f953724c8e2fe5acd37c8a47ed1cf5
100644
--- a/Source/core/editing/InputMethodController.cpp
+++ b/Source/core/editing/InputMethodController.cpp
@@ -259,7 +259,9 @@ void InputMethodController::setComposition(const
String& text, const Vector<Comp
// We should send a compositionstart event only when the given
text is not empty because this
// function doesn't create a composition node when the text is
empty.
if (!text.isEmpty()) {
-
target->dispatchEvent(CompositionEvent::create(EventTypeNames::compositionstart,
m_frame.domWindow(), m_frame.selectedText(), underlines));
+ RefPtrWillBeRawPtr<CompositionEvent> compositionStartEvent
= CompositionEvent::create(EventTypeNames::compositionstart,
m_frame.domWindow(), m_frame.selectedText(), underlines);
+ if (!target->dispatchEvent(compositionStartEvent))
+ return;
event =
CompositionEvent::create(EventTypeNames::compositionupdate,
m_frame.domWindow(), text, underlines);
}
} else {
Index: Source/core/editing/InputMethodControllerTest.cpp
diff --git a/Source/core/editing/InputMethodControllerTest.cpp
b/Source/core/editing/InputMethodControllerTest.cpp
new file mode 100644
index
0000000000000000000000000000000000000000..b432aefc0ec7d1d83da129ebc5b0f8b12b28371b
--- /dev/null
+++ b/Source/core/editing/InputMethodControllerTest.cpp
@@ -0,0 +1,72 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "config.h"
+#include "core/editing/InputMethodController.h"
+
+#include "core/dom/Text.h"
+#include "core/editing/FrameSelection.h"
+#include "core/frame/LocalFrame.h"
+#include "core/frame/Settings.h"
+#include "core/html/HTMLDocument.h"
+#include "core/html/HTMLElement.h"
+#include "core/testing/DummyPageHolder.h"
+#include <gtest/gtest.h>
+
+using namespace WebCore;
+
+namespace WebCore {
+
+class InputMethodControllerTest : public ::testing::Test {
+protected:
+ HTMLDocument& document() const { return *m_document; }
+ LocalFrame& frame() const { return m_dummyPageHolder->frame(); }
+
+private:
+ virtual void SetUp() OVERRIDE;
+
+ OwnPtr<DummyPageHolder> m_dummyPageHolder;
+ HTMLDocument* m_document;
+};
+
+void InputMethodControllerTest::SetUp()
+{
+ m_dummyPageHolder = DummyPageHolder::create(IntSize(800, 600));
+ m_document = toHTMLDocument(&m_dummyPageHolder->document());
+ ASSERT(m_document);
+}
+
+} // namespace WebCore
+
+namespace {
+
+using namespace WebCore;
+
+TEST_F(InputMethodControllerTest, setComposition)
+{
+ document().write("<div contenteditable='true' id='target'>foo</div>");
+ RefPtrWillBeRawPtr<Element> input =
document().getElementById("target");
+ frame().selection().moveTo(Position(toText(input->firstChild()), 0),
DOWNSTREAM);
+ frame().inputMethodController().setComposition("bar",
Vector<CompositionUnderline>(), 0, 0);
+ EXPECT_STREQ("barfoo", input->textContent().utf8().data());
+}
+
+TEST_F(InputMethodControllerTest, setCompositionCancelCompositionStart)
+{
+ frame().settings()->setScriptEnabled(true);
+ document().write("<div contenteditable='true' id='target'>foo</div>"
+ "<script>"
+ "document.getElementById('target').addEventListener('compositionstart',
function(event)
{"
+ " event.preventDefault();"
+ "});"
+ "</script>");
+ document().updateLayout();
+ RefPtrWillBeRawPtr<Element> input =
document().getElementById("target");
+ document().setFocusedElement(input);
+ frame().selection().moveTo(Position(toText(input->firstChild()), 0),
DOWNSTREAM);
+ frame().inputMethodController().setComposition("bar",
Vector<CompositionUnderline>(), 0, 0);
+ EXPECT_STREQ("foo", input->textContent().utf8().data());
+}
+
+} // namespace WebCore

[yu...@chromium.org]

According to
<https://developer.mozilla.org/en-US/docs/DOM/DOM_event_reference/compositionstart>,
this event is not cancelable in Gecko.

The following statement from the MDN looks worrysome:

> Gecko fires this event when IME starts composition, and
> some platforms don't have an API for canceling composition
> once it's begun. In addition, Gecko can't know whether a
> keyboard event will start composition or not until IME
> actually starts composition. Because of this,
> event.preventDefault() doesn't work on compositionstart
> events in Gecko.

Does this concerm apply to Blink?

https://codereview.chromium.org/311053008/

[ko...@chromium.org]

Just ignoring composition updates from IME will make the state between
Blink and IME inconsistent.

For example, Blink can just ignore composition update, but IME still
maintain its state (composition text and other related information).
There is no ACK mechanism that IME can confirm that its update is
accepted by editor etc.

We should make what "cancel" mean and how cancellation should behave first,
before doing implementation.


https://codereview.chromium.org/311053008/

[ko...@chromium.org]

According to the spec, cancellation differs from IME cancellation (close
the IME window or hit ESC key). So the cancel here looks fine - just
cancelling the current composition session.

Once compositionstart event is canceled, accompanying compositionend event
has to be sent.

In the spec, titled "Canceling Composition Events"
> If the initial compositionstart event is canceled then the text
> composition
> session should be terminated. Regardless of whether or not the composition
> session is terminated, the compositionend event must be sent.


https://codereview.chromium.org/311053008/

[ko...@chromium.org]

https://codereview.chromium.org/311053008/diff/20001/Source/core/editing/InputMethodController.cpp
File Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/311053008/diff/20001/Source/core/editing/InputMethodController.cpp#newcode264
Source/core/editing/InputMethodController.cpp:264: return;
You have to dispatch compositionend event for this case.

https://codereview.chromium.org/311053008/

[yo...@chromium.org]

On 2014/06/06 02:16:30, Yuta Kitamura wrote:
> According to

<https://developer.mozilla.org/en-US/docs/DOM/DOM_event_reference/compositionstart>,
> this event is not cancelable in Gecko.

> The following statement from the MDN looks worrysome:

> > Gecko fires this event when IME starts composition, and
> > some platforms don't have an API for canceling composition
> > once it's begun. In addition, Gecko can't know whether a
> > keyboard event will start composition or not until IME
> > actually starts composition. Because of this,
> > event.preventDefault() doesn't work on compositionstart
> > events in Gecko.

> Does this concerm apply to Blink?

Blink implementation of "compositionstart" event dispatch doesn't relate to
keyboard event. It is dispatched by |
InputMethodController::setComposition()|
when there is no |Node| for composition in DOM tree.

"compositionstart" isn't tight with keyboard. It can be fired speech
recognizer,
hand writing and so on.

Note: IE's compositionstart is cancelable.


https://codereview.chromium.org/311053008/

[yu...@chromium.org]

On 2014/06/09 02:29:14, yosi wrote:
> Blink implementation of "compositionstart" event dispatch doesn't relate
> to
> keyboard event. It is dispatched by |
> InputMethodController::setComposition()|
> when there is no |Node| for composition in DOM tree.

> "compositionstart" isn't tight with keyboard. It can be fired speech
recognizer,
> hand writing and so on.

I don't understand how this explanation relates to the MDN notes.

Please provide enough context about this compatibility story
in the change description.


https://codereview.chromium.org/311053008/

[yo...@chromium.org]

PTAL

I update
- change description what Blink and Chromium does at composition
cancelation
- dispatch "compositionend" event as the spec.

https://codereview.chromium.org/311053008/

[ko...@chromium.org]

On 2014/06/09 03:35:45, yosi wrote:
> PTAL

> I update
> - change description what Blink and Chromium does at composition
> cancelation
> - dispatch "compositionend" event as the spec.

I don't think the added paragraph ("In Blink, ~") in the description well
explains what this CL is.
If the intention was to explain the responsibility for cancelling the
current
composition,
how about the following:

A cancellation of "compositionstart" results in a renderer->browser IPC
(ViewHostMsg_ImeCancelComposition), and each platform-dependent RWHV
implementation is
responsible for handling it.


I don't understand your reply to yutak as well, you don't explain why
it is okay to land this CL in blink side, while some platform may not be
able to implement the functionality.

Please add yukishiino@ and shuchen@ for comments/concerns about
Linux/ChromeOS
implementations.


https://codereview.chromium.org/311053008/

[yo...@chromium.org]

On 2014/06/09 04:02:34, Takayoshi Kochi wrote:
> On 2014/06/09 03:35:45, yosi wrote:
> > PTAL
> >
> > I update
> > - change description what Blink and Chromium does at composition
cancelation
> > - dispatch "compositionend" event as the spec.

> I don't think the added paragraph ("In Blink, ~") in the description well
> explains what this CL is.
> If the intention was to explain the responsibility for cancelling the
> current
> composition,
> how about the following:

> A cancellation of "compositionstart" results in a renderer->browser IPC
> (ViewHostMsg_ImeCancelComposition), and each platform-dependent RWHV
> implementation is
> responsible for handling it.


> I don't understand your reply to yutak as well, you don't explain why
> it is okay to land this CL in blink side, while some platform may not be
> able to implement the functionality.

According to the spec, actual behavior of composition cancelation depends on
platform, e.g. Linux doesn't have concept about composition cancelation. It
seems there is way to simulate composition cancelation like focus movement
during text composition.

> Please add yukishiino@ and shuchen@ for comments/concerns about
> Linux/ChromeOS
> implementations.

I updated CL description and added yukishiino@ and shuchen@ to reviewer.

yukishiino@ and shuchen@, please put comments/concerns about composition
cancelation on Linux/ChromeOS.





https://codereview.chromium.org/311053008/

[yuk...@chromium.org]

HMM, I'm not sure how this proposed ViewHostMsg_ImeCancelComposition
resolves
the
original crbug.com/228440. (Is't on Android!)

Can we have a DesignDoc? I guess there are a lot of potential issues to be
discussed.
- Platform API feasibility
- Chromium specific limitations due to asynchronous IPCs.
- Test plans.
- etc...


https://codereview.chromium.org/311053008/

[yo...@chromium.org]

This patch is for Blink side changes of making "compositionstart"
cancelable.
"compositonend" event dispatch and Linu(http://crbug.com/382930) issue on
composition cancelation will be addressed by other patches. (Not sure about
ChromeOS at this time)

Here is design doc:
https://docs.google.com/a/chromium.org/document/d/138_d6VuMnAK2QELmdG-KtwAv7-7ru_fJL-l0o_EXkSU/edit#

Note: Blink editing team priority is implementing polyfill/C++ of
contentEditable=minimal. Implementing "compositionend" at cancellation will
be
handled later.

https://codereview.chromium.org/311053008/