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

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.

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

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.

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

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

Addressed comments.

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

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

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

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

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

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