Closed Bug 1158906 Opened 9 years ago Closed 9 years ago

Pressing <enter> on hardware keyboard does not perform search

Categories

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

40 Branch
defect
Not set
normal

Tracking

(firefox39 fixed, firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: toonetown, Assigned: toonetown)

References

Details

Attachments

(1 file, 1 obsolete file)

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?
Attached patch bug-1158906.diff (obsolete) — Splinter Review
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>
Assignee: nobody → nathan
Attachment #8598205 - Flags: review?(margaret.leibovic)
Attached patch bug-1158906.diffSplinter Review
Updated patch to remove the unneeded import statement
Attachment #8598205 - Attachment is obsolete: true
Attachment #8598205 - Flags: review?(margaret.leibovic)
Attachment #8598209 - Flags: review?(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.
Attachment #8598209 - Flags: review?(nchen)
Attachment #8598209 - Flags: review?(margaret.leibovic)
Attachment #8598209 - Flags: feedback+
I know nothing of IME either...so I'm willing to take a different approach if needed.
Comment on attachment 8598209 [details] [diff] [review]
bug-1158906.diff

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

LGTM!
Attachment #8598209 - Flags: review?(nchen) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3f2af35fb884
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
Bug 1172347 found that this is broken on Fx38 (already shipped). Can we try to uplift this to Fx39?
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
Attachment #8598209 - Flags: approval-mozilla-beta?
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.
Attachment #8598209 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.