856715 - Map L2/R2 to next/previous tab

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

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

Attachment: baby WIP

Pushing L2/R2 should open the tab sidebar if it's not already open, and cycle the focus through tabs forwards/backwards. Once the user reaches the desired tab they should be able to hit the action button to select the tab. Bonus points if we do some smart thing wrt closing the tab tray once a tab is picked.

I think this will be much easier once bug 854458 is in as that seems to add all the relevant code for moving around focus in the tabs tray.

Comment 1

[Shilpan Bhagat]

Attachment: Patch one for using l2/r2 + up/down to navigate tabs tray and select/delete

-L2/R2 to open and navigate tabs
-up/down dpad to do the same + spatial navigation on tabs tray

Additional info:
-moving focus to tabstray for initial navigation on opening tabspanel

Points of discussion:
-Whether crucial key events should be handled in the TabsTray or TwoWayView. Initially I sought to do it through TwoWayView but the way TabsTray handles clicks is kind of hacky i.e. it listens for touch events and if there is one with no gestures, it calls that as a click. Which means no listeners - hence you cannot perform any actions using keyevents either. The workaround for this would be to create an ActionButtonListener in TwoWayView which listens for the action button and calls an 'OnClick' on the selected view.

-The above solution, although hacky, works. In this situation we would still prefer that when the tab opens up, the focus is on one of the rows (initially the first row? or maybe selected row?). What would be the best way to focus on a particular view in the TabsTray? (Selection is different from Focus). Since we use selection it was a bit confusing to see to selection and Focus on the Tray. The current code takes care of that. Although we should discuss that as well.

Comment 2

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

Comment on attachment 765453 [details] [diff] [review]
Patch one for using l2/r2 + up/down to navigate tabs tray and select/delete

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

I took a quick look at the patch; it looks pretty good! Some comments below.

::: mobile/android/base/TabsTray.java
@@ +153,5 @@
> +                mTabsAdapter.removeTab(tab);
> +                return true;
> +        }
> +        super.onKeyDown(keyCode, event);
> +        return false;

This should probably return super.onKeyDown, rather than returning false.

@@ +163,5 @@
>          private boolean mIsPrivate;
>          private ArrayList<Tab> mTabs;
>          private LayoutInflater mInflater;
>          private Button.OnClickListener mOnCloseClickListener;
> +        private int currentSelectedIndex;

Private class variables should start with 'm', so this should be mCurrentSelectedIndex.

@@ +210,5 @@
>              }
>          }
>  
> +        private boolean selectNext() {
> +            if(currentSelectedIndex < getCount() - 1) {

nit: space before '('

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 765453 [details] [diff] [review]
Patch one for using l2/r2 + up/down to navigate tabs tray and select/delete

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

Pretty good stuff! Still needs some cleanups though. And this patch probably keyboard navigation inside the TabsTray widget.

::: mobile/android/base/BrowserApp.java
@@ +269,5 @@
>                      Tabs.getInstance().getSelectedTab().doForward();
>                      return true;
> +                case KeyEvent.KEYCODE_BUTTON_R2:
> +                case KeyEvent.KEYCODE_BUTTON_L2:
> +                    //Show Tabs Panel

nit: space between // and "Show"

::: mobile/android/base/TabsPanel.java
@@ +226,5 @@
>          }
>      }
>  
>      // Tabs Panel Toolbar contains the Buttons
> +    public static class TabsPanelToolbar extends LinearLayout

Unrelated change, please move this to a separate patch.

@@ +251,5 @@
>          public void onDetachedFromWindow() {
>              super.onDetachedFromWindow();
>              mActivity.getLightweightTheme().removeListener(this);
>          }
> +

Unrelated change.

@@ +330,5 @@
>              }
>          }
>      }
>  
> +    public void setFocus() {

Maybe rename this to something more explicit? setfocus() feels a bit too generic. Maybe requestFocusOnCurrentPanel()?

@@ +331,5 @@
>          }
>      }
>  
> +    public void setFocus() {
> +        mPanel.getLayout().requestFocus();

It's probably not safe to assume mPanel is not null at any given time. So, do a null check before calling requestFocus().

::: mobile/android/base/TabsTray.java
@@ +136,5 @@
> +    public boolean onKeyDown(int keyCode, KeyEvent event) {
> +        switch (keyCode) {
> +            case KeyEvent.KEYCODE_DPAD_DOWN:
> +            case KeyEvent.KEYCODE_BUTTON_L2:
> +                return mTabsAdapter.selectNext();

nit: add empty lines between "case" blocks

@@ +147,5 @@
> +                return true;
> +            case KeyEvent.KEYCODE_BACK:
> +                Tabs tabs = Tabs.getInstance();
> +                Tab tab = (Tab)this.getItemAtPosition(mTabsAdapter.getCurrentSelectedIndex());
> +                tabs.closeTab(tab);

This will cause the tab to be removed without any animation. We probably want to use the same animation than when you tap on the close button. So, you'll probably have to use animateClose() somewhere here. Have a look at close button click listener to get an idea of how you could implement this here.

@@ +149,5 @@
> +                Tabs tabs = Tabs.getInstance();
> +                Tab tab = (Tab)this.getItemAtPosition(mTabsAdapter.getCurrentSelectedIndex());
> +                tabs.closeTab(tab);
> +
> +                mTabsAdapter.removeTab(tab);

This is not necessary. tabs.closeTab() will trigger TabsTray's listener (see onTabChanged) to call this for you.

@@ +209,5 @@
>                      break;
>              }
>          }
>  
> +        private boolean selectNext() {

I'd probably rename this to selectNextTab() for clarity.

@@ +210,5 @@
>              }
>          }
>  
> +        private boolean selectNext() {
> +            if(currentSelectedIndex < getCount() - 1) {

nit: I tend to prefer early returns over enclosing the whole method in an if. Makes the code easier to follow. So, I'd go with:

if (currentSelectedIndex == getCount() - 1) {
    return false;
}

// same code.

@@ +212,5 @@
>  
> +        private boolean selectNext() {
> +            if(currentSelectedIndex < getCount() - 1) {
> +                TabsTray.this.setItemChecked(currentSelectedIndex + 1, true);
> +                TabsTray.this.setSelection(currentSelectedIndex + 1);

The code would probably look a bit simpler if you uncheck previous mCurrentSelectedIndex and increment mCurrentSelectedIndex first, then use it everywhere in the method. Instead of using "currentSelectedIndex + 1" everywhere.

@@ +221,5 @@
> +            }
> +            return false;
> +        }
> +
> +        private boolean selectPrevious() {

selectPrevious() -> selectPreviousTab()

selectPrevious() and selectNext() are pretty much the same code with different values. This code could be refactored into a selectPosition(int pos) then selectNext() would simply do selectPosition(mCurrentSelectedIndex + 1) and selectPrevious() would be selectPosition(mCurrentSelectedIndex - 1).

@@ +222,5 @@
> +            return false;
> +        }
> +
> +        private boolean selectPrevious() {
> +            if(currentSelectedIndex > 0) {

Same here. Early return to make it easier to follow.

@@ +227,5 @@
> +                TabsTray.this.setItemChecked(currentSelectedIndex - 1, true);
> +                TabsTray.this.setSelection(currentSelectedIndex - 1);
> +
> +                TabsTray.this.setItemChecked(currentSelectedIndex, false);
> +                currentSelectedIndex--;

Same comment than selectNext(). Uncheck previous selected index and decrement mCurrentSelectedIndex first, then simply use mCurrentSelectedIndex everywhere.

@@ +258,5 @@
>          }
>  
>          // Updates the selected position in the list so that it will be scrolled to the right place.
>          private void updateSelectedPosition() {
> +            currentSelectedIndex = getPositionForTab(Tabs.getInstance().getSelectedTab());

I wonder if you should just add a method getCurrentSelectedIndex() that calls getPositionForTab(...) as above instead of adding a separate attribute. We better have a single source of selection state. Bonus: you wouldn't need to care about keeping tab selection state in sync with mCurrentSelectedIndex.

@@ +333,5 @@
>                  row = (TabRow) convertView.getTag();
>              }
>  
>              Tab tab = mTabs.get(position);
>              assignValues(row, tab);

Unrelated change.

::: mobile/android/base/resources/layout/tabs_item_cell.xml
@@ -4,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <Gecko.TabRow xmlns:android="http://schemas.android.com/apk/res/android"
>                style="@style/TabsItem"
> -              android:focusable="true"

I suspect this change will break keyboard navigation inside the tab cell. Keyboard navigation is not there yet but it would be nice to avoid changes that will cause us problems later. The idea is that you should be able to move focus from the whole item to its inner close button and vice-versa. See how the TabsItem style defined the focus order here.

Why was this change necessary in the first place? So that the children of TabsTray don't steal focus?

::: mobile/android/base/resources/layout/tabs_item_row.xml
@@ -4,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <Gecko.TabRow xmlns:android="http://schemas.android.com/apk/res/android"
>                style="@style/TabsItem"
> -              android:focusable="true"

Same here.

Comment 4

[Wesley Johnston (:wesj)]

Comment on attachment 765453 [details] [diff] [review]
Patch one for using l2/r2 + up/down to navigate tabs tray and select/delete

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

I think lucas has this covered pretty well.

Comment 5

[Shilpan Bhagat]

Attachment: Patch for l2/r2 navigation + action button press selection on thumbnails

New patch, navigation of tabstray using l2/r2 + action button press to launch a tab. 

Had to write a action button clicklistener for selecting tabs since the current way of recognizing click is in onTouchListener which does not work for key presses. It currently only supports the 'o' button for Ouya but can be extended to work with 'Enter' as well.

Comment 6

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

Comment on attachment 770003 [details] [diff] [review]
Patch for l2/r2 navigation + action button press selection on thumbnails

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

LGTM with nits addressed.

::: mobile/android/base/TabsTray.java
@@ +146,5 @@
>          private boolean mIsPrivate;
>          private ArrayList<Tab> mTabs;
>          private LayoutInflater mInflater;
>          private Button.OnClickListener mOnCloseClickListener;
> +        private int mSelected = 0;

No need to initialize this to 0, that happens automatically in Java.

@@ +214,2 @@
>              for (int i=0; i < getCount(); i++)
> +                 TabsTray.this.setItemChecked(i, (i == mSelected));

This line is over-indented. Might as well fix it while you're here.

@@ +283,5 @@
>              } else {
>                  row = (TabRow) convertView.getTag();
>              }
>  
> +            if(position == mSelected) {

nit: space between if and (

::: mobile/android/base/widget/TwoWayView.java
@@ +2607,5 @@
>                  break;
> +
> +            case KeyEvent.KEYCODE_BUTTON_A:
> +                final View child = getChildAt(mSelectedPosition - mFirstPosition);
> +                mOnActionButtonClickListener.onActionButtonClick(child);

Check if mOnActionButtonClickListener is null here. Assuming it's non-null means this code is tightly coupled to other bits of code.

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 770003 [details] [diff] [review]
Patch for l2/r2 navigation + action button press selection on thumbnails

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

This change should not be directly in TwoWayView. TwoWayView is a standalone project (https://github.com/lucasr/twoway-view). We should be careful with any API changes in it. It would be better to simply add a key listener whenever you need this behaviour?

::: mobile/android/base/TabsTray.java
@@ +216,3 @@
>  
> +            if (mSelected != -1)
> +                TabsTray.this.setSelection(mSelected);

nit: Add {}'s

Comment 8

[Shilpan Bhagat]

Attachment: l2r2tabspatch.diff

Made changes based on the above comments

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 783933 [details] [diff] [review]
l2r2tabspatch.diff

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

Looks good overall, but the focus handling part needs work.

::: mobile/android/base/TabsTray.java
@@ +310,5 @@
>                  // the close animation. Remove any of those properties.
>                  resetTransforms(convertView);
>              }
>  
> +            if (position == mSelected) {

Hmm, this is likely to break keyboard navigation because the tray will steal focus from, say, the tabs panel toolbar when you scroll. TwoWayView will move focus to the selected child once it receives focus. Double-check if that's the case. If it's not, you can simply add a focus listeners on the tabstray view and call requestFocus() on the selected tab accordingly.

::: mobile/android/base/widget/TwoWayView.java
@@ +2503,5 @@
>          final int action = event.getAction();
>  
>          if (action != KeyEvent.ACTION_UP) {
>              switch (keyCode) {
> +            case KeyEvent.KEYCODE_BUTTON_L2:

Please submit a pull request for TwoWayView (https://github.com/lucasr/twoway-view) with these changes once we're done with the review here.

Comment 10

[Shilpan Bhagat]

(In reply to Lucas Rocha (:lucasr) from comment #9)
> Comment on attachment 783933 [details] [diff] [review]
> l2r2tabspatch.diff
> 
> Review of attachment 783933 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good overall, but the focus handling part needs work.
> 
> ::: mobile/android/base/TabsTray.java
> @@ +310,5 @@
> >                  // the close animation. Remove any of those properties.
> >                  resetTransforms(convertView);
> >              }
> >  
> > +            if (position == mSelected) {
> 
> Hmm, this is likely to break keyboard navigation because the tray will steal
> focus from, say, the tabs panel toolbar when you scroll. TwoWayView will
> move focus to the selected child once it receives focus. Double-check if
> that's the case. If it's not, you can simply add a focus listeners on the
> tabstray view and call requestFocus() on the selected tab accordingly.
> 
It won't break because, according to the API atleast :)

http://developer.android.com/reference/android/widget/Adapter.html#getView%28int,%20android.view.View,%20android.view.ViewGroup%29

But I agree onfocuschangelistener is a better way to go. updated patch coming up

Comment 11

[Shilpan Bhagat]

As I was looking into this, I had a few things I thought I should tell you as to why I did not go for the onfocuschangelistener in the first place. Based on the description given here, we want to shift the focus to the selected view as soon as possible so that we can move around the focus with the arrow/l2-r2 keys. Now if we click on the button, the current focus stays on the button and not on the tabs panel. We can perhaps try to steal the focus on the tabs panel which will then have to give its focus onto the current tabs tray which then goes onto giving its focus to the selected child.

I thought this was a little too complicated. And the API getView() gave a very reliable way of stealing the focus without breaking anything. Do you think I should go for the more elaborate manner that I described here?

Comment 12

[Shilpan Bhagat]

On the comment above

Comment 13

[Lucas Rocha (:lucasr)]

(In reply to Shilpan Bhagat from comment #11)
> As I was looking into this, I had a few things I thought I should tell you
> as to why I did not go for the onfocuschangelistener in the first place.
> Based on the description given here, we want to shift the focus to the
> selected view as soon as possible so that we can move around the focus with
> the arrow/l2-r2 keys. Now if we click on the button, the current focus stays
> on the button and not on the tabs panel. We can perhaps try to steal the
> focus on the tabs panel which will then have to give its focus onto the
> current tabs tray which then goes onto giving its focus to the selected
> child.

Not sure I follow. What do you mean by "click on the button" here? Which button?

Comment 14

[Shilpan Bhagat]

I meant the tabs button. What I mean was that, in order to use l2/r2 for navigating tabs once it's opened, we need the focus to be on selected tab when we open the tabs panel. The way I did it was stealing the focus to the selected tab in the tabs tray on getView(...). 

The round about way of doing this would be to to steal the focus on the tabs panel which will then have to give its focus onto the current tabs tray which then goes onto giving its focus to the selected child.

Neither of the methods would break.

Comment 15

[Lucas Rocha (:lucasr)]

(In reply to Shilpan Bhagat from comment #14)
> The round about way of doing this would be to to steal the focus on the tabs
> panel which will then have to give its focus onto the current tabs tray
> which then goes onto giving its focus to the selected child.

This option is probably more robust than using getView() for stealing input focus.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

Resetting assignee

Comment 17

[:Margaret Leibovic]

Do we still care about gamepad buttons?

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to :Margaret Leibovic from comment #17)
> Do we still care about gamepad buttons?

It's possible we still care, but it's not a priority. Some second screen opportunities might involve gamepads.