817732 - Apply large tablet UI to smaller tablets like the Nexus 7

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ian Barlow (:ibarlow)]

People are often confused by our current 7" tablet UI, not understanding how to "Switch to the tablet UI" they have heard about. Even though the decisions we have made are deliberate, it would seem there is an expectation -- even on smaller tablets -- that an app experience feel more tablet-like than phone-like. 

So based on user feedback we've been getting, as well as general trends in small tablet UI design we are seeing, let's take a look adjusting our small tablet UI to more closely reflect its larger tablet counterparts. 

Mockup http://cl.ly/image/0e230k3Q2V1p

* Tab button on left
* Tab sidebar in landscape mode
* Tabs on top in portrait mode
* Allow 1 more action bar icon on top (Bookmark)

Comment 1

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

I'd like to hold off on these changes until we get the rest of the UI changes settled. This change is also 180deg different than bug 817735, where we do the opposite for horz/vert scrolling.

Comment 2

[Lucas Rocha (:lucasr)]

I'll be working on this from now on. I'll start with the horizontal listview/gridview changes in tabs tray.

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: Add "TwoWay" views

For now this is just the GridView. Still working on the TwoWayListView, which is probably what we'll actually use.

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: Change TabsTray to use TwoWayGridView

I'll replace with TwoWayListView once I finish it. This is working fine for now.

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: Add support for width animations in PropertyAnimator

Comment 6

[Lucas Rocha (:lucasr)]

Attachment: Vertical swipe support in TabsTray

Comment 7

[Lucas Rocha (:lucasr)]

Attachment: WIP of initial layout changes

Just to give an idea of where things are heading. This is still a bit rough and misses the dynamic sidebar state for tabs panel.

Comment 8

[Lucas Rocha (:lucasr)]

Attachment: Add "TwoWay" views to repo

This is not really for review. I'll be using this view temporarily while I finish implementing my custom view with two way scrolling support. The plan is to implement my custom view using the exact same API than TwoWayGridView which means (in theory) that it will just be about flipping classes once I'm done.

Comment 9

[Lucas Rocha (:lucasr)]

Attachment: Change TabsTray to use TwoWayGridView

This patch doesn't change anything user visible. This is just about changing the parent class of TabsTray for now.

Comment 10

[Lucas Rocha (:lucasr)]

Attachment: Add support for width animations in PropertyAnimator

Necessary plumbing to support vertical swiping gestures on TabsTray (when scrolling horizontally).

Comment 11

[Lucas Rocha (:lucasr)]

Attachment: Vertical swipe support in TabsTray

This is implementing vertical gestures when TabsTray is horizontal. Node that this patch is just adding functionality but it's not really using it yet as the TwoWayGridView is always vertical at this point.

Comment 12

[Lucas Rocha (:lucasr)]

Attachment: Remove close button from tabs view

We'll be replacing the close button with a context menu action triggered with long tap. This is not implemented in this patch set just yet. I'll file a follow-up bug. Also, removing the close button before changing the tab view layout makes the following patches easier to grasp.

Comment 13

[Lucas Rocha (:lucasr)]

Attachment: New TabsPanel sizing

This is about applying different sizing behaviour when the tabs tray is horizontally scrollable. Again, this is just necessary plumbing. The sizing behaviour remains unchanged as the TwoWayGridView is always vertical at this point.

Comment 14

[Lucas Rocha (:lucasr)]

Attachment: Add more granular API to query tablet form-factors

Need more granular queries to make sure this patch series doesn't have any broken intermediary state. This is important because the last patch might take longer to land than the others.

Comment 15

[Lucas Rocha (:lucasr)]

Attachment: Change sidebar state to be dynamic

Ok, here the fun starts. This is about making the isSideBar state dynamic instead of a hardcoded property in the layouts. As a result, we don't need two separate gecko_app.xml.in anymore as the layout of the tabs tray will be changed dynamically in code.

Comment 16

[Lucas Rocha (:lucasr)]

Attachment: New tabs tray layout with horizontal scrolling

And here the big switch happens. This is targeting bug 817721, bug 817735, and bug 817732. The user visible changes:

1. The phone UI uses horizontal scrolling tabs tray while in landscape. The tabs tray in portrait is unchanged.
2. Small and large tablet UI use horizontal scrolling tabs tray while in portrait. Use sidebar layout for tabs tray in landscape.
3. Small and large tablet share the same toolbar layout.

As a result of 3, we don't need a separate browser_toolbar_menu.xml for large and xlarge devices anymore. They both use what used to be the xlarge layout now. I overwritten layout-large-v11/browser_toolbar_menu.xml with what we currently have in layout-xlarge-v11/browser_toolbar_menu.xml and removed layout-xlarge-v11/browser_toolbar_menu.xml.

Furthermore, the tabs row layout will depend on orientation across form factors. We now have two layouts: tabs_item_cell.xml (thumbnail with overlayed title) and tabs_item_row.xml (what we current use in the phone UI). I then use layout aliases to use one or the other as per design.

Both layout/tabs_row.xml and layout-xlarge-v11/tabs_row.xml are not used directly anymore so I removed them.

Generally speaking, the tabs_item_row.xml layout is only used on phone UIs while in portrait. tabs_item_cell.xml is used on all other cases. We might need to very margins, spacing, etc depending on form factor. I'll use styles (as opposed to separate layouts) if that becomes necessary.

I disabled the toolbar animation (the one that expands the entry to go to the awesome screen) on both small and large tablets now as they don't fit well on the sidebar UI.

Comment 17

[Lucas Rocha (:lucasr)]

Ok, here are some known issues:

- Rotating device while tabs tray is open causes layout to look broken.
- Crash when closing the tabs tray in portrait mode on tablets.
- Tabs button loses background color when tabs tray sidebar is opened on small tablets.
- The layout of tab items are not as per design spec yet (especially the new tabs_item_cell.xml)
- The tab items are sized incorrectly when scrolling horizontally
- Tabs button's little arrow should point left/down on landscape and portrait respectively.

I still think these patches should get reviewed now. These issues can be worked on in follow-up bugs.

Comment 18

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

Comment on attachment 701253 [details] [diff] [review]
Add "TwoWay" views to repo

I only really looked at the non-code parts of the patch. I have no plans to review the code itself, especially since we plan to replace it.

Comment 19

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

Comment on attachment 701255 [details] [diff] [review]
Change TabsTray to use TwoWayGridView


>diff --git a/mobile/android/base/TabsTray.java b/mobile/android/base/TabsTray.java

>-        setItemsCanFocus(true);
>+        //setItemsCanFocus(true);

you wanted to keep this around? I assume we'll put it back later.

Comment 20

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

Comment on attachment 701266 [details] [diff] [review]
Remove close button from tabs view

I'm worried about the affect on accessibility. How will people know/discover the swipe to close. How will blind people?

We might need to keep the button, but make it invisible (not gone). So the a11y tools can pick it up and use it.

Thoughts?

Comment 21

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

Comment on attachment 701273 [details] [diff] [review]
Add more granular API to query tablet form-factors

This seems a little clunky. Not sure is we could switch from booleans to constants:

formFactor() returns FORM_LARGETABLET, FORM_SMALLTABLET or FROM_PHONE

This is OK for now

Comment 22

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #20)
> Comment on attachment 701266 [details] [diff] [review]
> Remove close button from tabs view
> 
> I'm worried about the affect on accessibility. How will people know/discover
> the swipe to close. How will blind people?
> 
> We might need to keep the button, but make it invisible (not gone). So the
> a11y tools can pick it up and use it.
> 
> Thoughts?

The Android UI's approach for a11y is to provide the swipe gesture as the main interaction and context menu (triggered with long-tap) as the accessible alternative. See the task switcher, for example. But, yeah, let's discuss this with the a11y guys.

In terms of "discoverability", I agree we'll need to do something to show first-time users how to do it (especially on lower-end devices with pre-ICS Android).

Ian, what are your thoughts on this?

Comment 23

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

(In reply to Lucas Rocha (:lucasr) from comment #22)
> (In reply to Mark Finkle (:mfinkle) from comment #20)
> > Comment on attachment 701266 [details] [diff] [review]
> > Remove close button from tabs view
> > 
> > I'm worried about the affect on accessibility. How will people know/discover
> > the swipe to close. How will blind people?
> > 
> > We might need to keep the button, but make it invisible (not gone). So the
> > a11y tools can pick it up and use it.
> > 
> > Thoughts?
> 
> The Android UI's approach for a11y is to provide the swipe gesture as the
> main interaction and context menu (triggered with long-tap) as the
> accessible alternative. See the task switcher, for example. But, yeah, let's
> discuss this with the a11y guys.
> 
> In terms of "discoverability", I agree we'll need to do something to show
> first-time users how to do it (especially on lower-end devices with pre-ICS
> Android).
> 
> Ian, what are your thoughts on this?

Sounds like we want to use a long-press menu. Sriram mentioned the same thing in his own bug/patch that removes the close button. That patch breaks a few robocop tests, so we'll need to add the close menu and update the tests.

Comment 24

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

Comment on attachment 701266 [details] [diff] [review]
Remove close button from tabs view

How much of this would you keep if we added a "Close Tab" long press menu? Does it matter to try to keep the code, or shold we just remove it and add it back when the long-press menu is added? I guess the latter is OK by me.

Comment 25

[Lucas Rocha (:lucasr)]

Attachment: Add TwoWayView to repo

This is the new TwoWayView now.

Comment 26

[Lucas Rocha (:lucasr)]

Attachment: Change TabsTray to use TwoWayView

Comment 27

[Lucas Rocha (:lucasr)]

Attachment: Vertical swipe support in TabsTray

Comment 28

[Lucas Rocha (:lucasr)]

Attachment: New TabsPanel sizing

Comment 29

[Lucas Rocha (:lucasr)]

Attachment: Always scroll on tabs tray animation

Ok, this one is new. I decided to always use scroll for the tabs tray animation. In practice, this patch doesn't change anything because we're not using TextureView anyway. This change is just a way to simplify the necessary changes to make sidebar/non-sidebar layout switches work.

Even if we get to enable TextureView in the future, the performance of scroll-based animations is pretty good as we're using hardware layers whenever we can.

Comment 30

[Lucas Rocha (:lucasr)]

Attachment: New tabs tray layout with horizontal scrolling

This one changed quite a bit btw. This patch fixes all the issues mentioned on comment #17. The knows issues now are:

1. TwoWayView doesn't selection mode support yet (i.e. setItemChecked() is not available). This means the selected tab is not properly highlighted. I'll be working on this next.
2. The arrow on the tabs button sometimes points to the wrong direction after rotating device between landscape and portrait.
3. Sometimes TwoWayView gets "confused" when scrolling horizontally more than a 4-5 tabs.

This is ready to get input from UX team though.

Comment 31

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

Comment on attachment 713471 [details] [diff] [review]
Add TwoWayView to repo

I did not review the code itself, there is quite a bit, and since it is based on other code I am taking a leap that it mostly works. We'll find bugs as we start to test and fix them.

The parts of the patch to add the component look fine.

There is a lot of Log.d output that we will want to remove before releasing.

Some commented code in setupChild could be removed if not needed

I do want to get OK on the license from Grev, just so we are not surprising anyone. (For others, this is also being released as a standalone widget for general Android use)

Comment 32

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

Comment on attachment 713477 [details] [diff] [review]
Change TabsTray to use TwoWayView


>-        setItemsCanFocus(true);
>+        //setItemsCanFocus(true);

>         // Updates the selected position in the list so that it will be scrolled to the right place.
>         private void updateSelectedPosition() {
>-            int selected = getPositionForTab(Tabs.getInstance().getSelectedTab());
>-            for (int i=0; i < getCount(); i++)
>-                 TabsTray.this.setItemChecked(i, (i == selected));
>+            // int selected = getPositionForTab(Tabs.getInstance().getSelectedTab());
>+            // for (int i=0; i < getCount(); i++)
>+            //      TabsTray.this.setItemChecked(i, (i == selected));


I assume we'll change this code in the next patches. If not, remove it

Comment 33

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #32)
> Comment on attachment 713477 [details] [diff] [review]
> Change TabsTray to use TwoWayView
> 
> 
> >-        setItemsCanFocus(true);
> >+        //setItemsCanFocus(true);

I'll uncomment this line once I implement focus handling in TwoWayView.

> >         // Updates the selected position in the list so that it will be scrolled to the right place.
> >         private void updateSelectedPosition() {
> >-            int selected = getPositionForTab(Tabs.getInstance().getSelectedTab());
> >-            for (int i=0; i < getCount(); i++)
> >-                 TabsTray.this.setItemChecked(i, (i == selected));
> >+            // int selected = getPositionForTab(Tabs.getInstance().getSelectedTab());
> >+            // for (int i=0; i < getCount(); i++)
> >+            //      TabsTray.this.setItemChecked(i, (i == selected));
> 
> 
> I assume we'll change this code in the next patches. If not, remove it

I've just implemented choice mode support in TwoWayView. This code is not commented out anymore locally.

Comment 34

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #31)
> Comment on attachment 713471 [details] [diff] [review]
> Add TwoWayView to repo
> 
> I did not review the code itself, there is quite a bit, and since it is
> based on other code I am taking a leap that it mostly works. We'll find bugs
> as we start to test and fix them.
> 
> The parts of the patch to add the component look fine.
> 
> There is a lot of Log.d output that we will want to remove before releasing.

Yep, I'll make logging disabled by default once I finish all the core features of the widget. Right now it's just simpler to keep logging on all the time.

> Some commented code in setupChild could be removed if not needed

Ok, I'll remove it.

> I do want to get OK on the license from Grev, just so we are not surprising
> anyone. (For others, this is also being released as a standalone widget for
> general Android use)

FYI: we have a copy of Android's DateTimePicker in our repo under the same license. TwoWayView is pretty much the same case, license-wise.

Comment 35

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

Comment on attachment 713482 [details] [diff] [review]
New TabsPanel sizing

>diff --git a/mobile/android/base/TabsPanel.java b/mobile/android/base/TabsPanel.java

> import org.mozilla.gecko.widget.IconTabWidget;
> 
>+import org.mozilla.gecko.widget.TwoWayView;

Keep these together

>-        view.getWindowVisibleDisplayFrame(windowRect);
>+        listContainer.getWindowVisibleDisplayFrame(windowRect);
>         int windowHeight = windowRect.bottom - windowRect.top;
> 
>+

No need for extra blank line


Eventually, we'll need to check the affect of using TwoWay on startup performance. Are we inflating too soon, and stuff like that.

Comment 36

[Lucas Rocha (:lucasr)]

Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/52eb67d56577
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7b36d82768d
https://hg.mozilla.org/integration/mozilla-inbound/rev/d20e50778bbc
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ad939012eb8
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f0f2ba1521c
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ab1d763ff1c
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff74c059d79f
https://hg.mozilla.org/integration/mozilla-inbound/rev/173822e8dd83
https://hg.mozilla.org/integration/mozilla-inbound/rev/5445992580b1

Comment 37

[Ryan VanderMeulen [:RyanVM]]

When pushing needs-clobber changes to inbound, please update the CLOBBER file too.
https://hg.mozilla.org/integration/mozilla-inbound/rev/007046026bfa

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/52eb67d56577
https://hg.mozilla.org/mozilla-central/rev/f7b36d82768d
https://hg.mozilla.org/mozilla-central/rev/d20e50778bbc
https://hg.mozilla.org/mozilla-central/rev/2ad939012eb8
https://hg.mozilla.org/mozilla-central/rev/3f0f2ba1521c
https://hg.mozilla.org/mozilla-central/rev/5ab1d763ff1c
https://hg.mozilla.org/mozilla-central/rev/ff74c059d79f
https://hg.mozilla.org/mozilla-central/rev/173822e8dd83
https://hg.mozilla.org/mozilla-central/rev/5445992580b1
https://hg.mozilla.org/mozilla-central/rev/007046026bfa

Comment 39

[Alex Keybl [:akeybl]]

This bug has been listed as part of the Aurora 22 release notes in either [1], [2], or both. If you find any of the information or wording incorrect in the notes, please email [e-mail] asap. Thanks!

[1] https://www.mozilla.org/en-US/firefox/22.0a2/auroranotes/
[2] https://www.mozilla.org/en-US/mobile/22.0a2/auroranotes/

Comment 40

[Lucas Rocha (:lucasr)]

(In reply to Alex Keybl [:akeybl] from comment #39)
> This bug has been listed as part of the Aurora 22 release notes in either
> [1], [2], or both. If you find any of the information or wording incorrect
> in the notes, please email [e-mail] asap. Thanks!
> 
> [1] https://www.mozilla.org/en-US/firefox/22.0a2/auroranotes/
> [2] https://www.mozilla.org/en-US/mobile/22.0a2/auroranotes/

Looks right. I'd probably mention bug 817735 on the release notes too btw.