1398374 - Android Keyboard displays Next instead of Go (Works on Chrome)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Keyboards and IME, enhancement)

Description

[Mike Kaply [:mkaply]]

When you navigate to this website on Fennec:

http://browserhome.net/

And enter text in the search field, the Android Keyboard shows "next". Clicking it simply navigates to the Facebook button.

On Chrome, the Android Keyboard has "Go" and clicking it searches properly on the website.

Comment 1

[Jan Henning [:JanH]]

notifyIMEContext (https://dxr.mozilla.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoInputConnection.java#971) is being called with typeHint="text, modeHint="" and actionHint="next", so the problem is somewhere within Gecko's input handling (probably the IMEStateManager).

This is the markup for the search field:
> <div class="search-main clearfix">
>   <input name="ie" value="UTF-8" type="hidden">
>   <input name="brand" value="WIKO" type="hidden">
>   <input class="input-hidden" type="text">
>   <input value="" name="search_term" autocomplete="off" id="search-text" placeholder="What are you looking for?" type="text">
> </div>

If I delete the first three input nodes in the DOM Inspector, then notifyIMEContext is being called with actionHint="go" and pressing that button submits the search.

Comment 2

[Jan Henning [:JanH]]

Attachment: testcase actionHint behaviour

As far as I can tell, the difference in behaviour seems to happen because we default to an actionHint of "next" and only send something else (like "go" or "search") if the form has either a default submit element, or else if it is implicitly submittable [1].
Because for unknown reasons browserhome.net uses a second text <input> that is only hidden via "display: none", but not disabled, this means that as per the spec the search form is not implicitly submittable [2].

Chrome's algorithm on the other hand more or less seems to ignore submittability and (special handling for search fields and textareas aside) simply uses IME_ACTION_GO for the last input element of a form and IME_ACTION_NEXT for everything else [3]

[1] https://dxr.mozilla.org/mozilla-central/rev/57f68296c350469d73d788eb3695a898947b4acb/dom/events/IMEStateManager.cpp#1337
[2] https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#implicit-submission
[3] https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java?l=146&rcl=fca18931b5ee2b1e272ba0e86ac98f4c10d12e05

Comment 3

[Jan Henning [:JanH]]

Attachment: Bug 1398374 - Part 1 - Add helper method to test for the last non-disabled single line form input.

We want to treat those specially in IME handling.

Review commit: https://reviewboard.mozilla.org/r/185436/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/185436/

Comment 4

[Jan Henning [:JanH]]

Attachment: Bug 1398374 - Part 2 - Send an IME actionHint other than "next" for the last input element of a form.

Some pages might use neither a default submit element nor rely on implicit submission and instead implement their form handling with JS key event listeners.

On Android, most IMEs won't allow sending an "Enter" key press if we're using an actionHint of "next" as our default, so we now use "go"/"search" for the last single line text/number input that is not disabled.

Review commit: https://reviewboard.mozilla.org/r/185438/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/185438/

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8914127 [details]
Bug 1398374 - Part 2 - Send an IME actionHint other than "next" for the last input element of a form.

https://reviewboard.mozilla.org/r/185438/#review190416

::: dom/events/IMEStateManager.cpp:1340
(Diff revision 1)
> +          // ... and does it only have a single text input element ?
> +          if (!htmlForm->ImplicitSubmissionIsDisabled()) {
> -          willSubmit = true;
> +            willSubmit = true;
> +          // ... or is this the last non-disabled element?
> +          } else if (htmlForm->IsLastActiveElement(control)) {
> +            willSubmit = true;
> +          }

Why don't you just use || instead of else if? But up to you.

Comment 6

[Jan Henning [:JanH]]

Comment on attachment 8914127 [details]
Bug 1398374 - Part 2 - Send an IME actionHint other than "next" for the last input element of a form.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185438/diff/1-2/

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8914126 [details]
Bug 1398374 - Part 1 - Add helper method to test for the last non-disabled single line form input.

https://reviewboard.mozilla.org/r/185436/#review190770

r=me with the loop iteration issue below fixed.

::: dom/html/HTMLFormElement.cpp:1848
(Diff revision 1)
> +{
> +  NS_PRECONDITION(aControl, "Unexpected call");
> +
> +  bool isLastActive = false;
> +  uint32_t length = mControls->mElements.Length();
> +  for (uint32_t i = length - 1; i >= 0; --i) {

i >=0 is always true; it's an unsigned type.  So this loop doesn't have a useful loop-termination condition.

You need to do:

    for (uint32_t i = length; i > 0; --i) {
      auto* element = mControls->mElements[i-1];
      
or something.  Or better yet:

    for (auto* element : Reversed(mControls->mElements)) {

::: dom/html/HTMLFormElement.cpp:1852
(Diff revision 1)
> +      if (element == aControl) {
> +        isLastActive = true;
> +      }

May be simpler to just do:

    return element == aControl;

and after the loop return false and drop isLastActive altogether.

Comment 8

[Jan Henning [:JanH]]

Comment on attachment 8914126 [details]
Bug 1398374 - Part 1 - Add helper method to test for the last non-disabled single line form input.

https://reviewboard.mozilla.org/r/185436/#review190770

> i >=0 is always true; it's an unsigned type.  So this loop doesn't have a useful loop-termination condition.
> 
> You need to do:
> 
>     for (uint32_t i = length; i > 0; --i) {
>       auto* element = mControls->mElements[i-1];
>       
> or something.  Or better yet:
> 
>     for (auto* element : Reversed(mControls->mElements)) {

Ouch - thanks for catching that.
Also didn't know about the `Reversed()` trick, sound like a neat idea indeed.

Comment 9

[Jan Henning [:JanH]]

Comment on attachment 8914126 [details]
Bug 1398374 - Part 1 - Add helper method to test for the last non-disabled single line form input.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185436/diff/1-2/

Comment 10

[Jan Henning [:JanH]]

Comment on attachment 8914127 [details]
Bug 1398374 - Part 2 - Send an IME actionHint other than "next" for the last input element of a form.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185438/diff/2-3/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ca2a39919f2d
Part 1 - Add helper method to test for the last non-disabled single line form input. r=bz
https://hg.mozilla.org/integration/autoland/rev/2bde2497dd3f
Part 2 - Send an IME actionHint other than "next" for the last input element of a form. r=masayuki

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ca2a39919f2d
https://hg.mozilla.org/mozilla-central/rev/2bde2497dd3f