Closed Bug 1014987 Opened 10 years ago Closed 10 years ago

Display tabs horizontally

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 35

People

(Reporter: ywang, Assigned: lucasr)

References

Details

(Whiteboard: [fixed-larch])

Attachments

(4 files, 2 obsolete files)

Australis tabs on top
846.48 KB, image/jpeg
Details
Add tab strip to layout on new tablet UI (r=mcomella)
20.18 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
Introduce the new tab strip views (r=mcomella)
23.86 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
Gracefully handle out-of-bounds positions in TabsStripAdapter (r=mcomella)
996 bytes, patch
mcomella
: review+
Details | Diff | Splinter Review
Attached image Australis tabs on top — 
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
Assignee: nobody → lucasr.at.mozilla
Blocks: 1017307
Blocks: 1015467
Blocks: 1015447
Blocks: 1017313
Blocks: 1018426
Blocks: 1024816
Depends on: 1048865
Depends on: 1055576
Comment on attachment 8475222 [details] [diff] [review]
Introduce the new tab strip views (r=mcomella)

The tab strip views.
Attachment #8475222 - Flags: review?(michael.l.comella)
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.
Attachment #8475223 - Flags: review?(michael.l.comella)
Blocks: 1055597
Blocks: 1055601
Blocks: 1055604
Blocks: 1055606
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()`?
Attachment #8475222 - Flags: review?(michael.l.comella) → feedback+
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.
Attachment #8475223 - Flags: review?(michael.l.comella) → review+
(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.
Status: NEW → ASSIGNED
(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.
Blocks: 1064828
Backed out due to reftest failures. Note to self: next time run Try with reftests included.
https://hg.mozilla.org/projects/larch/rev/b526a44079c3
Whiteboard: [fixed-larch]
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?
Flags: needinfo?(jmaher)
Flags: needinfo?(blassey.bugs)
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.
Flags: needinfo?(jmaher)
(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'.
Blocks: 1065396
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
Attachment #8475222 - Attachment is obsolete: true
Attachment #8475223 - Attachment is obsolete: true
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.
Attachment #8487269 - Flags: review?(michael.l.comella)
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.
Attachment #8487265 - Flags: review?(michael.l.comella)
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.
Flags: needinfo?(blassey.bugs)
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.
Attachment #8487269 - Flags: review?(michael.l.comella) → review+
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.
Attachment #8487265 - Flags: review?(michael.l.comella) → review+
(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.
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.
Attachment #8487568 - Flags: review?(michael.l.comella)
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.
Attachment #8487568 - Flags: review?(michael.l.comella) → review+
(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 :-)
Blocks: 1085771
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: