848719 - Modify tab tray transition to keep the feeling of a layered UI

Status: RESOLVED FIXED

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

Description

[Ian Barlow (:ibarlow)]

Attachment: mockup

In keeping with the theme of UI cleanup, we have too many "layers" of colour in our title bar. Let's simply things down from ltblue/grey/black to just ltblue/grey. 

When we make this change, let's also adapt the behaviour of title bar tab elements to pull down the screen with the rest of the title bar. I'll post a mockup in the next comment to clarify this.

Comment 1

[Ian Barlow (:ibarlow)]

Attachment: mockup

As mentioned in the previous comment, here is a mockup of how the title bar should preserve its structure when we move it down the screen. The tab count and menu stay visible, instead of disappearing like it does now.

Comment 2

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 722154 [details]
mockup

Will the tab button have a curve on the right side? Are we just changing the color of Menu button (and tabs button) to a milder grey?

Comment 3

[Ian Barlow (:ibarlow)]

No, the tab selection will follow the curve on the left, and be straight up and down on the right.

Comment 4

[Sriram Ramasubramanian [:sriram]]

So, we have the curve still ;)

Comment 5

[Sriram Ramasubramanian [:sriram]]

The color change would not work in tablets -- in expanded mode. What's the resolution there?

Comment 6

[Sriram Ramasubramanian [:sriram]]

This is how the color difference on tablets be: http://cl.ly/image/3f2p1f0x0x21 (Unfortunately I couldn't take screenshots on tablet as its crashing due to something else. But this shows what I mean in my above comment).

Also, I need carats pointing upwards. (And how does tablets do? Do they show an up arrow currently?). There is no space above the number currently.

I somehow miss some affordance on the tabs button. It's plain and there isn't a demarcation between it and the menu button. If we want to make it look like a action-bar item, may be a  divider would be good. But that would spoil the design. How is AiUi dealing with this?

Comment 7

[Ian Barlow (:ibarlow)]

Thanks Sriram. There is a bigger discussion happening now, about whether this is an approach we want to take, or if we want to remove the curve altogether. 

Let's park this bug until we've done our due diligence in UX and explored all the possible options. Thanks.

Comment 8

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 1: Remove Tail Delegate

Tail Delegation is needed only on one side now. We don't have to do the ugly "Orange is a Fruit that contains a list of Fruits" inside TailTouchDelegate :(

Comment 9

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 2: Remove curves

:tears: :tears:
Removing one side curve on the tabs button. Now everything is a ShapedButton "with/without" curves. This cleanup the browser_toolbar.xml. The attribute for showing a "cropped" behavior is removed. The colors are all same for tabs and menu button -- hence one drawable. Also, I've used the "background_tabs_dark" instead of the given color, as it matches with the tabs-tray properly (for now).

Comment 10

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP: Remove browser_toolbar

Really? Did I just Remove "two" browser_toolbar files? :O :O
This works fine with the UI part. However, the animations are screwed up. Hence WIP.

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 725141 [details] [diff] [review]
Part 2: Remove curves

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

Love the amount of code being removed here.

::: mobile/android/base/BrowserToolbar.java
@@ +59,1 @@
>      private BrowserToolbarBackground mAddressBarBg;

You can probably simplify the animation code that expands the toolbar to enter awesomescreen. Currently, it does some layout updates related to the previously curvy background view which are probably not necessary anymore.

::: mobile/android/base/resources/layout-land-v14/browser_toolbar.xml.in
@@ +21,5 @@
>  
>          <org.mozilla.gecko.BrowserToolbarBackground android:id="@+id/address_bar_bg"
>                                                      android:layout_width="fill_parent"
>                                                      android:layout_height="fill_parent"
>                                                      android:layout_marginRight="14dip"

No need for margins anymore.

@@ +24,5 @@
>                                                      android:layout_height="fill_parent"
>                                                      android:layout_marginRight="14dip"
>                                                      android:layout_alignParentTop="true"
>                                                      android:layout_alignParentRight="true"
> +                                                    gecko:curveTowards="right"

Why do you need this curve?

::: mobile/android/base/resources/layout-land-v14/browser_toolbar_menu.xml.in
@@ +21,5 @@
>  
>          <org.mozilla.gecko.BrowserToolbarBackground android:id="@+id/address_bar_bg"
>                                                      android:layout_width="fill_parent"
>                                                      android:layout_height="fill_parent"
>                                                      android:layout_marginRight="50dip"

No need for this margin too I guess.

@@ +24,5 @@
>                                                      android:layout_height="fill_parent"
>                                                      android:layout_marginRight="50dip"
>                                                      android:layout_alignParentTop="true"
>                                                      android:layout_alignParentRight="true"
> +                                                    gecko:curveTowards="right"

Why do you need this curve?

@@ +133,5 @@
>                        android:layout_width="0dip"
>                        android:layout_height="0dip"
>                        android:visibility="gone"/>
>  
> +        <Gecko.ShapedButton android:id="@+id/menu"

Could probably just be a GeckoButton I guess?

::: mobile/android/base/resources/layout/browser_toolbar.xml.in
@@ +21,5 @@
>  
>          <org.mozilla.gecko.BrowserToolbarBackground android:id="@+id/address_bar_bg"
>                                                      android:layout_width="fill_parent"
>                                                      android:layout_height="fill_parent"
>                                                      android:layout_marginRight="18dip"

I guess you can now remove this margin too?

::: mobile/android/base/resources/layout/browser_toolbar_menu.xml.in
@@ +21,5 @@
>  
>          <org.mozilla.gecko.BrowserToolbarBackground android:id="@+id/address_bar_bg"
>                                                      android:layout_width="fill_parent"
>                                                      android:layout_height="fill_parent"
>                                                      android:layout_marginRight="66dip"

Same here. I think this margin is not necessary anymore.

@@ +131,5 @@
>                        android:layout_width="0dip"
>                        android:layout_height="0dip"
>                        android:visibility="gone"/>
>  
> +        <Gecko.ShapedButton android:id="@+id/menu"

Can't this be a more regular GeckoButton now given that is has no special shape anymore?

Comment 12

[Sriram Ramasubramanian [:sriram]]

(In reply to Lucas Rocha (:lucasr) from comment #11)
> Comment on attachment 725141 [details] [diff] [review]
> Part 2: Remove curves
> 
> Review of attachment 725141 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Love the amount of code being removed here.
> 

GeckoButton cannot be used as it is reserved for the pre-processed standard Android Button that derives from GeckoView.java.frag. :(

Also, the margins are for the distance from the right end. The WIP posted removes one of the two files, and makes the margins count from the tabs button. They seem to be needed.

curveTowards is to identify which side the curve has to be shown. On phones it curves towards the right, while on tablets, its towards the left.

Comment 13

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #12)
> (In reply to Lucas Rocha (:lucasr) from comment #11)
> > Comment on attachment 725141 [details] [diff] [review]
> > Part 2: Remove curves
> > 
> > Review of attachment 725141 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Love the amount of code being removed here.
> > 
> 
> GeckoButton cannot be used as it is reserved for the pre-processed standard
> Android Button that derives from GeckoView.java.frag. :(
> 
> Also, the margins are for the distance from the right end. The WIP posted
> removes one of the two files, and makes the margins count from the tabs
> button. They seem to be needed.

I thought the curve on the background was only needed when we were showing the 'void' in the toolbar while the tabs tray was open.

Comment 14

[Ian Barlow (:ibarlow)]

Attachment: tab opening transition

Sadly, UX has had something of a change of heart here. 

Losing the second tab curve makes the title bar feel less like it has a "tab" layer, and more like it just has a curve in it. I think we can preserve the current look with a couple of smart transitions, as described in the attached mockup. 

Things to consider:
* Sliding the tab curve to the right when we open the tab tray
* Fading the tab tray icons into view as the tray is sliding open
* Keeping a "hole" on the right of the title bar area as it opens. Right now this conflicts with the way our dynamic header is working -- we see through to web content instead of the tab tray. 

There's been a lot of discussion in IRC today, which conflicting opinions about what can / can't or should / shouldn't be done. Sriram seemed to have an idea of how he might approach this, though so over to you sir :)

Comment 16

[Sriram Ramasubramanian [:sriram]]

Doubts regarding the screen artifacts in Bug 853133:
Is the BrowserToolbar drawn as a part of the LayerView now?
I added a "footer" (like in tablet, to be of same color as header) to the XML file and tried it.
Until Gecko started, if I open the tabs-tray, there was no corruption. I was able to see the footer. http://cl.ly/image/2T1P1x3q3Y2T
But after that, another layer of blue is added behind the BrowserToolbar (evident from overdraw too), and the grey portion isn't seen.

Comment 17

[Sriram Ramasubramanian [:sriram]]

Oh! (Sorry Lucas if you were working on this :( ), I know the problem here.
If you install a persona, open and close tabs-panel-sidebar, there is a visual artifact that can be see (before we refresh the BrowserToolbar background). It's the same problem here. The blue pixels from the top are repeating in the hollow region.

There are 2 ways to fix this:
1. Have the tabs and menu button visible always. (Design option 1 with flat tabs-menu-button-bar).
2. Have a dark region behind the tabs and menu button and it will stay when the tabs-button is hidden. (one case of overdraw in the top right corner).

Comment 18

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch: Corruption

As per my theory on animation and corruption, the only better way to solve it is to have a small layer beneath it. This add a View of size tabs-width ( + its margin) on top right corner. Thereby, when the tabs and menu buttons aren't visible, this layer is visible. We can selectively show/hide this layer (to avoid overdraw) -- but that can be done with the animation.

Comment 19

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch: Corruption

Ah! The tablet case.

Comment 20

[Sriram Ramasubramanian [:sriram]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #19)
> Created attachment 727983 [details] [diff] [review]
> Patch: Corruption
> 
> Ah! The tablet case.

This won't work on tablets, sorry. We don't crop the url-bar background on tablets. I'll post a better patch that uses this logic, and crops the url-bar background on tablets soonish.

Comment 21

[Sriram Ramasubramanian [:sriram]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #20)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #19)
> > Created attachment 727983 [details] [diff] [review]
> > Patch: Corruption
> > 
> > Ah! The tablet case.
> 
> This won't work on tablets, sorry. We don't crop the url-bar background on
> tablets. I'll post a better patch that uses this logic, and crops the
> url-bar background on tablets soonish.

Oops. This patch works on tablets. Just that it won't work with personas. I can post a patch for that if needed.

Comment 22

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch: Corruption (Personas)

This patch does the same thing as the previous one. In addition, this also has a personas image that matches with the tabs-panel-header.

Comment 23

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch: Corruption (Personas)

Had missed a qrefresh.

Comment 24

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

Comment on attachment 727983 [details] [diff] [review]
Patch: Corruption

This patch fixes a visual problem on Nightly in the URL toolabr when the tabs tray is open. We need this fix.

This patch does not remove any curves or change the overall layout of the URL toolbar.

Comment 25

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

Comment on attachment 728485 [details] [diff] [review]
Patch: Corruption (Personas)

This patch is needed to fix the visual problem when Personas (light weight themes) are used.

Comment 26

[Sriram Ramasubramanian [:sriram]]

Patch with personas: https://hg.mozilla.org/integration/mozilla-inbound/rev/7b734d6f2fea

Comment 27

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 727983 [details] [diff] [review]
Patch: Corruption

Pushed the other patch that takes care of personas too. Marking this as obsolete.

Comment 28

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7b734d6f2fea

Comment 30

[Aaron Train [:aaronmt]]

Which patch are we waiting for here?

Comment 31

[Lucas Rocha (:lucasr)]

(In reply to Aaron Train [:aaronmt] from comment #30)
> Which patch are we waiting for here?

I think this is blocking on a design decision now. Ian, any new proposal coming soon?

Comment 32

[Ian Barlow (:ibarlow)]

This is the most recent state of the design here: http://cl.ly/image/3x2P3W3K082Q/o

Biggest changes are:
- Removing the double curve (again)
- Using the same dark grey for the tab button, tab title bar and tab tray, to make the UI look more seamless.

Sriram has a test buid of this mostly working. 

STILL TO DO: I need to address an issue the rest of the design team has, which is how to give the tab counter a clearer affordance. 

At this point I am open to landing what Sriram has so far in Nightly and refining from there.

Comment 33

[Lucas Rocha (:lucasr)]

FYI: whatever we do about this bug will have to be uplifted to Aurora.

Comment 34

[Lucas Rocha (:lucasr)]

(In reply to Ian Barlow (:ibarlow) from comment #32)
> This is the most recent state of the design here:
> http://cl.ly/image/3x2P3W3K082Q/o
> 
> Biggest changes are:
> - Removing the double curve (again)
> - Using the same dark grey for the tab button, tab title bar and tab tray,
> to make the UI look more seamless.
> 
> Sriram has a test buid of this mostly working. 
> 
> STILL TO DO: I need to address an issue the rest of the design team has,
> which is how to give the tab counter a clearer affordance. 
> 
> At this point I am open to landing what Sriram has so far in Nightly and
> refining from there.

Maybe we can land the changes that Sriram has almost ready in Nightly and do the fading transition on a separate bug. Just to get things moving here. Unless Sriram already has the fading animation implemented?

Comment 35

[Sriram Ramasubramanian [:sriram]]

I'll post the patch today. I need to fix the font sizes. Rest is done as per spec. Animation could be a followup.

Comment 36

[Sriram Ramasubramanian [:sriram]]

Btw, why should this be uplifted to aurora? The corruption is already handled in aurora.

Comment 37

[Ian Barlow (:ibarlow)]

Yeah, but it looks pretty ugly. I'd prefer to push this up if we can.

Comment 38

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

no need to track this. Let's just do it and it can ride the trains.

Comment 39

[Lucas Rocha (:lucasr)]

(In reply to Ian Barlow (:ibarlow) from comment #37)
> Yeah, but it looks pretty ugly. I'd prefer to push this up if we can.

Exactly. I personally don't consider the current state of Aurora good enough for a release.

Comment 40

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 725140 [details] [diff] [review]
Part 1: Remove Tail Delegate

Removed one of the two tail delegates. We don't need to share the tail with the menu button anymore.

Comment 41

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 2: Remove curves

This removes the curves. The layout files have the "empty view to avoid corruption" removed. The unwanted colors are removed. There is only one dark background_tabs color now.

Comment 42

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP: Remove browser_toolbar

This is re-based to latest code. I'm confused on how the calculation on amount of transition is done. Lucas, could you please take care of that part and make the animation work fine on phones? I just added a part where the "translation" should include mMenu.getWidth(). The awesomebar doesn't stretch to the end of the screen. May be some margin problems.

Comment 43

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 4: "One" BrowserToolbar

Note: This can be used only if the animation in the WIP for removing browser_toolbar can be fixed.

This removes the layout-land-v14/browser_toolbar_menu.xml and uses dimens in different folders to have just "one" browser_toolbar for phones.

Wait! Did we just remove 3 browser_toolbars by removing "one" curve?

Comment 44

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

Comment on attachment 725140 [details] [diff] [review]
Part 1: Remove Tail Delegate

We could almost remove TailTouchDelegate and just use the raw TouchDelegate. But this is fine.

Comment 45

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

Comment on attachment 733557 [details] [diff] [review]
Part 2: Remove curves

This is a lot of changes. Make sure you test this on a variety of devices, portrait and landscape.

Comment 46

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #44)
> Comment on attachment 725140 [details] [diff] [review]
> Part 1: Remove Tail Delegate
> 
> We could almost remove TailTouchDelegate and just use the raw TouchDelegate.
> But this is fine.

We cannot use a raw touch delegate. They have a state with a problem and I've filed an Android bug for it. http://sriramramani.wordpress.com/2012/08/17/squishy-button/

Comment 47

[Sriram Ramasubramanian [:sriram]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=03b1d3bcf353

Comment 48

[Sriram Ramasubramanian [:sriram]]

Part 1&2 in inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72e02f2e3521
https://hg.mozilla.org/integration/mozilla-inbound/rev/841a0c51f991

Comment 49

[Sriram Ramasubramanian [:sriram]]

This is needed for Aurora (22).

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/72e02f2e3521
https://hg.mozilla.org/mozilla-central/rev/841a0c51f991

Comment 51

[Alex Keybl [:akeybl]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #49)
> This is needed for Aurora (22).

Please request uplift.

Comment 52

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 725140 [details] [diff] [review]
Part 1: Remove Tail Delegate

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New UI to mitigate old UI pain.
User impact if declined: The url bar will look disconnected from tabs UI.
Testing completed (on m-c, etc.): Landed in m-c on 04/05. Nightly users aren't complaining ;)
Risk to taking this patch (and alternatives if risky): None. This removes a lot of code that I added in the past one year to make the curve look good. :tears:
String or IDL/UUID changes made by this patch: None.

Comment 53

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 733557 [details] [diff] [review]
Part 2: Remove curves

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New UI to mitigate old UI pain.
User impact if declined: The url bar will look disconnected from tabs UI.
Testing completed (on m-c, etc.): Landed in m-c on 04/05. Nightly users aren't complaining ;)
Risk to taking this patch (and alternatives if risky): None. This removes a lot of code that I added in the past one year to make the curve look good. :tears:
String or IDL/UUID changes made by this patch: None.

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fcea76b703b7
https://hg.mozilla.org/releases/mozilla-aurora/rev/0b63d33784cf

Comment 55

[Kartikaya Gupta (email:[e-mail])]

FYI these patches reduced our resident memory usage by ~0.5 MB. \o/

Comment 56

[Sriram Ramasubramanian [:sriram]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #55)
> FYI these patches reduced our resident memory usage by ~0.5 MB. \o/

That's the value of one curve ;)

Comment 57

[Lucas Rocha (:lucasr)]

Attachment: Major simplification of browser toolbar layout