Pressing <enter> on hardware keyboard does not perform search

RESOLVED FIXED in Firefox 40

Status

Firefox for Android Graveyard
Search Activity
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: toonetown, Assigned: toonetown)

Tracking

40 Branch
Firefox 40

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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?
(Assignee)

Comment 1

3 years ago
Created attachment 8598205 [details] [diff] [review]
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>
Assignee: nobody → nathan
Attachment #8598205 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 3

3 years ago
Created attachment 8598209 [details] [diff] [review]
bug-1158906.diff

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 4

3 years ago
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+
(Assignee)

Comment 5

3 years ago
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+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/3f2af35fb884
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/3f2af35fb884
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
Duplicate of this bug: 1172347
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+
status-firefox38.0.5: fixed → ---
status-firefox39: --- → fixed

Updated

4 months ago
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.