1474902 - Gecko doesn't report the correct IME action when in an HTML form field

Status: RESOLVED FIXED

(GeckoView :: IME, defect, P3)

Description

[Imanol Fernandez]

Chrome correctly reports IME action as IME_ACTION_NEXT if there are more fields but Gecko reports IME_ACTION_DONE.

Example to reproduce it: https://www.w3schools.com/htmL/html_forms.asp

Info from m_kato: Hmm, https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chromium/content/browser/input/ImeUtils.java?q=IME_ACTION_NEXT&sq=package:chromium&g=0&l=146 is chrome implementation.  If there is other input element that is focusable, we should add it for IME's hint.

Comment 1

[Chris Peterson [:cpeterson]]

Jim says P3 and this more of a platform issue than GeckoView.

Comment 2

[Jan Henning [:JanH]]

Unless something regressed since I last looked at this, for us the fact that a form has a default submit element (which I think is the case for those w3schools examples) takes precedence in determining the IME hint to use:
https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/dom/events/IMEStateManager.cpp#1340

Comment 3

[Jan Henning [:JanH]]

(In reply to Jan Henning [:JanH] from comment #2)
> [...] for us the fact that
> a form has a default submit element (which I think is the case for those
> w3schools examples) takes precedence in determining the IME hint to use:

Which might come from mimicking Desktop: There, pressing Enter in any of the text input fields will submit the form, and with mobile IMEs often the only way to press Enter is when receiving an action hint of IME_ACTION_DONE.

Comment 4

[Manuel Martin]

Any idea when this is going to be addressed?

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Makoto, do you have ideas on this?

Comment 6

[Makoto Kato [:m_kato]]

We may need to set "next" to HTMLInputInputMode when calling SetInputContext from IMEStateManager.  So IMEStateManager should set "next" to aInputContext.mHTMLInputInputMode even if content has submit button.

https://searchfox.org/mozilla-central/rev/39cb1e96cf97713c444c5a0404d4f84627aee85d/dom/events/IMEStateManager.cpp#1332 doesn't consider this situation.

Comment 7

[Makoto Kato [:m_kato]]

jchen, are you working this?  Since this bug is IMEStateManager in dom/event, I can take this.

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

Sounds good. Thanks!

Comment 9

[Makoto Kato [:m_kato]]

nsFocusManager doesn't export a method that gets next focusable content....

Comment 10

[Makoto Kato [:m_kato]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf5aa0358d6774d7f2cfc2ff6aae8662e99aed21

Comment 11

[Makoto Kato [:m_kato]]

Attachment: Bug 1474902 - Part 1 . Set event time when translating to TAB key

Since event time isn't set when translating ENTER key to TAB key, comparing
event time hits an assertion.  So event time should be set.

Comment 12

[Makoto Kato [:m_kato]]

Attachment: Bug 1474902 - Part 2. Make nsFocusManager::DetermineElementToMoveFocus public. r?NeilDeakin

Actually, there is no public method to get next element/content by tabIndex or
TAB key.  I would like to use GetNextTabbableContent from IMEStateManager.

Depends on D12883

Comment 13

[Makoto Kato [:m_kato]]

Attachment: Bug 1474902 - Part 4. Set hint to next when next focusable element is input element that is text control.

Currently, IMEStateManager::SetIMEState sets hint to the following logic.

- If there is no submit button into form element, set "next"
- If there is submit button, set "search" or "go"
- If there isn't into form element, no hint.

But Chrome sets "next" hint when next focusable element is input that is text
control. So even if there is submit button into form element, we should set
"next" to hint when next focusable element is input that is text controll.

Also, there is no way to add unit test since hint isn't exposed to script.
Do you have any idea for test?

Depends on D12884

Comment 16

[Makoto Kato [:m_kato]]

Attachment: Bug 1474902 - Part 3. Move setting action hint to static function

To make setting action hint simple, I would like to move it to static function.

Comment 17

[Makoto Kato [:m_kato]]

Attachment: Bug 1474902 - Part 5. Add mochitest

Comment 18

[Chris Peterson [:cpeterson]]

Will we want to uplift this IME fix to GV 67 Beta for Fenix MVP? Since this is not a recent regression, maybe we don't need to fix it in Fenix MVP.

Comment 19

[Makoto Kato [:m_kato]]

(In reply to Chris Peterson [:cpeterson] from comment #18)

Will we want to uplift this IME fix to GV 67 Beta for Fenix MVP? Since this is not a recent regression, maybe we don't need to fix it in Fenix MVP.

This is not regression. This is an improvement for software keyboard. So it is unnecessary to uplift to beta.

Comment 20

[Chris Peterson [:cpeterson]]

(In reply to Makoto Kato [:m_kato] from comment #19)

This is not regression. This is an improvement for software keyboard. So it is unnecessary to uplift to beta.

Thanks. In that case, I will set status flag 67=wontfix.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c54533bd0e04
Part 1. Set event time when translating to TAB key r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/05cee469e93e
Part 2. Make nsFocusManager::DetermineElementToMoveFocus public. r=NeilDeakin
https://hg.mozilla.org/integration/mozilla-inbound/rev/1848a0f1a982
Part 3. Move setting action hint to static function. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/f190032bbb2e
Part 4. Set hint to next when next focusable element is input element that is text control. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/98305e6fa0d6
Part 5. Add mochitest. r=masayuki

Comment 22

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/c54533bd0e04
https://hg.mozilla.org/mozilla-central/rev/05cee469e93e
https://hg.mozilla.org/mozilla-central/rev/1848a0f1a982
https://hg.mozilla.org/mozilla-central/rev/f190032bbb2e
https://hg.mozilla.org/mozilla-central/rev/98305e6fa0d6

Comment 23

[Chris Peterson [:cpeterson]]

Moving some IME bugs to the new GeckoView::IME component.