Adding support for Smart GO NEXT feature in Android Chrome

Source/web/WebViewImpl.cpp

Index: Source/web/WebViewImpl.cpp
diff --git a/Source/web/WebViewImpl.cpp b/Source/web/WebViewImpl.cpp
index 50a9d84742d99a31c48274c54d42174bc6df2596..80e03e2c8d3372a924de57eee148c6dd5c800d0c 100644
--- a/Source/web/WebViewImpl.cpp
+++ b/Source/web/WebViewImpl.cpp
@@ -116,6 +116,7 @@
 #include "public/platform/WebCompositeAndReadbackAsyncCallback.h"
 #include "public/platform/WebDragData.h"
 #include "public/platform/WebFloatPoint.h"
+#include "public/platform/WebFocusType.h"
 #include "public/platform/WebGestureCurve.h"
 #include "public/platform/WebImage.h"
 #include "public/platform/WebLayerTreeView.h"
@@ -2519,9 +2520,65 @@ int WebViewImpl::textInputFlags()
         }
     }
 
+    if (wantEnterEvents(element))
+        flags |= WebTextInputFlagWantEnterEvents;
+
+    if (haveNextInput(element, WebFocusTypeForward))
+        flags |= WebTextInputFlagHaveNextInput;
+
+    if (haveNextInput(element, WebFocusTypeBackward))
+        flags |= WebTextInputFlagHavePreviousInput;
+
     return flags;
 }
 
+Element* WebViewImpl::haveNextInput(Element* element, WebFocusType focusType)

[tkent, 2015/04/14 23:27:19]

The function name is not reasonable.
- The name looks like it returns a bool, but it returns an element.  The name
should be |nextInput|.
- The function returns not only <input>, but also <textarea> and contenteditable
elements.  The name should be |nextEditableElement|.
- A TODO comment says this will be expanded to other form controls.  The name
should be |nextFocusableElement| or something.

[AKV, 2015/04/16 19:05:19]

Done. nextFocusableElement is ideal. Thanks :)

+{
+    if (!element->isFormControlElement())

[tkent, 2015/04/14 23:27:19]

Do you want to reject a contenteditable element though this function can return
a contenteditable element?

[AKV, 2015/04/16 19:05:19]

I care about elements which are inside the form. If the ContentEditable is child
of a form, then also isFormControlElement() function returns false ? Could you
please clarify on this ?

[tkent, 2015/04/17 04:12:35]

IsFormControlElement() return false for contenteditable elements.  They are not
form controls.

+        return nullptr;
+
+    HTMLFormControlElement* formControlElement = toHTMLFormControlElement(element);
+    Node* parentFormNode = reinterpret_cast<Node*>(formControlElement->formOwner());

[tkent, 2015/04/14 23:27:19]

|parentFormNode| is not a good name because it's not a parent, and it's an
element.  |formOwner| is a good candidate.
You don't need to use reinterpret_cast<>.

[AKV, 2015/04/16 19:05:19]

Done.

+    if (!parentFormNode)
+        return nullptr;
+
+    Node* nextNode = element;
+    while ((nextNode = page()->focusController().findFocusableNode(focusType, *nextNode))) {
+        if (!nextNode->isDescendantOf(parentFormNode))

[tkent, 2015/04/14 23:27:19]

This doesn't work for the following case.

<form id=f1>
<input >  <!-- This is the current focused element -->
</form>
<input form=f1>

We should check HTMLFormControlElement::formOwner() equivalence.

[kochi, 2015/04/15 01:09:18]

Hmm, I was not aware of this way of declaring form element outside
<form></form>.
It means that until the focus controller reaches browser chrome (out of
content),
we have to search for a focusable element associated with the current form.

IIUC this is not a performance-critical path, so it would not matter much for
iterating nodes exhaustively. It might be worth exploring the solution to
iterate through HTMLFormElement::associatedElements().

[AKV, 2015/04/16 19:05:19]

Thanks. I have taken care this case by checking formOwner.

[AKV, 2015/04/16 19:05:19]

Thanks. I have taken care this case by checking formOwner.

+            return nullptr;
+        LayoutObject* layout = nextNode->layoutObject();
+        if (nextNode->isContentEditable() || (layout && layout->isTextControl())) {
+            // TODO(AKV) Extend it for Select element, Radio button and Check boxes
+            if (nextNode->isElementNode())

[tkent, 2015/04/14 23:27:19]

No need to check isElementNode.  Only elements are focusable.

[AKV, 2015/04/16 19:05:19]

Done.

+                return toElement(nextNode);
+        }
+    }
+    return nullptr;
+}
+
+bool WebViewImpl::wantEnterEvents(Element* element)

[tkent, 2015/04/14 23:27:19]

The argument should be |const Element&|.

The function name and the implementation doesn't match.  The name should be
|isListeningToKeyboardEvents| if we follow the implementation.  What's the
intention of this function?

[AKV, 2015/04/16 19:05:19]

I wanted to check whether the element has any listener for "Enter Key". If no
enter key listener, we don't want to show Enter key from IME side. I will check
specific case of Enter Key instead of whole key events. Do you have any
suggestion to check Enter key listener is present or not ? Thanks.

[tkent, 2015/04/17 04:12:35]

No way to check only Enter key.

BTW, If we don't show the enter key in IME, it's impossible for users to submit
a form without a submit button like:

<form action="... ">
<input name=foo>
</form>

+{
+    if (!element->isFormControlElement())
+        return false;
+
+    return (element->hasEventListeners(EventTypeNames::keydown) || element->hasEventListeners(EventTypeNames::keypress) || element->hasEventListeners(EventTypeNames::keystatuseschange) || element->hasEventListeners(EventTypeNames::keyup));

[tkent, 2015/04/14 23:27:19]

Why do you check keystatuschange event, which is for encrypted media?

[AKV, 2015/04/16 19:05:19]

Right. this is not required.

+}
+
+void WebViewImpl::advanceFocusToNextInputField(WebFocusType focusType)
+{
+    Element* element = focusedElement();
+    if (!element)
+        return;
+
+    Element* nextElement = haveNextInput(element, focusType);
+    if (!nextElement)
+        return;
+
+    nextElement->scrollIntoViewIfNeeded(true /*centerIfNeeded*/);
+    nextElement->dispatchSimulatedClick(0, SendMouseUpDownEvents);

[tkent, 2015/04/14 23:27:19]

0 -> nullptr.

Why do you need to dispatch a click event?

[AKV, 2015/04/16 19:05:19]

Not required. Thanks.

+    nextElement->focus(false, focusType);

[tkent, 2015/04/14 23:27:19]

The click event handler can delete |nextElement|.  We need to make |nextElement|
|RefPtrWIllBeRawPtr<Element>|.

[AKV, 2015/04/16 19:05:19]

Thanks. I corrected it.

+}
+
 WebString WebViewImpl::inputModeOfFocusedElement()
 {
     if (!RuntimeEnabledFeatures::inputModeAttributeEnabled())