[IME] Differentiate s_suppressNextKeypressEvent in WebViewImpl.

Source/web/WebViewImpl.cpp

 /*
  * Copyright (C) 2011, 2012 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 371 matching lines @@
     , m_doubleTapZoomPageScaleFactor(0)
     , m_doubleTapZoomPending(false)
     , m_enableFakePageScaleAnimationForTesting(false)
     , m_fakePageScaleAnimationPageScaleFactor(0)
     , m_fakePageScaleAnimationUseAnchor(false)
     , m_contextMenuAllowed(false)
     , m_doingDragAndDrop(false)
     , m_ignoreInputEvents(false)
     , m_compositorDeviceScaleFactorOverride(0)
     , m_rootLayerScale(1)
-    , m_suppressNextKeypressEvent(false)
+    , m_suppressNextKeypressEvent(Suppress_None)
     , m_imeAcceptEvents(true)
     , m_operationsAllowed(WebDragOperationNone)
     , m_dragOperation(WebDragOperationNone)
     , m_isTransparent(false)
     , m_tabsToLinks(false)
     , m_layerTreeView(0)
     , m_rootLayer(0)
     , m_rootGraphicsLayer(0)
     , m_rootTransformLayer(0)
     , m_graphicsLayerFactory(adoptPtr(new GraphicsLayerFactoryChromium(this)))
@@ skipping 583 matching lines @@
         || (event.type == WebInputEvent::KeyDown)
         || (event.type == WebInputEvent::KeyUp));
 
     // Halt an in-progress fling on a key event.
     endActiveFlingAnimation();
 
     // Please refer to the comments explaining the m_suppressNextKeypressEvent
     // member.
     // The m_suppressNextKeypressEvent is set if the KeyDown is handled by
     // Webkit. A keyDown event is typically associated with a keyPress(char)
-    // event and a keyUp event. We reset this flag here as this is a new keyDown
+    // event and a keyUp event. We reset this state here as this is a new keyDown
     // event.
-    m_suppressNextKeypressEvent = false;
+    m_suppressNextKeypressEvent = Suppress_None;
 
     // If there is a select popup, it should be the one processing the event,
     // not the page.
     if (m_selectPopup)
         return m_selectPopup->handleKeyEvent(PlatformKeyboardEventBuilder(event));
     if (m_pagePopup) {
         m_pagePopup->handleKeyEvent(PlatformKeyboardEventBuilder(event));
         // We need to ignore the next Char event after this otherwise pressing
         // enter when selecting an item in the popup will go to the page.
         if (WebInputEvent::RawKeyDown == event.type)
-            m_suppressNextKeypressEvent = true;
+            m_suppressNextKeypressEvent = Suppress_KeyEventCanceled;

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

Why it's Canceled rather than Handled?

         return true;
     }
 
     RefPtrWillBeRawPtr<Frame> focusedFrame = focusedCoreFrame();
     if (focusedFrame && focusedFrame->isRemoteFrame()) {
         WebRemoteFrameImpl* webFrame = WebRemoteFrameImpl::fromFrame(*toRemoteFrame(focusedFrame.get()));
         webFrame->client()->forwardInputEvent(&event);
         return true;
     }
 
     if (!focusedFrame || !focusedFrame->isLocalFrame())
         return false;
 
     LocalFrame* frame = toLocalFrame(focusedFrame.get());
 
     PlatformKeyboardEventBuilder evt(event);
 
     if (frame->eventHandler().keyEvent(evt)) {
         if (WebInputEvent::RawKeyDown == event.type) {
             // Suppress the next keypress event unless the focused node is a plug-in node.
             // (Flash needs these keypress events to handle non-US keyboards.)
             Element* element = focusedElement();
             if (!element || !element->renderer() || !element->renderer()->isEmbeddedObject())
-                m_suppressNextKeypressEvent = true;
+                m_suppressNextKeypressEvent = Suppress_KeyEventHandled;

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

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.

         }
         return true;
     }
 
 #if !OS(MACOSX)
     const WebInputEvent::Type contextMenuTriggeringEventType =
 #if OS(WIN)
         WebInputEvent::KeyUp;
 #else
         WebInputEvent::RawKeyDown;
@@ skipping 12 matching lines @@
 
 bool WebViewImpl::handleCharEvent(const WebKeyboardEvent& event)
 {
     ASSERT(event.type == WebInputEvent::Char);
 
     // Please refer to the comments explaining the m_suppressNextKeypressEvent
     // member.  The m_suppressNextKeypressEvent is set if the KeyDown is
     // handled by Webkit. A keyDown event is typically associated with a
     // keyPress(char) event and a keyUp event. We reset this flag here as it
     // only applies to the current keyPress event.
-    bool suppress = m_suppressNextKeypressEvent;
-    m_suppressNextKeypressEvent = false;
+    bool suppress = (m_suppressNextKeypressEvent != Suppress_None);
+    m_suppressNextKeypressEvent = Suppress_None;
 
     // If there is a select popup, it should be the one processing the event,
     // not the page.
     if (m_selectPopup)
         return m_selectPopup->handleKeyEvent(PlatformKeyboardEventBuilder(event));
     if (m_pagePopup)
         return m_pagePopup->handleKeyEvent(PlatformKeyboardEventBuilder(event));
 
     LocalFrame* frame = toLocalFrame(focusedCoreFrame());
     if (!frame)
@@ skipping 1183 matching lines @@
     // editable because JavaScript may delete a parent node of the composition
     // node. In this case, WebKit crashes while deleting texts from the parent
     // node, which doesn't exist any longer.
     RefPtrWillBeRawPtr<Range> range = inputMethodController.compositionRange();
     if (range) {
         Node* node = range->startContainer();
         if (!node || !node->isContentEditable())
             return false;
     }
 
-    // If we're not going to fire a keypress event, then the keydown event was
-    // canceled.  In that case, cancel any existing composition.
-    if (text.isEmpty() || m_suppressNextKeypressEvent) {
+    // The keydown event was canceled, so cancel any existing composition.
+    if (text.isEmpty() || m_suppressNextKeypressEvent == Suppress_KeyEventCanceled) {

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

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]

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!

         // A browser process sent an IPC message which does not contain a valid
         // string, which means an ongoing composition has been canceled.
         // If the ongoing composition has been canceled, replace the ongoing
         // composition string with an empty string and complete it.
         String emptyString;
         Vector<CompositionUnderline> emptyUnderlines;
         inputMethodController.setComposition(emptyString, emptyUnderlines, 0, 0);
         return text.isEmpty();
     }
 
@@ skipping 2351 matching lines @@
     return mainFrameImpl()->frameView()->layoutSize().width() == m_size.width
         || (constraints.minimumScale == constraints.maximumScale && constraints.minimumScale != -1);
 }
 
 void WebViewImpl::forceNextWebGLContextCreationToFail()
 {
     WebGLRenderingContext::forceNextWebGLContextCreationToFail();
 }
 
 } // namespace blink