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)

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.

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

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

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

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.

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.

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

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

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.

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.

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.

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.

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.

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.

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

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

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

This is the new TwoWayView now.

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.

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

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

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

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

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

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

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 release-mgmt@mozilla.com asap. Thanks! [1] https://www.mozilla.org/en-US/firefox/22.0a2/auroranotes/ [2] https://www.mozilla.org/en-US/mobile/22.0a2/auroranotes/

(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 release-mgmt@mozilla.com 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.