Closed Bug 762968 Opened 8 years ago Closed 8 years ago

tapping on search suggestions in awesomescreen takes two taps

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 16
Tracking Status
firefox15 --- unaffected
firefox16 --- verified

People

(Reporter: joe, Assigned: bnicholson)

References

Details

Attachments

(4 files, 1 obsolete file)

When I search, I now get search suggestions (yay!) However, when I tap on that, the first tap dismisses the keyboard, and the *second* tap actually navigates to the search suggestion. It should work with one tap.
tracking-fennec: --- → ?
OS: Mac OS X → Android
Hardware: x86 → ARM
Removing the hideSoftInput() call in onInterceptTouchEvent() seems to fix this (but the keyboard stays up). But I don't know why hiding the soft input would interfere with children receiving events.

wesj, cpeterson: any ideas?
OS: Android → Mac OS X
Hardware: ARM → x86
OS: Mac OS X → Android
Hardware: x86 → ARM
Assignee: nobody → bnicholson
The onKeyDown() hack was introduced in bug 697773 to fix hardware keyboard input; we should only need to use this on those phones.
Attachment #636906 - Flags: review?(cpeterson)
As mentioned in comment 1, the hideSoftInput() call in onInterceptTouchEvent() seemed to be the cause of this behavior.

Here is what I *think* is going on:
- onInterceptTouchEvent() is called, hiding the keyboard before the touch events are sent to the suggestion view.
- Since the hiding keyboard resizes the AwesomeScreen view, this requires that it be updated
- This, in turn, calls our getView() method that populates the suggestion view and assigns them click handlers
- Since the view/handlers are recreated during the touch event, their touch state is lost

Replacing onInterceptTouchEvent() with individual handlers (i.e., reverting bug 750511) seems to fix this problem. This apparently causes the keyboard to be hidden *after* the touch events, avoiding the view creation issues mentioned above.

Bug 750511 was landed to fix issues with Galaxy Note stylus prematurely hiding the keyboard. The stylus triggers an onKeyDown() event when near the screen; we then give focus to the EditText. This triggered the onFocusChanged() listener Wes mentioned at https://bugzilla.mozilla.org/show_bug.cgi?id=750511#c13, which hides the keyboard. By making sure the onKeyDown() code is only fired for phones with hardware keyboards, the EditText won't be focused, onFocusChanged() won't be called, and the keyboard won't be hidden.

I've tested these patches on a Galaxy Note, Droid RAZR, and the Android emulator with a hardware keyboard.
Attachment #636917 - Flags: review?(wjohnston)
Alternate approach - rather than only applying the HKB hack to HKB phones, this simply consumes the Note's stylus event.
Attachment #636906 - Attachment is obsolete: true
Attachment #636906 - Flags: review?(cpeterson)
Attachment #636925 - Flags: review?(cpeterson)
Comment on attachment 636925 [details] [diff] [review]
Part 1: Ignore bogus stylus events in onKeyDown()

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

LGTM
Attachment #636925 - Flags: review?(cpeterson) → review+
(In reply to Brian Nicholson (:bnicholson) from comment #3)
> Replacing onInterceptTouchEvent() with individual handlers (i.e., reverting
> bug 750511) seems to fix this problem. This apparently causes the keyboard
> to be hidden *after* the touch events, avoiding the view creation issues
> mentioned above.

Correction - without onInterceptTouchEvent(), the keyboard isn't hidden at all when touching the individual suggestion items. I guess this is because we aren't touching the allPagesList view itself.
Attached patch PatchSplinter Review
I'm not a big fan of this patch, mostly cause I hate having this many listeners. I hate that every search term has its own click listener. But Android makes this hard(er, compared to html at least).

I have an alternative for you. Tell me if you hate it, and maybe we can come to some sort of consensus....
Attachment #637233 - Flags: review?(bnicholson)
Also not a pretty fix, but at least this one keeps a single touch listener.
Attachment #637285 - Flags: review?(wjohnston)
Comment on attachment 637285 [details] [diff] [review]
alt fix: Prevent hiding keyboard if suggestion view is clicked

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

Brian, you might consider extracting this complicated code into a helper function. Maybe something like `FlowLayout getSuggestionView()` and/or `boolean suggestionViewContains(MotionEvent event)`.
Comment on attachment 636917 [details] [diff] [review]
Part 2: Replace onInterceptTouchEvent() with individual listeners

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

I hate this least.
Attachment #636917 - Flags: review?(wjohnston) → review+
Attachment #637233 - Flags: review?(bnicholson)
Comment on attachment 637285 [details] [diff] [review]
alt fix: Prevent hiding keyboard if suggestion view is clicked

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

Lets take the other guy.
Attachment #637285 - Flags: review?(wjohnston)
https://hg.mozilla.org/mozilla-central/rev/14bd687b39b2
https://hg.mozilla.org/mozilla-central/rev/93aa6874678d
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Status: RESOLVED → VERIFIED
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.