From feedback channel, tablet users also would like to see tabs visible on top for quick switching and closing a tab. The number one finding of sprint testing is that desktop style controls are familiar to participants and they adopt them quickly. This is a great opportunity to bring Australis to Android, creating a visual consistency across desktop and tablet. Next step: * visual design tweaks * implementation

Comment on attachment 8475222 [details] [diff] [review] Introduce the new tab strip views (r=mcomella) The tab strip views.

Comment on attachment 8475223 [details] [diff] [review] Add tab strip to layout on new tablet UI (r=mcomella) Conditionally inflate the tab strip in the app's layout when new tablet UI is enabled. Tested with tablet UI enabled and disabled, working fine. Patch with UI tests coming soon.

I should have also mentioned that the current Tabs API for fetching the ordered list of tabs and handle new tabs *sucks* e.g. it doesn't give us a normalized tab position for private and normal tabs, it forces us to copy the list of tabs to refresh, etc. So I'm not fixing this now. We can discuss this in the front-end meeting this week.

FWIW, this patch draws the tab strip items using the same technique than ShapedButton. The more I think about this the more I'm convinced that we shouldn't actually be using saveLayer() for such fundamental building blocks of UI. It's too expensive. The right thing to do here is to simply draw the shapes with colors that respond to the view's drawable state (pressed, checked, etc) where possible. Still need to figure out how our lightweight themes interact with our ShapedButtons. I'll update the patch accordingly. Filed bug 1056132 to track this.

Actually, before I got ahead and get rid of saveLayer(), let me first have a look at our current theming code to see what we can do. For now, let's go ahead with this approach.

Comment on attachment 8475222 [details] [diff] [review] Introduce the new tab strip views (r=mcomella) Review of attachment 8475222 [details] [diff] [review]: ----------------------------------------------------------------- Initial comments - more tomorrow. ::: mobile/android/base/tabs/TabStripAdapter.java @@ +45,5 @@ > + } > + > + @Override > + public View getView(int position, View convertView, ViewGroup parent) { > + TabStripItemView item = (TabStripItemView) convertView; I'd prefer: final TapStripItemView item; if (convertView == null) { item = ...; else { item = ...; } `final` ensures you properly initialize item. @@ +81,5 @@ > + } > + > + void refresh(List<Tab> tabs) { > + this.tabs = tabs; > + notifyDataSetChanged(); if (tabs == null) { notifyDataSetInvalidated(); } else { notify...Changed(); } ? @@ +86,5 @@ > + } > + > + void clear() { > + tabs = null; > + notifyDataSetChanged(); notifyDataSetInvalidated(); ? ::: mobile/android/base/tabs/TabStripItemView.java @@ +71,5 @@ > + @Override > + public void onAttachedToWindow() { > + super.onAttachedToWindow(); > + > + setOnClickListener(new View.OnClickListener() { Should we keep a reference to these listeners so we don't cause GC? This is a question I've always had, so I'm going to add it to the frontend meeting notes. @@ +75,5 @@ > + setOnClickListener(new View.OnClickListener() { > + @Override > + public void onClick(View v) { > + if (id >= 0) { > + Tabs.getInstance().selectTab(id); When might we click a tab that has an id < 0? Should we throw an IllegalStateException instead? @@ +83,5 @@ > + > + closeView.setOnClickListener(new View.OnClickListener() { > + @Override > + public void onClick(View v) { > + if (id >= 0) { Same, re id < 0.

Comment on attachment 8475222 [details] [diff] [review] Introduce the new tab strip views (r=mcomella) Review of attachment 8475222 [details] [diff] [review]: ----------------------------------------------------------------- I'm not too familiar with Canvas/Path/Region and drawing techniques in general, so you may want to get a second reviewer for that code, but otherwise this looks mostly good to me - just a few questions. Is it okay that tab_strip_item & tab_strip_item_view do not have "new_tablet" prepended to their name? ::: mobile/android/base/resources/layout/tab_strip_item.xml @@ +2,5 @@ > +<!-- This Source Code Form is subject to the terms of the Mozilla Public > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > + > +<org.mozilla.gecko.tabs.TabStripItemView Why separate this from tab_strip_item_view? @@ +8,5 @@ > + android:layout_width="@dimen/new_tablet_tab_strip_item_width" > + android:layout_height="match_parent" > + android:background="@drawable/new_tablet_tab_strip_item_bg" > + android:paddingLeft="28dp" > + android:paddingRight="15dp"/> Why is the padding asymmetric? ::: mobile/android/base/tabs/TabStripItemView.java @@ +122,5 @@ > + Canvas.CLIP_TO_LAYER_SAVE_FLAG); > + > + super.draw(canvas); > + > + tabPaint.setXfermode(tabXferMode); Is setting this every time we draw necessary? ::: mobile/android/base/tabs/TabStripView.java @@ +55,5 @@ > + } > + > + private View getViewForTab(Tab tab) { > + final int position = adapter.getPositionForTab(tab); > + return getChildAt(position - getFirstVisiblePosition()); I'm not sure I understand this - what is getFirstVisiblePosition? @@ +99,5 @@ > + } > + > + private void removeTab(Tab tab) { > + adapter.removeTab(tab); > + updateSelectedStyle(getPositionForSelectedTab()); Any reason you did not use `updateSelectedPosition()`?

Comment on attachment 8475223 [details] [diff] [review] Add tab strip to layout on new tablet UI (r=mcomella) Review of attachment 8475223 [details] [diff] [review]: ----------------------------------------------------------------- Not exactly sure how ViewStubs in RelativeLayouts work, specifically how the views are laid out if the ViewStub is never inflated, but I see how your approach can work so if it looks correct on the device, lgtm. ::: mobile/android/base/resources/layout/gecko_app.xml @@ +67,5 @@ > android:layout_below="@+id/browser_actionbar" > android:background="@android:color/white" > android:visibility="invisible"/> > > + nit: ws.

(In reply to Michael Comella (:mcomella) from comment #8) > Comment on attachment 8475222 [details] [diff] [review] > Introduce the new tab strip views (r=mcomella) > > Review of attachment 8475222 [details] [diff] [review]: > ----------------------------------------------------------------- > > Initial comments - more tomorrow. > > ::: mobile/android/base/tabs/TabStripAdapter.java > @@ +45,5 @@ > > + } > > + > > + @Override > > + public View getView(int position, View convertView, ViewGroup parent) { > > + TabStripItemView item = (TabStripItemView) convertView; > > I'd prefer: > > final TapStripItemView item; > if (convertView == null) { > item = ...; > else { > item = ...; > } > > `final` ensures you properly initialize item. Ok, done. > @@ +81,5 @@ > > + } > > + > > + void refresh(List<Tab> tabs) { > > + this.tabs = tabs; > > + notifyDataSetChanged(); > > if (tabs == null) { > notifyDataSetInvalidated(); > } else { > notify...Changed(); > } > > ? tabs is never null. No need. > @@ +86,5 @@ > > + } > > + > > + void clear() { > > + tabs = null; > > + notifyDataSetChanged(); > > notifyDataSetInvalidated(); ? Never used it before. Doesn't seem strictly necessary here but let's go with it. > ::: mobile/android/base/tabs/TabStripItemView.java > @@ +71,5 @@ > > + @Override > > + public void onAttachedToWindow() { > > + super.onAttachedToWindow(); > > + > > + setOnClickListener(new View.OnClickListener() { > > Should we keep a reference to these listeners so we don't cause GC? Nice catch. Generally speaking it's fine to allocate new objects in onAttachToWindow() but this view is used in an AdapterView which means it going to be recycled on scroll i.e. constantly detached and then re-attached. I simply moved this code to the constructor. > @@ +75,5 @@ > > + setOnClickListener(new View.OnClickListener() { > > + @Override > > + public void onClick(View v) { > > + if (id >= 0) { > > + Tabs.getInstance().selectTab(id); > > When might we click a tab that has an id < 0? Should we throw an > IllegalStateException instead? Yeah. This was just me being paranoid but it's necessary. Changed it to throw instead. > @@ +83,5 @@ > > + > > + closeView.setOnClickListener(new View.OnClickListener() { > > + @Override > > + public void onClick(View v) { > > + if (id >= 0) { > > Same, re id < 0. Done.

(In reply to Michael Comella (:mcomella) from comment #9) > Comment on attachment 8475222 [details] [diff] [review] > Introduce the new tab strip views (r=mcomella) > > Review of attachment 8475222 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm not too familiar with Canvas/Path/Region and drawing techniques in > general, so you may want to get a second reviewer for that code, but > otherwise this looks mostly good to me - just a few questions. > > Is it okay that tab_strip_item & tab_strip_item_view do not have > "new_tablet" prepended to their name? > > ::: mobile/android/base/resources/layout/tab_strip_item.xml > @@ +2,5 @@ > > +<!-- This Source Code Form is subject to the terms of the Mozilla Public > > + - License, v. 2.0. If a copy of the MPL was not distributed with this > > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > > + > > +<org.mozilla.gecko.tabs.TabStripItemView > > Why separate this from tab_strip_item_view? So that we can set attributes that are specific to this view inside TabStripView. > @@ +8,5 @@ > > + android:layout_width="@dimen/new_tablet_tab_strip_item_width" > > + android:layout_height="match_parent" > > + android:background="@drawable/new_tablet_tab_strip_item_bg" > > + android:paddingLeft="28dp" > > + android:paddingRight="15dp"/> > > Why is the padding asymmetric? Because the close button has padding to increase its hit area. This asymetric padding compensates that. > ::: mobile/android/base/tabs/TabStripItemView.java > @@ +122,5 @@ > > + Canvas.CLIP_TO_LAYER_SAVE_FLAG); > > + > > + super.draw(canvas); > > + > > + tabPaint.setXfermode(tabXferMode); > > Is setting this every time we draw necessary? Nope, moved it to the constructor. > ::: mobile/android/base/tabs/TabStripView.java > @@ +55,5 @@ > > + } > > + > > + private View getViewForTab(Tab tab) { > > + final int position = adapter.getPositionForTab(tab); > > + return getChildAt(position - getFirstVisiblePosition()); > > I'm not sure I understand this - what is getFirstVisiblePosition? The first visible position from the adapter that is represented by an active child. The first child of a ListView (or GridView, TwoWayView, etc) is always bound to the first visible position. So, (tab-position - first-visible-position) gives the child index. This method will return null is the tab position is not currently represented by an active view in the layout (hence the null check in TabsListener). > @@ +99,5 @@ > > + } > > + > > + private void removeTab(Tab tab) { > > + adapter.removeTab(tab); > > + updateSelectedStyle(getPositionForSelectedTab()); > > Any reason you did not use `updateSelectedPosition()`? Nope, done.

(In reply to Lucas Rocha (:lucasr) from comment #12) > > I'm not too familiar with Canvas/Path/Region and drawing techniques in > > general, so you may want to get a second reviewer for that code, but > > otherwise this looks mostly good to me - just a few questions. > > > > Is it okay that tab_strip_item & tab_strip_item_view do not have > > "new_tablet" prepended to their name? Those are specific to the tab strip views which are very self-contained. I'm only using the 'new_tablet' prefix where we touch common code.

(In reply to Michael Comella (:mcomella) from comment #10) > Comment on attachment 8475223 [details] [diff] [review] > Add tab strip to layout on new tablet UI (r=mcomella) > > Review of attachment 8475223 [details] [diff] [review]: > ----------------------------------------------------------------- > > Not exactly sure how ViewStubs in RelativeLayouts work, specifically how the > views are laid out if the ViewStub is never inflated, but I see how your > approach can work so if it looks correct on the device, lgtm. It will just align with parent in this case, which works fine. > ::: mobile/android/base/resources/layout/gecko_app.xml > @@ +67,5 @@ > > android:layout_below="@+id/browser_actionbar" > > android:background="@android:color/white" > > android:visibility="invisible"/> > > > > + > > nit: ws. Fixed.

(In reply to Lucas Rocha (:lucasr) from comment #11) > tabs is never null. No need. Add a comment please.

(In reply to Michael Comella (:mcomella) from comment #15) > (In reply to Lucas Rocha (:lucasr) from comment #11) > > tabs is never null. No need. > > Add a comment please. Done.

https://hg.mozilla.org/projects/larch/rev/c8cbe8cb8feb https://hg.mozilla.org/projects/larch/rev/0356dcf5da53 Filed bug 1064828 to land the UI tests.

Backed out due to reftest failures. Note to self: next time run Try with reftests included. https://hg.mozilla.org/projects/larch/rev/b526a44079c3

Bra, Joel, for some reason, adding the tab strip is causing all sorts of reftest failures. See: https://tbpl.mozilla.org/?tree=Larch&rev=0356dcf5da53 I thought reftests weren't supposed to be affected by UI changes but I guess I was wrong. Any clues/tips on how to debug this?

in general reftests are testing the gecko part, not the java front end. That doesn't mean there is a chance for errors like you saw. I would look in reftest.js: http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest.js specifically look for bootstrap, as that is what is different between desktop and android. We call in there from bootstrap.js: http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/bootstrap.js There might be clues there. To debug this, can you run it locally? This should work on a local device. Either gbrown or myself could help answer some of the odd harness issues you might run into.

(In reply to Joel Maher (:jmaher) from comment #20) > To debug this, can you run it locally? This should work on a local device. > Either gbrown or myself could help answer some of the odd harness issues you > might run into. Yes, I can run the tests locally. The tests fail but at different spots than tbpl and the reference images ('image 2') in the logs are all black for the failing tests.

another vector to look at is to see if there is a common pattern in the failing tests on tbpl. It doesn't surprise me that local vs tbpl/try is different- every device/machine is so different. As for the black image2, could we be taking a snapshot of the wrong window or canvas (front end vs back end?)

If you look at the reftest analyzer for the failures in https://tbpl.mozilla.org/?tree=Larch&rev=0356dcf5da53 you'll see some random rectangular artifacts in the generated images ('image 1'). I've also sent the same patches to Try before (see https://tbpl.mozilla.org/?tree=Try&rev=974b68ec646b). It fails in different spots but still has the same random rectangles in 'image 1'.

FYI: I'm trying a different version of the gecko_app layout changes which is a little less disruptive here: https://tbpl.mozilla.org/?tree=Try&rev=ff786bc9ed94

Comment on attachment 8487269 [details] [diff] [review] Introduce the new tab strip views (r=mcomella) Just realized that you had not r+'d this patch before. Apologies.

Comment on attachment 8487265 [details] [diff] [review] Add tab strip to layout on new tablet UI (r=mcomella) Ok, here's a different take which I think both makes more sense and fixes the reftest failures. This patch encloses tab strip in the same container than toolbar and slide them in/out as you scroll web content. This is what Chrome for Android does right now. It feels right to me after using it a bit. We can just close bug 1048575 for now.

FWIW, the cause of reftests failures was that I was shifting down the web content view (SurfaceView) so that it was placed below the tab strip but that was causing gfx issues for unknown reasons. This new patch simple makes the tab strip part of the 'browser chrome' that slides in and out as you scroll the web content.

Comment on attachment 8487269 [details] [diff] [review] Introduce the new tab strip views (r=mcomella) Review of attachment 8487269 [details] [diff] [review]: ----------------------------------------------------------------- lgtm. (In reply to Lucas Rocha (:lucasr) from comment #12) > > Why separate this from tab_strip_item_view? > > So that we can set attributes that are specific to this view inside > TabStripView. Like in case a tab becomes more than just a favicon and a text view? > > Why is the padding asymmetric? > > Because the close button has padding to increase its hit area. This > asymetric padding compensates that. Add a comment so it's not confusing if we come back to it later.

Comment on attachment 8487265 [details] [diff] [review] Add tab strip to layout on new tablet UI (r=mcomella) Review of attachment 8487265 [details] [diff] [review]: ----------------------------------------------------------------- lgtm.

(In reply to Michael Comella (:mcomella) from comment #30) > Comment on attachment 8487269 [details] [diff] [review] > Introduce the new tab strip views (r=mcomella) > > Review of attachment 8487269 [details] [diff] [review]: > ----------------------------------------------------------------- > > lgtm. > > (In reply to Lucas Rocha (:lucasr) from comment #12) > > > Why separate this from tab_strip_item_view? > > > > So that we can set attributes that are specific to this view inside > > TabStripView. > > Like in case a tab becomes more than just a favicon and a text view? No exactly. This is for things like: width/height params, padding, stuff specific to how a TabStripItemView should look within a specific layout. For instance, the padding values dependent of the specific item margins set in that TabStripView. > > > Why is the padding asymmetric? > > > > Because the close button has padding to increase its hit area. This > > asymetric padding compensates that. > > Add a comment so it's not confusing if we come back to it later. Good point. Done.

Here we go again: https://hg.mozilla.org/projects/larch/rev/17df9c908e09 https://hg.mozilla.org/projects/larch/rev/442ff10e74a9

Comment on attachment 8487568 [details] [diff] [review] Gracefully handle out-of-bounds positions in TabsStripAdapter (r=mcomella) Adapters shouldn't crash when given an out-of-positions position. They should simply return an invalid ID or null item.

For context: getting a crasher on a couple of mochitests due to a little bug in the adapter. TwoWayView queries item IDs for old positions when the adapter notifies changes. Adapters should just return an invalid ID if the position is out of bounds.

Comment on attachment 8487568 [details] [diff] [review] Gracefully handle out-of-bounds positions in TabsStripAdapter (r=mcomella) Review of attachment 8487568 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Lucas Rocha (:lucasr) from comment #35) > Adapters shouldn't crash when given an out-of-positions position. They > should simply return an invalid ID or null item. I don't think this is necessarily true - SimpleAdapter crashes in this case, but CursorAdapter does not.

(In reply to Michael Comella (:mcomella) from comment #37) > Comment on attachment 8487568 [details] [diff] [review] > Gracefully handle out-of-bounds positions in TabsStripAdapter (r=mcomella) > > Review of attachment 8487568 [details] [diff] [review]: > ----------------------------------------------------------------- > > (In reply to Lucas Rocha (:lucasr) from comment #35) > > Adapters shouldn't crash when given an out-of-positions position. They > > should simply return an invalid ID or null item. > > I don't think this is necessarily true - SimpleAdapter crashes in this case, > but CursorAdapter does not. True. SimpleAdapter has a bug then :-)

https://hg.mozilla.org/mozilla-central/rev/17df9c908e09 https://hg.mozilla.org/mozilla-central/rev/442ff10e74a9 https://hg.mozilla.org/mozilla-central/rev/518f412e118d