762968 - tapping on search suggestions in awesomescreen takes two taps

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Joe Drew (not getting mail)]

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.

Comment 1

[Brian Nicholson (:bnicholson)]

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?

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Only apply keyboard focus hack to hardware keyboards

The onKeyDown() hack was introduced in bug 697773 to fix hardware keyboard input; we should only need to use this on those phones.

Comment 3

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Replace onInterceptTouchEvent() with individual listeners

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.

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Ignore bogus stylus events in onKeyDown()

Alternate approach - rather than only applying the HKB hack to HKB phones, this simply consumes the Note's stylus event.

Comment 5

[Chris Peterson [:cpeterson]]

Comment on attachment 636925 [details] [diff] [review]
Part 1: Ignore bogus stylus events in onKeyDown()

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

LGTM

Comment 6

[Brian Nicholson (:bnicholson)]

(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.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch

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....

Comment 8

[Brian Nicholson (:bnicholson)]

Attachment: alt fix: Prevent hiding keyboard if suggestion view is clicked

Also not a pretty fix, but at least this one keeps a single touch listener.

Comment 9

[Chris Peterson [:cpeterson]]

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 10

[Wesley Johnston (:wesj)]

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.

Comment 11

[Wesley Johnston (:wesj)]

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.

Comment 12

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/14bd687b39b2
http://hg.mozilla.org/integration/mozilla-inbound/rev/93aa6874678d

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/14bd687b39b2
https://hg.mozilla.org/mozilla-central/rev/93aa6874678d