1158906 - Pressing <enter> on hardware keyboard does not perform search

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Search Activity, defect)

Description

[Nathan Toone [:toonetown]]

When searching using the search widget, the software keyboard shows a "search" key instead of an "enter" key.  However, performing the same search with a hardware keyboard (for example, in the android emulator with hardware keyboard enabled), the action that gets sent is an "enter" only.

I believe that this is due to the listener only looking for EditorInfo.IME_ACTION_SEARCH on line ~87 of org.mozilla.search.autocomplete.SearchBar.java.  I don't know what the input system is actually sending when you press the hardware <enter> key - looking at the list of IME_ACTION_* in the android docs, I would guess it's probably IME_ACTION_UNSPECIFIED.

I am not very familiar with how the android input system works, but perhaps it could be fixed by adding some kind of IME_MASK_ACTION on the enter key?

Comment 1

[Nathan Toone [:toonetown]]

Attachment: bug-1158906.diff

Patch which changes the logic so that it checks for the software keyboard search action (EditorInfo.IME_ACTION_SEARCH) as well as the "default" action which is sent by hardware keyboards (EditorInfo.IME_ACTION_UNSPECIFIED).

This makes the emulator's hardware keyboard work to perform searches by pressing <enter>

Comment 2

[Nathan Toone [:toonetown]]

Try server https://treeherder.mozilla.org/#/jobs?repo=try&revision=3df3b02edd09

Comment 3

[Nathan Toone [:toonetown]]

Attachment: bug-1158906.diff

Updated patch to remove the unneeded import statement

Comment 4

[:Margaret Leibovic]

Comment on attachment 8598209 [details] [diff] [review]
bug-1158906.diff

This looks reasonable to me, but jchen knows more about IME, so I think he should take a look as well.

Comment 5

[Nathan Toone [:toonetown]]

I know nothing of IME either...so I'm willing to take a different approach if needed.

Comment 6

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

Comment on attachment 8598209 [details] [diff] [review]
bug-1158906.diff

Review of attachment 8598209 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/3f2af35fb884

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/3f2af35fb884

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Bug 1172347 found that this is broken on Fx38 (already shipped). Can we try to uplift this to Fx39?

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8598209 [details] [diff] [review]
bug-1158906.diff

Approval Request Comment
[Feature/regressing bug #]: bug 1172347
[User impact if declined]: Broken <enter> for physical keyboards
[Describe test coverage new/current, TreeHerder]: Patch has been on Fx40 for a while and has no regressions
[Risks and why]: Low risk change with a narrow code-change scope
[String/UUID change made/needed]:none

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8598209 [details] [diff] [review]
bug-1158906.diff

Approved for uplift to beta so that search will work on phones with physical keyboards. Recent regression.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/6e88bd2e3743

Comment 14

[Nick Thomas [:nthomas] (UTC+12)]

http://hg.mozilla.org/releases/mozilla-beta/rev/fafac0aeafd0