[FIG] Transition for creating a new tab from a menu




6 years ago
5 years ago


(Reporter: ibarlow, Unassigned)


Dependency tree / graph

Firefox Tracking Flags



(Whiteboard: abouthome-hackathon)


(1 attachment)



6 years ago
See https://bugzilla.mozilla.org/show_bug.cgi?id=885363#c2 for a detailed description. 

The transition is the first sequence in the files below:

.MOV: http://cl.ly/0j402j2I0k2h
.GIF: http://cl.ly/image/1d3N102N201f
Putting this as a blocker (although we'll still have to discuss the content of this bug)
Priority: -- → P1
This bug is just about flipping about:home UI on top of web content when a new tab is created. We might need to add a hack to fade/gray out the web content while the about:home UI flips in.
Whiteboard: abouthome-hackathon
Assignee: nobody → wjohnston
tracking-fennec: --- → ?
OS: Mac OS X → Android
Hardware: x86 → All
Version: Firefox 24 → unspecified
tracking-fennec: ? → +

Comment 3

6 years ago
Friendly ping to check on progress here, -- while it wasn't crucial for v1 this would be a nice transition to see sooner rather than later :)
Posted patch PatchSplinter Review
Built on top of the patches in bug 889620. Build at:

Attachment #830669 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 830669 [details] [diff] [review]

Review of attachment 830669 [details] [diff] [review]:

I'd like to understand the implications of the the explicit showHomePager() call before approving.

::: mobile/android/base/BrowserApp.java
@@ +1229,5 @@
> +    private Tab animateAddTab(String url, int flags) {
> +        final PropertyAnimator anim = new PropertyAnimator(TABS_ANIMATION_DURATION);
> +        final Tab tab = Tabs.getInstance().loadUrl(url, flags | Tabs.LOADURL_NEW_TAB | Tabs.LOADURL_BACKGROUND);
> +
> +        ((BrowserApp)GeckoAppShell.getContext()).showHomePager(HomePager.AnimationType.FLIP, anim);

Why do you need to use getContext() to get a reference to BrowserApp here? Isn't this a BrowserApp method after all?

Also, isn't this code a bit racy? Once the new tab is selected we will also call showHomePager, no? Wouldn't it be better to handle this when the new tab is selected?

::: mobile/android/base/Tab.java
@@ -153,5 @@
>      private void setAboutHomePage(HomePager.Page page) {
>          mAboutHomePage = page;
>      }
> -

Attachment #830669 - Flags: review?(lucasr.at.mozilla) → review-
(In reply to Lucas Rocha (:lucasr) from comment #5)
> Also, isn't this code a bit racy? Once the new tab is selected we will also
> call showHomePager, no? Wouldn't it be better to handle this when the new
> tab is selected?

1.) Selecting the new tab makes the LayerView blank, which results in a pretty ugly transition. This needs to happen before Gecko selects the new tab. We could do something where java selects the tab and waits to tell Gecko until after the transition, but this felt more... confined. Or we could try "flipping" the view in Gecko as well. That's a pretty major refactor away from using a deck in browser.xul...

2.) selectTab doesn't currently know if something is a "new tab" or not. We could make it aware, but I like that selectTab doesn't have to know about whether the tab is new or not. showHomePager gets called twice here, but bails the second time (since its already showing).

The biggest problem with this patch (IMO) is that we avoid creating views in about:home until after the transition ends, which leads to a strange "blank" transition and a bunch of views (jankily) appearing after. Allowing the views to be created during the transition leads to jank as well. I think we probably need to animate adding the gridview/listview rows to about:home (in this situation at least) if we do this. Or we could try waiting until the views were created/inserted to do the transition but that risks feeling laggy.
Ian told me he wants to rethink this. Unassigning until we have new mockups.
Assignee: wjohnston → nobody
I'm going to close this because yuan closed bug 885363. antlam filed bug 1053390 about creating some new designs for this.
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.