Closed Bug 1203345 Opened 4 years ago Closed 4 years ago

Add shadow to tab previews in tabs tray

Categories

(Firefox for Android :: Theme and Visual Design, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed

People

(Reporter: antlam, Assigned: vjoshi, Mentored)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [lang=java][lang=xml][good next bug])

Attachments

(4 files, 3 obsolete files)

Adding a shadow helps lift each tab away from the backdrop and make it seem more tangible. This detail in the design should help with making each tab feel more polished and more tangible.

If it's not too much work, we should try and reuse the same shadow that we used in our doorhangers work in bug 1137921. 

Unsure how much work this is though, Martyn?
Flags: needinfo?(mhaigh)
(In reply to Anthony Lam (:antlam) from comment #0)
> If it's not too much work, we should try and reuse the same shadow that we
> used in our doorhangers work in bug 1137921. 

I asked liuche in bug 1146488 whether we should use CardView instead.
WFM! I really like CardView
Bug 1203345 - (WIP) Add shadows to tabs tray items via CardView.

I built on the build changes in bug 1203730 to get CardView.

I couldn't get this to work properly - the selection state seems to be
overridden by the CardView background and shadows don't appear.

Theory: The thumbnails are set to an absolute size, forcing the wrap_content
CardView to expand outwards, exceeding the bounds of its parent so it gets
clipped when drawing shadows.
To be explicit, I do not intend to continue working on this at the moment, especially because CardView is *huge* and we may not ship it (see bug 1203730).
Can I get a mock up of what the shadows would look like alongside the highlighted orange border
Flags: needinfo?(mhaigh) → needinfo?(alam)
The shadow has the same specs as the ones we used in our doorhangers, share overlay, etc 

it also sits above the orange highlight :)
Flags: needinfo?(alam)
^ :D
Flags: needinfo?(mhaigh)
Just found this bug while searching for some other tabs tray bug. Isn't a CardView a bit too heavy for just adding a shadow? A lightweight alternative would be to just add android:elevation to the tab view. This attribute will be ignored on old versions though.
(In reply to Sebastian Kaspari (:sebastian) from comment #9)
> Just found this bug while searching for some other tabs tray bug. Isn't a
> CardView a bit too heavy for just adding a shadow? A lightweight alternative
> would be to just add android:elevation to the tab view. This attribute will
> be ignored on old versions though.

That sounds good to me! 

What's the support like? as in, how old are we talking?
Flags: needinfo?(s.kaspari)
(In reply to Anthony Lam (:antlam) from comment #10)
> What's the support like? as in, how old are we talking?

Elevation has been introduced in Android 5.0.

http://developer.android.com/training/material/shadows-clipping.html
Flags: needinfo?(s.kaspari)
(In reply to Sebastian Kaspari (:sebastian) from comment #9)
> Isn't a CardView a bit too heavy for just adding a shadow? A lightweight alternative
> would be to just add android:elevation to the tab view.

Yes, it is quite heavy. I think I started this business when I found out CardView had built-in shadows but I wasn't aware elevation worked on anything other than CardView – I'm happy to use elevation!
To be fair, I'm not too worried about this right now. 

Open to making this mentor-able and I'm also going to put this under fennec-polish.
Flags: needinfo?(martyn.haigh+bugzilla)
Sebastian, feel free to add [good next bug] if you think it'd be that simple.
Mentor: s.kaspari, michael.l.comella
Whiteboard: [lang=java][lang=xml]
(In reply to Michael Comella (:mcomella) from comment #14)
> Sebastian, feel free to add [good next bug] if you think it'd be that simple.

I expect old Android versions to just ignore the attribute. If we need to set this in code then ViewCompat.setElevation() should handle backwards compatibility for us.

Anthony: The elevation is defined in dp as well. Do you have a good value or should we just try a couple of values to see what looks good?

Other elements like the Toolbar have elevation values set as well. We should make sure that we do not accidentally change the order in which the views are drawn (e.g. tabs on top of toolbar).
Whiteboard: [lang=java][lang=xml] → [lang=java][lang=xml][good next bug]
(In reply to Sebastian Kaspari (:sebastian) from comment #15)
> (In reply to Michael Comella (:mcomella) from comment #14)
> > Sebastian, feel free to add [good next bug] if you think it'd be that simple.
> 
> I expect old Android versions to just ignore the attribute. If we need to
> set this in code then ViewCompat.setElevation() should handle backwards
> compatibility for us.
 
That's fine :)

> Anthony: The elevation is defined in dp as well. Do you have a good value or
> should we just try a couple of values to see what looks good?

I haven't got a guideline for this value yet, but I'd love to establish one going forward. In the meantime, trying a couple logical values sounds like a good starting point.
 
> Other elements like the Toolbar have elevation values set as well. We should
> make sure that we do not accidentally change the order in which the views
> are drawn (e.g. tabs on top of toolbar).

Indeed, I also want us to be aware that we will likely use different elevations for different things, states, metaphors, etc somewhere down the line. 

Nothing concrete yet, but like our color palette, I am aiming to be very prescriptive and deliberate when choosing these values.
I'd like to take this up. What do you think would be the best way to go about it and decide the value for the elevation parameter?
(In reply to varunj.1011 from comment #17)
> I'd like to take this up. What do you think would be the best way to go
> about it and decide the value for the elevation parameter?

Hi and welcome! :) 

The design guidelines have a good chapter about that:
http://www.google.com/design/spec/what-is-material/elevation-shadows.html

On that page you can find a list of elevations used by standard Android components. According to that list "cards" use an elevation of 2dp. Maybe that's a good value to start. Test this (or other) value(s) in various orientations and scroll states: We do not want to draw the tabs on top of the ActionBar or things like that. Maybe create a screenshot (or multiple if there are other good values) for Anthony to review (You can set feedback? flags on uploaded attachments). Also have a look on how older Android versions behave - I guess they will just ignore the value - but we do not want to crash.
(In reply to varunj.1011 from comment #17)
> I'd like to take this up. What do you think would be the best way to go
> about it and decide the value for the elevation parameter?

Awesome! give it a try :)

As per Sebastian's suggestions, I'd agree that 2dp (since these most resemble "cards") is probably what we want so a screenshot with that would be a great place to start.
I added the elevation attribute to the TabsPanelThumbnailView and tried giving it values from 2-20dp, but I couldn't see a shadow while testing on my phone (Android 5.0.2), though the shadows do show up in the design view in Android Studio. I added a clipToPadding attribute to the parent TabThumbnailWrapper as well, but it didn't make a difference. What could I have been doing wrong?
There are some things here that make it more complicated:

* We have two layouts for tabs: tabs_list_item_view.xml (list) and tabs_layout_item_view.xml (grid)

* TabsPanelTHumbnailViews are wrapped in a TabThumbnailWrapper. This view draws the selection state etc. So you want to add the elevation to the wrapper.

* Views can't draw over the bounds of its parent. It looks like currently the parent (TabsLayoutItemView) does not leave any room for drawing a shadow (at least on tablets). Try adding some padding to the parent container and remove now unnecessary padding or margin from the children to remain the same look (if possible).

* Add clipToPadding=false to the parent (TabsLayoutItemView) to avoid Android clipping the view and effectively removing the shadow again.

With those changes the shadows should show up on the device. Later (maybe in a follow-up bug) we could also look at dynamically changing the elevation based on the state (pressed). :)
(In reply to Sebastian Kaspari (:sebastian) from comment #21)
> There are some things here that make it more complicated:
> 
> * We have two layouts for tabs: tabs_list_item_view.xml (list) and
> tabs_layout_item_view.xml (grid)
> 
> * TabsPanelTHumbnailViews are wrapped in a TabThumbnailWrapper. This view
> draws the selection state etc. So you want to add the elevation to the
> wrapper.
> 
> * Views can't draw over the bounds of its parent. It looks like currently
> the parent (TabsLayoutItemView) does not leave any room for drawing a shadow
> (at least on tablets). Try adding some padding to the parent container and
> remove now unnecessary padding or margin from the children to remain the
> same look (if possible).
> 
> * Add clipToPadding=false to the parent (TabsLayoutItemView) to avoid
> Android clipping the view and effectively removing the shadow again.
> 
> With those changes the shadows should show up on the device. Later (maybe in
> a follow-up bug) we could also look at dynamically changing the elevation
> based on the state (pressed). :)

Thanks! I tried all of your suggestions and I finally figured out what was going wrong: We need to add a solid background color to the child view for the shadow to be seen, in addition to having clipToPadding=false for the parent ViewGroup. I'll be uploading a patch adding the shadows soon! I don't have access to a tablet device, can someone test it for me on a tablet?
(In reply to varunj.1011 from comment #22)
> I don't have access to a tablet device, can someone test it for me on a tablet?

We use the same grid view on phones in landscape mode. :)
Does this look fine?
Attachment #8685379 - Flags: feedback?(alam)
Attachment #8685379 - Attachment description: Screenshot of tab switcher screen with shadows → Screenshot of tab switcher screen with shadows (portrait)
(In reply to Sebastian Kaspari (:sebastian) from comment #23)
> (In reply to varunj.1011 from comment #22)
> > I don't have access to a tablet device, can someone test it for me on a tablet?
> 
> We use the same grid view on phones in landscape mode. :)

Oh, that makes things easier :)
Added the clipToPadding=false attribute to the parent views in both tabs_list_item_view.xml and tabs_layout_item_view.xml. Added background="#000000" and elevation="2dp" to the TabsPanelThumbnailView in both files.

The background attribute is added because it seems Android doesn't draw shadows when the child element doesn't have solid background color.
Attachment #8686628 - Flags: review?(s.kaspari)
Attachment #8686628 - Flags: review?(alam)
Attachment #8686628 - Flags: feedback?(alam)
Comment on attachment 8685379 [details]
Screenshot of tab switcher screen with shadows (portrait)

This looks good!
Attachment #8685379 - Flags: feedback?(alam) → feedback+
Comment on attachment 8686626 [details]
Screenshot of tab switcher screen with shadows (landscape/tablet)

Thanks! this is looking good!

But I don't really see it on the Ars Technica tab as much as the vertical orientation...
Attachment #8686626 - Flags: feedback?(alam) → feedback+
(In reply to Anthony Lam (:antlam) from comment #29)
> Comment on attachment 8686626 [details]
> Screenshot of tab switcher screen with shadows (landscape/tablet)
> 
> Thanks! this is looking good!
> 
> But I don't really see it on the Ars Technica tab as much as the vertical
> orientation...

I've used the same value for elevation (2dp) for both. Do you think we should increase it for landscape?
(In reply to varunj.1011 from comment #30)
> I've used the same value for elevation (2dp) for both. Do you think we
> should increase it for landscape?

No, we should keep it the same :)
Comment on attachment 8686628 [details] [diff] [review]
Adds shadows to tab previews in tray via elevation

Leaving this one to Sebastian
Attachment #8686628 - Flags: review?(alam)
Attachment #8686628 - Flags: feedback?(alam)
Comment on attachment 8686628 [details] [diff] [review]
Adds shadows to tab previews in tray via elevation

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

My eyes are incapable of seeing any shadow on the dark gray background (at least the 2dp one). I changed the background to white locally and can see that there's in fact a shadow. :) However it's clearly visible when a tab is selected because the shadow is drawn on top of the orange outline.

::: mobile/android/base/resources/layout/tabs_layout_item_view.xml
@@ +62,5 @@
>          <org.mozilla.gecko.tabs.TabsPanelThumbnailView android:id="@+id/thumbnail"
>                                                         android:layout_width="@dimen/tab_thumbnail_width"
>                                                         android:layout_height="@dimen/tab_thumbnail_height"
> +                                                       android:elevation="2dp"
> +                                                       android:background="#000000"

I am not really happy about this background because it will require an additional / invisible draw.

With some digging around I found that an OutlineProvider is used to define the outline of a view from which the shadow will be generated[1]. The default outline provider indeed uses the background to generate the outline[2]. However there's also an outline provider available that uses the bounds of the view. This seems to be good enough for our rectangular ImageView.

So instead of setting the background color, you could set the outline provider:
android:outlineProvider="bounds"

[1] developer.android.com/reference/android/view/View.html#setOutlineProvider(android.view.ViewOutlineProvider)
[2] http://developer.android.com/reference/android/view/ViewOutlineProvider.html#BACKGROUND
[3] http://developer.android.com/reference/android/view/ViewOutlineProvider.html#BOUNDS

::: mobile/android/base/resources/layout/tabs_list_item_view.xml
@@ +27,5 @@
>          <org.mozilla.gecko.tabs.TabsPanelThumbnailView android:id="@+id/thumbnail"
>                                                         android:layout_width="@dimen/tab_thumbnail_width"
> +                                                       android:layout_height="@dimen/tab_thumbnail_height"
> +                                                       android:elevation="2dp"
> +                                                       android:background="#000000"/>

Same here. :)
Attachment #8686628 - Flags: review?(s.kaspari) → feedback+
Attachment #8659573 - Attachment is obsolete: true
Attachment #8659574 - Attachment is obsolete: true
Assignee: nobody → varunj.1011
Status: NEW → ASSIGNED
Changed to use outlineProvider=bounds
Attachment #8686628 - Attachment is obsolete: true
Attachment #8689143 - Flags: review?(s.kaspari)
(In reply to Sebastian Kaspari (:sebastian) from comment #33)
> Comment on attachment 8686628 [details] [diff] [review]
> Adds shadows to tab previews in tray via elevation
> 
> Review of attachment 8686628 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> My eyes are incapable of seeing any shadow on the dark gray background (at
> least the 2dp one). I changed the background to white locally and can see
> that there's in fact a shadow. :) However it's clearly visible when a tab is
> selected because the shadow is drawn on top of the orange outline.
> 
> ::: mobile/android/base/resources/layout/tabs_layout_item_view.xml
> @@ +62,5 @@
> >          <org.mozilla.gecko.tabs.TabsPanelThumbnailView android:id="@+id/thumbnail"
> >                                                         android:layout_width="@dimen/tab_thumbnail_width"
> >                                                         android:layout_height="@dimen/tab_thumbnail_height"
> > +                                                       android:elevation="2dp"
> > +                                                       android:background="#000000"
> 
> I am not really happy about this background because it will require an
> additional / invisible draw.
> 
> With some digging around I found that an OutlineProvider is used to define
> the outline of a view from which the shadow will be generated[1]. The
> default outline provider indeed uses the background to generate the
> outline[2]. However there's also an outline provider available that uses the
> bounds of the view. This seems to be good enough for our rectangular
> ImageView.
> 
> So instead of setting the background color, you could set the outline
> provider:
> android:outlineProvider="bounds"
> 
> [1]
> developer.android.com/reference/android/view/View.
> html#setOutlineProvider(android.view.ViewOutlineProvider)
> [2]
> http://developer.android.com/reference/android/view/ViewOutlineProvider.
> html#BACKGROUND
> [3]
> http://developer.android.com/reference/android/view/ViewOutlineProvider.
> html#BOUNDS
> 
> ::: mobile/android/base/resources/layout/tabs_list_item_view.xml
> @@ +27,5 @@
> >          <org.mozilla.gecko.tabs.TabsPanelThumbnailView android:id="@+id/thumbnail"
> >                                                         android:layout_width="@dimen/tab_thumbnail_width"
> > +                                                       android:layout_height="@dimen/tab_thumbnail_height"
> > +                                                       android:elevation="2dp"
> > +                                                       android:background="#000000"/>
> 
> Same here. :)

The outlineProvider=bounds method is much more elegant than the hack I'd used!
Should I somehow change the shadows or are the fine as they are? It's true that they are almost invisible on the gray background, so should I increase the elevation and post a screenshot?
I think it's nice that it's kind of subtle. Plus, this will still come in handy when we want to leverage this "depth" created by shadows in our other interactions like swiping to dismiss, selecting multiple tabs, etc.
(In reply to Anthony Lam (:antlam) from comment #36)
> I think it's nice that it's kind of subtle. Plus, this will still come in
> handy when we want to leverage this "depth" created by shadows in our other
> interactions like swiping to dismiss, selecting multiple tabs, etc.

Makes sense. So I'll leave them as they are for now.
Comment on attachment 8689143 [details] [diff] [review]
addShadowsToTabThumbnails.patch

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

LGTM. Thank you! :)
Attachment #8689143 - Flags: review?(s.kaspari) → review+
Should I mark this as fixed?
The tests have passed, so we can add the "checkin-needed" keyword. Someone will land the patch and mark the bug as fixed. :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e8da96844aed
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
You need to log in before you can comment on or make changes to this bug.