1087219 - UI navigation in full screen tabs panel

Status: RESOLVED DUPLICATE of bug 1100464

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Martyn Haigh (:mhaigh)]

How do we want to approach UI navigation (e.g.
affordance for going back to browser, behaviour when adding a new tab in tabs
panel, etc) in this new full-screen mode.

Comment 1

[Lucas Rocha (:lucasr)]

antlam, thoughts?

Comment 2

[Lucas Rocha (:lucasr)]

FWIW, for v1, I think we only need something like 'back button' on screen. As a more obvious way to go back to the browser.

Comment 3

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Yeah , this is basically what we talked about here right? 

https://bugzilla.mozilla.org/show_bug.cgi?id=1064415#c10

Yuan and I were kind of undecided on solutions that included leaving the tabs button in (top right), adding gestures, and relying on the hardware back button.

Gestures obviously are more of a power user feature and so there's a bit of a discover-ability issue there but as long as we don't rely solely on gestures to navigate, I think we'll be fine.

Because we don't currently have that icon there (in the top right), I want to test out her idea first of relying on the hardware back (as well as tapping on any tab preview obviously) and then if we test it for a while and feel like the button needs to be added back, we can go ahead and do that. 

My suspicion is that a lot of users are tapping on the giant tab that takes up most of the screen right now (and the strip at the bottom on Mobile) to return to what they were doing and so removing that in this Full screen tabs panel means they'll be looking for another way to do that...

That being said, let's leave this bug open to track that work.

Comment 4

[Martyn Haigh (:mhaigh)]

Attachment: change tabs panel animation for new tablet UI

Comment 5

[Lucas Rocha (:lucasr)]

Comment on attachment 8520682 [details] [diff] [review]
change tabs panel animation for new tablet UI

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

Need some questions answered and more context.

::: mobile/android/base/BrowserApp.java
@@ +1709,4 @@
>              return false;
>          }
>  
> +        ViewStub tabsPanelStub = null;

final ViewStub tabsPanelStub;
if (...) {
} else {
}

@@ +1780,5 @@
>          }
>  
> +
> +        if(newTabletUi) {
> +            mMainLayoutAnimator = new PropertyAnimator(animationLength, new DecelerateInterpolator());

Have you tried using sTabsInterpolator here?

@@ +1789,5 @@
> +                                           1.0f);
> +            } else {
> +                mMainLayoutAnimator.attach(mBrowserChrome,
> +                                           PropertyAnimator.Property.ALPHA,
> +                                           0.0f);

Maybe use a slightly less verbose version of this?

mMainLayoutAnimator.attach(mBrowserChrome,	
                           PropertyAnimator.Property.ALPHA,
                           areTabsShown() ? 0.0f : 1.0f);

@@ +1803,4 @@
>                                         PropertyAnimator.Property.SCROLL_X,
>                                         -width);
>          } else {
> +            final float animationDistance = newTabletUi ? -(float)(height*0.75) : -height;

nit: maybe something like this is little more readable?
final float animationDistance = -height * (newTabletUi ? 0.75f : 1f);

@@ +1851,5 @@
>              mBrowserToolbar.cancelEdit();
> +
> +            if(NewTabletUI.isEnabled(getContext())) {
> +                mMainLayout.setVisibility(View.INVISIBLE);
> +                mMainLayout.setBackgroundDrawable(null);

Why do you need to set/reset the background of mMainLayout?

::: mobile/android/base/tabs/TabsPanel.java
@@ +127,4 @@
>      }
>  
>      private void initialize() {
> +        mMainContainer = getRootView();

This looks smelly. TabsPanel shouldn't really be animating views that it doesn't own. Why do you need this?

@@ +451,4 @@
>      }
>  
>      public void refresh() {
> +        if(NewTabletUI.isEnabled(getContext())) {

Why is this necessary? Isn't this going to stop the tabs panel from being inflated?

@@ +517,5 @@
> +                objectAnimator = new ObjectAnimator();
> +                objectAnimator.setDuration(200);
> +                objectAnimator.setTarget(mMainContainer);
> +                objectAnimator.setPropertyName("alpha");
> +            }

The tabs panel is never a sidebar when the new tablet UI is enabled. Why is this necessary?

@@ +522,5 @@
>              if (mVisible) {
>                  ViewHelper.setTranslationY(mHeader, -toolbarHeight);
>                  ViewHelper.setTranslationY(mTabsContainer, -toolbarHeight);
> +                if(isNewTabletUi) {
> +                    ViewHelper.setAlpha(mMainContainer, 0.0f);

I need more context about this animation in the root view?

Comment 6

[Martyn Haigh (:mhaigh)]

Attachment: Change tabs panel animation for new tablet UI

(In reply to Lucas Rocha (:lucasr) from comment #5)
> Comment on attachment 8520682 [details] [diff] [review]
> change tabs panel animation for new tablet UI
> 
> Review of attachment 8520682 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +1780,5 @@
> >          }
> >  
> > +
> > +        if(newTabletUi) {
> > +            mMainLayoutAnimator = new PropertyAnimator(animationLength, new DecelerateInterpolator());
> 
> Have you tried using sTabsInterpolator here?

Yes - it was a bit too fast; this interpolator feels like there is less of a sudden change given the increased distance the animation is played out over

> 
> @@ +1851,5 @@
> >              mBrowserToolbar.cancelEdit();
> > +
> > +            if(NewTabletUI.isEnabled(getContext())) {
> > +                mMainLayout.setVisibility(View.INVISIBLE);
> > +                mMainLayout.setBackgroundDrawable(null);
> 
> Why do you need to set/reset the background of mMainLayout?

To reduce overdraw; when the tabs panel is hidden, we can safely remove the background of the main layout and only reinstate it when we next show the tabs panel.

> 
> ::: mobile/android/base/tabs/TabsPanel.java
> @@ +127,4 @@
> >      }
> >  
> >      private void initialize() {
> > +        mMainContainer = getRootView();
> 
> This looks smelly. TabsPanel shouldn't really be animating views that it
> doesn't own. Why do you need this?


Good spot - this was actually meant to be the main container of the tabs panel, not the global root view. 

> 
> @@ +451,4 @@
> >      }
> >  
> >      public void refresh() {
> > +        if(NewTabletUI.isEnabled(getContext())) {
> 
> Why is this necessary? Isn't this going to stop the tabs panel from being
> inflated?

Unless I'm missing something, we don't actually need to strip down and reinflate the tabs panel on an orientation change as we aren't changing the views as with the old UI.  Also by not doing a UI refresh, apart from saving a load of CPU work, we don't need setup the view state ready for the tabs panel closing animation.  Running with and without this early exit code gives me the same result on the new tablet ui with the exception that if this change is removed the state is slightly messed up if we rotate whilst the tabs panel is open, resulting in a slightly odd closing animating the next time it's closed.  I suggest we don't bother refreshing unless there's a compelling reason to remove this change which I'm not aware of, 
> 
> @@ +517,5 @@
> > +                objectAnimator = new ObjectAnimator();
> > +                objectAnimator.setDuration(200);
> > +                objectAnimator.setTarget(mMainContainer);
> > +                objectAnimator.setPropertyName("alpha");
> > +            }
> 
> The tabs panel is never a sidebar when the new tablet UI is enabled. Why is
> this necessary?

This is only run when mIsSideBar != true.  
> 
> @@ +522,5 @@
> >              if (mVisible) {
> >                  ViewHelper.setTranslationY(mHeader, -toolbarHeight);
> >                  ViewHelper.setTranslationY(mTabsContainer, -toolbarHeight);
> > +                if(isNewTabletUi) {
> > +                    ViewHelper.setAlpha(mMainContainer, 0.0f);
> 
> I need more context about this animation in the root view?

We're animating the background colour of the tabs panel container to give a fade in effect, this is run alongside the Y translation of the gridview and the header

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 8522380 [details] [diff] [review]
Change tabs panel animation for new tablet UI

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

Looking good overall. However, I tested this locally and I'm seeing artifacts during the animation (SurfaceView brokenness?) and the animation to open the panel looks a bit weird (it's almost looks like the fading is not doing to alpha=0 or something). The animation to close the panel looks better but the fading feels a bit too quick. Maybe show this to antlam?

::: mobile/android/base/tabs/TabsPanel.java
@@ +127,4 @@
>      }
>  
>      private void initialize() {
> +        mMainContainer = findViewById(R.id.tabs_panel);

Why do you need this? 'this' already is your main container. No need for an extra reference.

@@ +515,5 @@
> +            if (isNewTabletUi) {
> +                objectAnimator = new ObjectAnimator();
> +                objectAnimator.setDuration(200);
> +                objectAnimator.setTarget(mMainContainer);
> +                objectAnimator.setPropertyName("alpha");

We better not mix animations with our own PropertyAnimator with ObjectAnimators. Might break in subtle ways. Maybe just new PropertyAnimator here? I assume the reason you're creating a separate animator here is that you need the fading to be quicker than the global transition?

@@ +521,5 @@
>              if (mVisible) {
>                  ViewHelper.setTranslationY(mHeader, -toolbarHeight);
>                  ViewHelper.setTranslationY(mTabsContainer, -toolbarHeight);
> +                if(isNewTabletUi) {
> +                    ViewHelper.setAlpha(mMainContainer, 0.0f);

I think setFloatValues() will reset the view for you before the animation starts, no? Is this setAlpha() actually necessary?

@@ -511,5 @@
>              animator.attach(mHeader, PropertyAnimator.Property.TRANSLATION_Y, translationY);
>          }
> -
> -        mHeader.setLayerType(View.LAYER_TYPE_HARDWARE, null);
> -        mTabsContainer.setLayerType(View.LAYER_TYPE_HARDWARE, null);

Why are those not necessary anymore? Isn't this going to affect the phone/old tablet transition in a bad way?

Comment 8

[Martyn Haigh (:mhaigh)]

Attachment: UI navigation in full screen tabs panel

(In reply to Lucas Rocha (:lucasr) from comment #7)

> I tested this locally and I'm seeing
> artifacts during the animation (SurfaceView brokenness?)

Fixe - this is because there was no background behind the tabs panel whilst it was animating, giving us some glitchy looking background during the transition at the top of the screen, behind the semi transparent tabs panel.

> > -
> > -        mHeader.setLayerType(View.LAYER_TYPE_HARDWARE, null);
> > -        mTabsContainer.setLayerType(View.LAYER_TYPE_HARDWARE, null);
> 
> Why are those not necessary anymore? Isn't this going to affect the
> phone/old tablet transition in a bad way?

The PropertyAnimator defaults to using hardware layers, and switching them off again, so these calls aren't necessary.  I've left them in at the moment with a TODO as it's not in the scope of this bug.

antlam: Can you checkout this apk (https://dl.dropboxusercontent.com/u/7163922/1087219.apk) and let me know what you think

Comment 9

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Getting a 404 when I try to get to that link. Have you got another?

Comment 10

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Got the link you resent but it doesn't contain the back arrow in the tabs tray...

Comment 11

[Martyn Haigh (:mhaigh)]

antlam: that's but 1100464 that you're thinking of!  This is to do with the animation of the tabs panel reveal

Comment 12

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

looks great and works on the N7 and N9, no artifacts!

Though, when I hit around 15 tabs, I start getting a weird bug where I have to select (orange stroke around) multiple tabs in the panel before the animation triggers.

Comment 13

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

my bad ^ bug title confused me...

Comment 14

[Martyn Haigh (:mhaigh)]

antlam: there's bug 936849 tracking that issue - turns out that it's a known issue but the gridview seems to exaggerate it more than the list view did.

Comment 15

[:Margaret Leibovic]

[Tracking Requested - why for this release]:

Comment 16

[Lucas Rocha (:lucasr)]

Comment on attachment 8529038 [details] [diff] [review]
UI navigation in full screen tabs panel

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

I'm slightly concerned about the increased complexity in the animation code here. I'm finding it hard to follow the patch without more context. Maybe post an overview of the changes here? Please redirect reviews to mcomella from now on.

I wonder if a simple fade animation in the 'browser_chrome' container would be enough here?

::: mobile/android/base/resources/layout/tabs_panel_default.xml
@@ +3,4 @@
>     - 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/. -->
>  
> +<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"

What is this change necessary?

::: mobile/android/base/resources/layout/tabs_panel_view.xml
@@ +9,4 @@
>  <org.mozilla.gecko.tabs.TabsPanel xmlns:android="http://schemas.android.com/apk/res/android"
>                                    android:id="@+id/tabs_panel"
>                                    android:layout_width="match_parent"
>                                    android:layout_height="match_parent"

Won't this break the phone UI?