904122 - [fig] Scrolling on search suggestions page does not dismiss the keyboard

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

1) Launch Figgy Fennec
2) Click the url bar
3) Type some content to bring up the search suggestions page
4) Scroll through the list

Expected: Keyboard is dismissed (like any other time on the awesomescreen when the url bar is selected)
Actual: Keyboard is not dismissed

Note that the back button will still dismiss the keyboard

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Patch

I wanted to place requestFocus() in an if for the down event...

if (event.getActionMasked() == MotionEvent.ACTION_DOWN) {
    requestFocus();
}
return false;

But for some reason, the search engine rows do not receive a DOWN event (though history rows and the empty background space does). Would you happen to know why?

Comment 2

[Lucas Rocha (:lucasr)]

Comment on attachment 789695 [details] [diff] [review]
Patch

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

::: mobile/android/base/home/BrowserSearch.java
@@ +248,5 @@
>                      return selected.onKeyDown(keyCode, event);
>                  }
>                  return false;
>              }
>          });

nit: add empty line here.

@@ +252,5 @@
>          });
> +        mList.setOnTouchListener(new View.OnTouchListener() {
> +            @Override
> +            public boolean onTouch(View v, MotionEvent event) {
> +                v.requestFocus(); // to drop soft keyboard.

to drop -> Dismiss

nit: move this comment before this line. We don't usually add comments on the same line.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 01a: Patch

As per IRC, I moved the call to onInterceptTouchEvent.

I created a new class extending HomeListView for this so I wasn't reduplicating any of the work for dismissing the keyboard that the HomePager does (since it also uses HomeListView).

Is it worth making a comment in the code about this or is the patch sufficient?

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 790519 [details] [diff] [review]
01a: Patch

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

Looks good (with suggested change).

::: mobile/android/base/home/HomeSearchListView.java
@@ +13,5 @@
> +
> +/**
> + * HomeSearchListView is a list view for displaying search engine results on the awesome screen.
> + */
> +public class HomeSearchListView extends HomeListView {

This is not big enough to deserve its own file, imo. This is very specific to BrowserSearch anyway. I'd turn this into a public inner static class in BrowserSearch.

@@ +27,5 @@
> +    public boolean onInterceptTouchEvent(MotionEvent event) {
> +        if (event.getActionMasked() == MotionEvent.ACTION_DOWN) {
> +            // Dismiss the soft keyboard.
> +            requestFocus();
> +        }

nit: add empty line.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 01b: Patch

Fix nits, move r+.

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 790828 [details] [diff] [review]
01b: Patch

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

::: mobile/android/base/resources/layout/browser_search.xml
@@ +12,5 @@
>                android:layout_width="match_parent"
>                android:layout_height="wrap_content"
>                android:layout="@layout/home_suggestion_prompt" />
>  
> +    <org.mozilla.gecko.home.HomeSearchListView

You have to update this to something like:
<view class="org.mozilla.gecko.home.BrowserSearch$HomeSearchListView"
 ...

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 01c: Patch

Whoops! Sorry! x_x

Not moving my r+ this time!

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 790837 [details] [diff] [review]
01c: Patch

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

Cool.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/projects/fig/rev/ed952cface7e

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ed952cface7e