[IME] Differentiate s_suppressNextKeypressEvent in WebViewImpl.

[IME] Differentiate s_suppressNextKeypressEvent in WebViewImpl.

There are different reasons why s_suppressNextKeyEvent is set to true:
1. The key event is cancelled.
2. The keydown event is taken (e.g., for backspace), so the keypress
  event is not needed.
The old code assumes they're both the same.  On Android this causes
compositions to be canclled when backspace is pressed.

An alternative fix proposed by suzhe@ is here: https://codereview.chromium.org/970743002/

BUG=453499

[huangs, 2015-02-26 00:43:03]

huangs@chromium.org changed reviewers:
+ aelias@chromium.org, boliu@chromium.org, hush@chromium.org, kbr@chromium.org,
marcheu@chromium.org, suzhe@chromium.org

[huangs, 2015-02-26 00:43:03]

PTAL.  This is an urgent CL to fix m40 Android Chrome Release blocker.

The CL affects behavior of keydown when a composition is present, and most
notably, the effect of backspace.

I'm unfamiliar of the nuances of WebKit event handlers, please be critical and
consider potential side effects on other OS's.

[huangs, 2015-02-26 00:45:48]

huangs@chromium.org changed reviewers:
+ gsennton@chromium.org

[Ken Russell (switch to Gerrit), 2015-02-26 02:11:22]

kbr@chromium.org changed reviewers:
+ esprehn@chromium.org, jchaffraix@chromium.org, shuchen@chromium.org,
tkent@chromium.org

[Ken Russell (switch to Gerrit), 2015-02-26 02:11:22]

I'm not sure who is the best reviewer for this change but think others than on
the original reviewer list (including myself) are needed. Adding more.

Please add a test to Source/web/tests/WebViewTest.cpp at the same time as this
CL. Otherwise it is inevitably going to regress again.

There's nothing Android-specific about this change, so it seems inevitable it's
going to affect other platforms. Are there already tests for IME that would
cover this code path? A full Chromium test run on all platforms needs to be run
against this before backporting it -- meaning it should land on top of tree and
be rolled into Chromium before considering backporting.

[huangs, 2015-02-26 05:33:25]

Added unit tests while breaking my fix on purpose to ensure test is meaningful. 
Probably have to iterate because I only compiled the test on local machine and
relying on try bot for running.

PTAL quickly to see if I'm on a right track, and let me know if I'm going
overboard with refactoring.  Thanks!

[huangs, 2015-02-26 07:43:55]

huangs@chromium.org changed reviewers:
+ torne@chromium.org

[James Su, 2015-02-26 09:16:59]

https://codereview.chromium.org/956133002/diff/40001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/956133002/diff/40001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2273: if (text.isEmpty() ||
m_suppressNextKeypressEvent == Suppress_KeyEventCancelled) {
1. Looks like Suppress_KeyEventHandled is not used at all. Is it correct?

2. I'm wondering how to distinguish between Handled (event.stopPropagation())
and Cancelled (event.preventDefault()) ?

3. According to my understanding, the major purpose of this logic is to make
sure a call to event.preventDefault() will actually prevent the default behavior
(the UA's behavior) of the event, which is, in this case, setting a composing
text.

Thus if we only look at the change of this line, it should be reasonable. But
I'm wondering what actually happens on the problematic websites?

As you said, the backspace got swallowed probably because the website wants to
prevent the default behavior of "navigating to previous page", then in this case
event.preventDefault() might be called by the website, instead of
event.stopPropagation().

If it's the case, then this change won't fix the problem.

[huangs, 2015-02-26 09:25:07]

Thanks for the comments.  The fix does solve the problem, but right now I'm
dealing with unit tests.

Suppress_KeyEventHandled is being used on 1033.  I changed it in Set 3 to
reproduce the original behavior, so the unit test is meaning ful (i.e., breaks
before fix and passes after fix).  However currently I'm tracing through the
test (windbg slowness hell) to see why the composition gets reset.

[huangs, 2015-02-26 12:20:07]

Uploaded and fixed test (verified that it's useful).  PTAL.

[James Su, 2015-02-26 14:05:49]

On 2015/02/26 12:20:07, huangs wrote:
> Uploaded and fixed test (verified that it's useful).  PTAL.

Just did some tests with Safari and Firefox on Mac to see how
event.preventDefault() works with composing text, the results are interesting:

Safari:
event.preventDefault() will not prevent setting/updating composing text at all.
But it'll prevent confirming the composing text with a different text.

Firefox:
If there is NO composing text, then event.preventDefault() will prevent
setting/updating/confirming composing text. (IMHO, this behavior is expected)
If a piece of composing text is already set, then event.preventDefault() will
NOT prevent updating/confirming composing text at all.

IMHO, Firefox's behavior is more reasonable. If we want to following the same
way, then I think we can simply ignore m_suppressNextKeypressEvent in
WebViewImpl::setComposition() when inputMethodController.hasComposition() is
true.

Then we don't need to differentiate m_suppressNextKeypressEvent. And it's
actually not feasible at all.


But you probably want to verify the behavior on Windows as well.

[huangs, 2015-02-26 16:21:17]

Indeed it would be cleaner to just remove the line and keep
m_suppressNextKeyPressEvent as bool.  However, for the current effort to fix the
release blocker, I think it's safer to preserve more existing behavior, and then
do the clean up later.

In fact perhaps we should get rid of the m_suppressNextKeyPressEvent altogether?
 It seems to assume that there's no interleaving of keydown and keypress events,
which I think is broken.  E.g.,

keydown A
keydown B  =>  say this sets m_suppressNextKeyPressEvent
keypress A  =>  gets suppressed!
keypress B  =>  doe snot get suppressed

It would be nice if each keypress keeps track of the associated keydown event,
and then suppression state can be tied to the event rather than the handler. 
But I digress...

[huangs, 2015-02-26 19:57:53]

Ping reviewers.

Please note that I'll be on an offsite from this afternoon to tomorrow night. 
I'll bring my laptop and should be able to make updates.

[James Su, 2015-02-27 01:46:18]

https://codereview.chromium.org/956133002/diff/60001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/956133002/diff/60001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1009: m_suppressNextKeypressEvent =
Suppress_KeyEventCanceled;
Why it's Canceled rather than Handled?

https://codereview.chromium.org/956133002/diff/60001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1033: m_suppressNextKeypressEvent =
Suppress_KeyEventHandled;
Why it's Handled?

eventHandler().keyEvent(evt) returns true whenever event.preventDefault() or
event.stopPropagation() gets called. And IMHO, event.preventDefault() should be
interpreted as Canceled, while event.stopPropagation() should be interpreted as
Handled.
So simply assuming Handled here doesn't make sense.

https://codereview.chromium.org/956133002/diff/60001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2272: if (text.isEmpty() ||
m_suppressNextKeypressEvent == Suppress_KeyEventCanceled) {
Based on this change, the composition events will never be prevented when
calling corresponding keydown event's preventDefault() method. It looks not
quite right to me.

I'd suggest to do the change in this way:

if (!inputMethodController.hasComposition() && m_suppressNextKeypressEvent) {
    return text.isEmpty();
}

if (text.isEmpty()) {
    if (inputMethodController.hasComposition()) {
        // clear composition
    }
    return true;
}

// update composition

return inputMethodController.hasComposition();


No need to change m_suppressNextKeypressEvent into an enum.

With this change, if there is no composition text, then the call to keydown's
preventDefault() will prevent the following composition event as expected.
But if there is composition text, then the call to preventDefault() will do
nothing.

[huangs, 2015-02-27 03:43:58]

Despite urgency, no reviewers other than suzhe@ has replied.  I'll defer to
suzhe@'s expertise and let him implement and test an alternative solution.

https://codereview.chromium.org/956133002/diff/60001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/956133002/diff/60001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2272: if (text.isEmpty() ||
m_suppressNextKeypressEvent == Suppress_KeyEventCanceled) {
Could you create your own CL and offer it as an alternative to this? It's
inconvenient for me to implement and test this right now. Please feel free to
copy anything you might find useful from this CL (e.g., the unit test). Thanks!

[huangs, 2015-02-27 04:15:06]

Also also see b/19363073 internally for discussions and status on the Stable
blocker.

[James Su, 2015-02-27 06:34:43]

Sorry that, I'm currently travelling in Tokyo, thus not possible to work on
it, though I can review your CL on my laptop.
 2015年2月27日 下午12:44于 <huangs@chromium.org>写道：

> Despite urgency, no reviewers other than suzhe@ has replied.  I'll defer
> to
> suzhe@'s expertise and let him implement and test an alternative solution.
>
>
> https://codereview.chromium.org/956133002/diff/60001/
> Source/web/WebViewImpl.cpp
> File Source/web/WebViewImpl.cpp (right):
>
> https://codereview.chromium.org/956133002/diff/60001/
> Source/web/WebViewImpl.cpp#newcode2272
> Source/web/WebViewImpl.cpp:2272: if (text.isEmpty() ||
> m_suppressNextKeypressEvent == Suppress_KeyEventCanceled) {
> Could you create your own CL and offer it as an alternative to this?
> It's inconvenient for me to implement and test this right now. Please
> feel free to copy anything you might find useful from this CL (e.g., the
> unit test). Thanks!
>
> https://codereview.chromium.org/956133002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[aelias_OOO_until_Jul13, 2015-02-27 21:15:11]

I chatted with Bo about the best course of action for this and we agreed that
given there's no agreement that this is a safe fix, we should just revert
https://codereview.chromium.org/846053002 on the WebView M40 branch (and
probably on M41/M42 and trunk as well, but that's something I'll follow up later
on).  I see no reason why it should be unsafe to revert, it will just bring us
back to a known-good state and only bring back the minor bug
http://crbug.com/230921 .  Then we can take more time to decide on the real fix.

[huangs, 2015-03-01 00:59:53]

suzhe@: I tried your method. I needed to change "return text.isEmpty();" to
"return !text.isEmpty();" then it seems to work after.

setComposition() is overridden from WebWidget::setComposition, which "Returns
true if the composition text was set successfully."  So why can't we just
"return false;"?

[huangs, 2015-03-01 01:00:56]

On 2015/03/01 00:59:53, huangs wrote:
> suzhe@: I tried your method. I needed to change "return text.isEmpty();" to
> "return !text.isEmpty();" then it seems to work after.
> 
> setComposition() is overridden from WebWidget::setComposition, which "Returns
> true if the composition text was set successfully."  So why can't we just
> "return false;"?

Er, I mean "return true;"?

[huangs, 2015-03-01 23:55:38]

suzhe@: I dug into your solution a bit more by apply the new unittest from this
CL.  It looks like when Backspace is pressed,


if (!inputMethodController.hasComposition() && m_suppressNextKeypressEvent) {
    return !text.isEmpty();
}

code path fails to call inputMethodController.setComposition().  As a result, we
do not properly update the end of composition (as seen in
webView->textInputInfo())?  So looks like for the Backspace code path we have to
call inputMethodController.setComposition().  However, if keypress gets
cancelled by some other way we shouldn't.  Therefore I still think
m_suppressNextKeypressEvent does not store enough information.

[huangs, 2015-03-02 00:04:03]

A symptom of the above phenomenon is that when Backspace is pressed (with
autocomplete="off"), the CompositionUpdate event does not fire.

[James Su, 2015-03-02 02:19:14]

On 2015/03/01 00:59:53, huangs wrote:
> suzhe@: I tried your method. I needed to change "return text.isEmpty();" to
> "return !text.isEmpty();" then it seems to work after.
> 
> setComposition() is overridden from WebWidget::setComposition, which "Returns
> true if the composition text was set successfully."  So why can't we just
> "return false;"?

I use "return text.isEmpty();" on purpose. It'll be weird if it doesn't work.
The purpose of this statement is that: we return false when we want to prevent a
composition text, thus the upper level can reset the input method correctly.

[James Su, 2015-03-02 02:24:56]

On 2015/03/01 23:55:38, huangs wrote:
> suzhe@: I dug into your solution a bit more by apply the new unittest from
this
> CL.  It looks like when Backspace is pressed,
> 
> 
> if (!inputMethodController.hasComposition() && m_suppressNextKeypressEvent) {
>     return !text.isEmpty();
> }
> 
> code path fails to call inputMethodController.setComposition().  As a result,
we
> do not properly update the end of composition (as seen in
> webView->textInputInfo())?  So looks like for the Backspace code path we have
to
> call inputMethodController.setComposition().  However, if keypress gets
> cancelled by some other way we shouldn't.  Therefore I still think
> m_suppressNextKeypressEvent does not store enough information.

this code block should only take effect when there is no composition. For the
case of Backspace with composition, this code block should do nothing.

[huangs, 2015-03-02 02:37:11]

That's because I made a mistake (missed "!").  Let me test again.

[huangs, 2015-03-02 02:52:51]

Okay looks like the UI works and the test passes.  :)  Going to create new patch
after I figure out what to write as comments.

[huangs, 2015-03-02 03:15:28]

Created new CL for your fix:

https://codereview.chromium.org/970743002/

Please see if the comments are accurate, and check the tests.  Thanks!