855431 - Make the awesomescreen viewpager sections focus-friendly

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Make it so that the gamepad action button actually goes to the URL that focus is on. Also make the history sections expandable because it makes it a lot easier to navigate to entries far below using a gamepad.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

I just made up the highlight color for the header row. Note that clicking on the header row on a touch device still has the same behaviour (i.e. it does nothing). On a gamepad where you have to button-mash to scroll it helps that the sections can be collapsed.

Comment 2

[Sriram Ramasubramanian [:sriram]]

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

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

Looks good to me. I'll r+ once ensuring the "select" behavior.

::: mobile/android/base/awesomebar/HistoryTab.java
@@ +94,5 @@
>              });
> +            list.setOnKeyListener(new View.OnKeyListener() {
> +                @Override public boolean onKey(View v, int keyCode, KeyEvent event) {
> +                    if (GamepadUtils.isActionKeyDown(event)) {
> +                        ExpandableListView expando = (ExpandableListView)v;

"expando" :P Feels like "desperado" ;)
Could you rename this to "view" or "list"?

@@ +95,5 @@
> +            list.setOnKeyListener(new View.OnKeyListener() {
> +                @Override public boolean onKey(View v, int keyCode, KeyEvent event) {
> +                    if (GamepadUtils.isActionKeyDown(event)) {
> +                        ExpandableListView expando = (ExpandableListView)v;
> +                        long selected = expando.getSelectedPosition();

Same question as in XML file: does focus change "select" a row?

@@ +104,5 @@
> +                        case ExpandableListView.PACKED_POSITION_TYPE_GROUP:
> +                            int group = ExpandableListView.getPackedPositionGroup(selected);
> +                            return (expando.isGroupExpanded(group)
> +                                ? expando.collapseGroup(group)
> +                                : expando.expandGroup(group));

Do we need this? Expanding and collapsing group on clicking on it? How do other apps respond to this?

::: mobile/android/base/resources/drawable/awesomebar_header_row.xml
@@ +4,5 @@
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<selector xmlns:android="http://schemas.android.com/apk/res/android">
> +    <item android:state_selected="true"
> +          android:drawable="@color/awesomebar_header_row_focused"/>

state_selected won't be set in "touch" mode. So, we should be good, for phones and tablets.
But do we actually "select" when the focus changes from one row to another?

Comment 3

[Kartikaya Gupta (email:[e-mail])]

(In reply to Sriram Ramasubramanian [:sriram] from comment #2)
> "expando" :P Feels like "desperado" ;)
> Could you rename this to "view" or "list"?

Sure. I thought this would make the code a little more exciting :)

> @@ +95,5 @@
> > +            list.setOnKeyListener(new View.OnKeyListener() {
> > +                @Override public boolean onKey(View v, int keyCode, KeyEvent event) {
> > +                    if (GamepadUtils.isActionKeyDown(event)) {
> > +                        ExpandableListView expando = (ExpandableListView)v;
> > +                        long selected = expando.getSelectedPosition();
> 
> Same question as in XML file: does focus change "select" a row?

Yes, for the expandable list, moving the focus around on via the d-pad actually changes the "selected" item. When I do a getFocusedView() call on the ExpandableListView it always returns null, so it appears that the focus just remains on the ExpandableListView itself and never propagates down to the child views.

> @@ +104,5 @@
> > +                        case ExpandableListView.PACKED_POSITION_TYPE_GROUP:
> > +                            int group = ExpandableListView.getPackedPositionGroup(selected);
> > +                            return (expando.isGroupExpanded(group)
> > +                                ? expando.collapseGroup(group)
> > +                                : expando.expandGroup(group));
> 
> Do we need this? Expanding and collapsing group on clicking on it? How do
> other apps respond to this?

We don't strictly need it, but it's a nice touch. That's the main reason to use an ExpandableListView in the first place. Note that at [1] we explicitly disable the expand/collapse behaviour on touch because we didn't want it, but as I said in comment 1 I like having it for gamepad because scrolling is just so much harder than on touch.

> > +<selector xmlns:android="http://schemas.android.com/apk/res/android">
> > +    <item android:state_selected="true"
> > +          android:drawable="@color/awesomebar_header_row_focused"/>
> 
> state_selected won't be set in "touch" mode. So, we should be good, for
> phones and tablets.
> But do we actually "select" when the focus changes from one row to another?

Yup, I tried using state_focused here as well and that didn't have the desired effect. The state changes to "selected" for these items when I navigate to them.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/awesomebar/HistoryTab.java#84

Comment 4

[Sriram Ramasubramanian [:sriram]]

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

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

This is all good. Have "expando" in there :)

Comment 5

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/70896d62e7d3

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/70896d62e7d3