876765 - Can't navigate to search suggestion row using d-pad

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Zhenshuo Fang (:fang) - Firefox UX Team]

After turned on the search suggestion, user can click on the search suggestions in awesome screen using the cursor but not the d-pad.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Factor out search view into SearchEngineRow

This is largely just a port of SearchEngineRow and its layouts from bug 871650, with minor tweaks like support for animations after an opt-in.

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Give buttons a focusable state in AwesomeScreen

Gives buttons a focus state, which is an orange border matching the URL bar focus state.

Comment 3

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Enable navigating to search suggestions with gamepad

Here's the main fix for this bug. It was pretty tricky to provide support for navigation within nested ListView items, but this seems to work.

ListView focus is handled internally, so we can't use the normal focus manipulation techniques (setNextFocusDownId(), requestFocus(), etc.) on the suggestion views. Instead, I intercepted key down events on the ListView and handed them to SearchEngineRow. SearchEngineRow then handles the key down events internally and sets the selected state for the corresponding suggestion view (using setDuplicateParentStateEnabled()). This also required keeping track of the active SearchEngineRow by listening for the ListView focus and item selected events, which is what the ListSelectionListener is for.

One minor feature this patch doesn't support is the ability to long-press suggestions to copy their text to the URL bar. Since there are higher priority gamepad bugs that need attention, I figured we could address this later.

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: Part 4: Fix search suggestion opt-in prompt focus behavior

Finally, we also need to fix focusing for the opt-in prompt. Currently, hitting d-pad down from the URL EditText skips over the prompt buttons.

Like patch 3, the parent is focusable and hands off focus to the child button when it gains focus. This patch is much simpler, however, since the prompt is not inside of a ListView, meaning we can just use requestFocus() to pass the focus to the child button.

I also set the nextFocusRight attribute on the "no" button to itself, so hitting right does nothing. Without it, clicking right in the suggestion prompt made the focus jump up to the URL bar's go button, which felt awkward.

Comment 5

[Lucas Rocha (:lucasr)]

Comment on attachment 767517 [details] [diff] [review]
Part 1: Factor out search view into SearchEngineRow

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

This will cause some pain to merge into fig...

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 767519 [details] [diff] [review]
Part 2: Give buttons a focusable state in AwesomeScreen

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

Assuming ibarlow approved it?

Comment 7

[Brian Nicholson (:bnicholson)]

(In reply to Lucas Rocha (:lucasr) from comment #6)
> Comment on attachment 767519 [details] [diff] [review]
> Part 2: Give buttons a focusable state in AwesomeScreen
> 
> Review of attachment 767519 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Assuming ibarlow approved it?

Didn't ask anyone directly, but I was following the UX suggestion in bug 876767 which is to use the orange border for highlights.

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 767525 [details] [diff] [review]
Part 3: Enable navigating to search suggestions with gamepad

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

Looks generally good but I'd like to see the final version of this patch (with suggested changes) before giving r+.

::: mobile/android/base/SearchEngineRow.java
@@ +170,5 @@
>      }
> +
> +    @Override
> +    public boolean onKeyDown(int keyCode, android.view.KeyEvent event) {
> +        View suggestion = mSuggestionView.getChildAt(mSelectedView);

final

@@ +172,5 @@
> +    @Override
> +    public boolean onKeyDown(int keyCode, android.view.KeyEvent event) {
> +        View suggestion = mSuggestionView.getChildAt(mSelectedView);
> +
> +        if (event.getAction() == android.view.KeyEvent.ACTION_DOWN) {

Early return if action != ACTION_DOWN instead?

@@ +173,5 @@
> +    public boolean onKeyDown(int keyCode, android.view.KeyEvent event) {
> +        View suggestion = mSuggestionView.getChildAt(mSelectedView);
> +
> +        if (event.getAction() == android.view.KeyEvent.ACTION_DOWN) {
> +            if (event.getKeyCode() == android.view.KeyEvent.KEYCODE_DPAD_RIGHT) {

Code would be a little less verbose if you used a switch here. Assign keycode to a variable in each 'if' (if you want to keep the if's here)

@@ +174,5 @@
> +        View suggestion = mSuggestionView.getChildAt(mSelectedView);
> +
> +        if (event.getAction() == android.view.KeyEvent.ACTION_DOWN) {
> +            if (event.getKeyCode() == android.view.KeyEvent.KEYCODE_DPAD_RIGHT) {
> +                View nextView = mSuggestionView.getChildAt(mSelectedView + 1);

final

@@ +179,5 @@
> +                if (nextView != null) {
> +                    suggestion.setDuplicateParentStateEnabled(false);
> +                    nextView.setDuplicateParentStateEnabled(true);
> +                    mSuggestionView.getChildAt(mSelectedView).refreshDrawableState();
> +                    nextView.refreshDrawableState();

Maybe you should factor our part of this code into a separate method so that you can avoid this code duplication when handling DPAD_RIGHT and DPAD_LEFT here?

@@ +183,5 @@
> +                    nextView.refreshDrawableState();
> +                    mSelectedView++;
> +                    return true;
> +                }
> +            } else if (event.getKeyCode() == android.view.KeyEvent.KEYCODE_DPAD_LEFT) {

Import KeyEvent so that you simply do KeyEvent.KEYCODE_DPAD_LEFT here. Same for the other key codes.

@@ +184,5 @@
> +                    mSelectedView++;
> +                    return true;
> +                }
> +            } else if (event.getKeyCode() == android.view.KeyEvent.KEYCODE_DPAD_LEFT) {
> +                View nextView = mSuggestionView.getChildAt(mSelectedView - 1);

final

@@ +209,5 @@
> +        mUserEnteredView.refreshDrawableState();
> +    }
> +
> +    public void onDeselected() {
> +        View suggestion = mSuggestionView.getChildAt(mSelectedView);

final

::: mobile/android/base/awesomebar/AllPagesTab.java
@@ +136,5 @@
>              AwesomeBarCursorAdapter adapter = getCursorAdapter();
>              list.setAdapter(adapter);
>              list.setOnTouchListener(mListListener);
> +
> +            ListSelectionListener listener = new ListSelectionListener();

final

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 767528 [details] [diff] [review]
Part 4: Fix search suggestion opt-in prompt focus behavior

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

Need more context to give r+ here.

::: mobile/android/base/resources/layout/awesomebar_suggestion_prompt.xml
@@ +45,4 @@
>                  android:textSize="14sp"
>                  android:layout_marginLeft="6dip"
>                  android:background="@drawable/suggestion_selector"
> +                android:nextFocusRight="@+id/suggestions_prompt_no"

The "No" buttons points to itself?

Comment 10

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Enable navigating to search suggestions with gamepad, v2

Addressed comments.

Comment 11

[Brian Nicholson (:bnicholson)]

Comment on attachment 767528 [details] [diff] [review]
Part 4: Fix search suggestion opt-in prompt focus behavior

(In reply to Lucas Rocha (:lucasr) from comment #9)
> The "No" buttons points to itself?

Yes, see comment 4.

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 772153 [details] [diff] [review]
Part 3: Enable navigating to search suggestions with gamepad, v2

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

Nice.

::: mobile/android/base/SearchEngineRow.java
@@ +171,5 @@
>      }
> +
> +    @Override
> +    public boolean onKeyDown(int keyCode, android.view.KeyEvent event) {
> +        final View suggestion = mSuggestionView.getChildAt(mSelectedView);

Is it safe to assume 'suggestion' will never be null?

Comment 13

[Lucas Rocha (:lucasr)]

Before I forget: these patches will have to be ported to fig. Please file a bug blocking bug 862793 to track that.

Comment 14

[Brian Nicholson (:bnicholson)]

(In reply to Lucas Rocha (:lucasr) from comment #12)
> ::: mobile/android/base/SearchEngineRow.java
> @@ +171,5 @@
> >      }
> > +
> > +    @Override
> > +    public boolean onKeyDown(int keyCode, android.view.KeyEvent event) {
> > +        final View suggestion = mSuggestionView.getChildAt(mSelectedView);
> 
> Is it safe to assume 'suggestion' will never be null?

Good catch. This could probably lead to a NPE if the suggestions are updated while being selected. I'll add a clamp here to make sure the selected view is within the child count.

Comment 15

[Brian Nicholson (:bnicholson)]

(In reply to Lucas Rocha (:lucasr) from comment #13)
> Before I forget: these patches will have to be ported to fig. Please file a
> bug blocking bug 862793 to track that.

Filed bug 894045.

Comment 16

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f6d0e525f96
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3cfb3b2751d
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ac444380f33
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d3f5418ae89

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5f6d0e525f96
https://hg.mozilla.org/mozilla-central/rev/f3cfb3b2751d
https://hg.mozilla.org/mozilla-central/rev/9ac444380f33
https://hg.mozilla.org/mozilla-central/rev/2d3f5418ae89