1039519 - Implement a smooth transition to show/hide the rocketbar when scrolling up/down a page

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Browser Chrome, defect)

Description

[Ben Francis [:benfrancis]]

Using a CSS translation to move a window during scroll causes performance issues and jank.

This bug is to track switching to using scrollgrab on an outer div and trigger show/hide Rocketbar on scroll position of the outer div instead of scroll events from the inner iframe.

Comment 1

[Ben Francis [:benfrancis]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/21864

WIP patch

Comment 2

[Chris Lord [:cwiiis]]

Working on this here: https://github.com/Cwiiis/gaia/tree/rocketbar-showhide

Comment 3

[Chris Lord [:cwiiis]]

Re-titling - scrollgrab is kind of orthogonal to what this bug is, though we almost certainly will end up using it.

Comment 4

[Chris Lord [:cwiiis]]

So I think to implement this nicely for pages that aren't scrollable, I'm going to need an event similar to mozbrowserasyncscroll, but that informs of page size changes. Needinfo'ing Dale as I remember him saying he prototyped a patch for this at some point?

Comment 5

[Dale Harvey (:daleharvey)]

https://bugzilla.mozilla.org/show_bug.cgi?id=757859 is the bug with the original implementation that sounds like what you are looking for

Comment 6

[Chris Lord [:cwiiis]]

Attachment: Show/hide rocketbar transitions

This is my preliminary work. I think (once the inevitably broken tests are fixed) we should land this and build from there. It touches too many things to comfortably maintain this for a long time outside of the tree.

Not strictly required, but highly recommended to apply the patches on bug 1040226 before trying this.

Comment 7

[Dale Harvey (:daleharvey)]

Comment on attachment 8460259 [details] [review]
Show/hide rocketbar transitions

Passing review as this is most definitely Alive's purview

This is starting to feel nice, the page not moving out from under us is a huge improvement

Comment 8

[Kevin Grandon :kgrandon]

Comment on attachment 8460259 [details] [review]
Show/hide rocketbar transitions

I'm going to pass this review onto Etienne as he's doing investigation into the per-app rocketbar right now and I think it would be valuable for him to take a look. I will also look at the code and comment where appropriate.

Comment 9

[Ben Francis [:benfrancis]]

Comment on attachment 8460259 [details] [review]
Show/hide rocketbar transitions

Would be nice to not need the extra DOM element and this removes some CSS that is probably still needed, but other than that looks good to me. Thanks!

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: scrollgrab.hack.patch

Hey Chris,

Can we build on top of bug 1042083, where there is one title per app window instead of re-using the single rocketbar instance ?

A completely unpolished approach that shows how to do it with the architecture promoted in bug 1042083 is done in the attachment.
Asking a feedback just to make sure you see the attachment as I don't plan to work on that :)

In order to apply it you need to check out the branch: https://github.com/etiennesegonzac/gaia/tree/titlebar and apply the attached patch on top of it.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I also think that the changes to utility_tray.js needs to be done in a separate bug - and fwiw statusbar-background is going to die soon!

Comment 12

[Chris Lord [:cwiiis]]

Comment on attachment 8460259 [details] [review]
Show/hide rocketbar transitions

Removing review request for now, reworking this to work on top of the rocketbar-per-app branch.

Comment 13

[Chris Lord [:cwiiis]]

After talking to Vivien, Kevin and Etienne, I'm convinced of a new direction for this transition:

For apps with navigation where the rocketbar can be expanded, we will have the rocketbar entry in-line with the iframe using scrollgrab, similar to the patch we have. There will be no animation of the bar on/off, it will be exposed/collapsed directly by scrolling.

If the user scrolls the bar into an intermediate state (i.e. makes it half-visible), we will set the scroll position manually to the nearest state (fully on/fully off) after scrolling finishes - we need CSSOM-View smooth scrolling on APZ to do this nicely, but for now it can just immediately appear/disappear. This situation is reasonably rare as the entry is quite small, but it would be a shame to compromise the experience, so we need to block on this for 2.1.

This is a lot simpler than trying to sync animations with scroll events, though we still need all the same machinery for this to work (indeed, a little more).

A draw-back of this technique is that we'll either have to forsake the animation of the entry shrinking into the status bar, or at least, this transition will be made substantially harder to implement adequately. It's certainly still possible though.

Comment 14

[Chris Lord [:cwiiis]]

Attachment: Show/hide rocketbar transitions v2

This is my take on our discussion. This is loosely based off of Vivien's patch.

The idea here is that the animation of the expansion of the rocketbar entry is an animation that gets triggered based on the scroll position of the scrollgrab inner div. This is the same as my first patch.

The difference is that the titlebar now lives inside the scrollgrab div and so scrolls with the page. This, perhaps, enhances the feeling of being connected to the transition. Ideally, we'd drive the progress of the transition based on the scroll position, but this requires significant platform work.

When the user leaves the rocketbar in a half-expanded/collapsed state, the scroll position will be reset to the nearest value that will allow the rocketbar to expand/collapse - Once we have the smooth-scrolling work, we can use that here and make the transition smooth rather than instant.

There is some oddness with flinging and scrollgrab (it seems the fling happens on the inner child before the scrollgrab child) that means this is getting triggered more than it should - I'll open a bug for this.

Comment 15

[Etienne Segonzac (:etienne)]

Comment on attachment 8461133 [details]
Show/hide rocketbar transitions v2

General boring comments:
* +1 on the status bar mirror in app_window, I think it makes a lot of sense
* heads up: the AppWindowManager is probably going to take care of collapsing once search is launched (it's a small change asked by Alive) so we might conflict a bit but that's a detail

Now for the crazy part:
How about having the bubble outside of the titlebar div (still handled by the AppTitleBar subcomponent obviously) and... in position:fixed in the top left part of the container.

So it will always grow from the top left as opposed to moving down then growing toward the top of the screen.

We might have to trigger the shrinking transition a bit earlier so that the bottom edge of the title bar background is never across the bubble.

Let's talk on IRC if my comment isn't clear enough, anyway, it's exciting :)

Comment 16

[Chris Lord [:cwiiis]]

(In reply to Etienne Segonzac (:etienne) from comment #15)
> Comment on attachment 8461133 [details]
> Show/hide rocketbar transitions v2
> 
> General boring comments:
> * +1 on the status bar mirror in app_window, I think it makes a lot of sense
> * heads up: the AppWindowManager is probably going to take care of
> collapsing once search is launched (it's a small change asked by Alive) so
> we might conflict a bit but that's a detail
> 
> Now for the crazy part:
> How about having the bubble outside of the titlebar div (still handled by
> the AppTitleBar subcomponent obviously) and... in position:fixed in the top
> left part of the container.
> 
> So it will always grow from the top left as opposed to moving down then
> growing toward the top of the screen.
> 
> We might have to trigger the shrinking transition a bit earlier so that the
> bottom edge of the title bar background is never across the bubble.
> 
> Let's talk on IRC if my comment isn't clear enough, anyway, it's exciting :)

So I considered and tried this out - the problem then is that the background is moving in sync with the user but the bubble isn't, and at that point, there's no point in it being in the scrollable container at all (this is basically what my patch before this one implements).

That maybe wouldn't be so bad, but it's easy to trigger the situation that the background moves out from behind the bubble before the bubble can start to animate, which looks very odd.

I wonder if a different kind of transition might look better than the growing one for this case (until we get animations controlled by scroll position, then it ought to be fine). It might be easier to experiment with this after landing it though...

I tried triggering the animation based on direction of scroll too, which when it worked made for a nicer looking transition, but was very flaky due to touch jitter (which is hard to compensate for without breaking the effect/degrading it substantially).

Comment 17

[Ben Francis [:benfrancis]]

Comment on attachment 8461133 [details]
Show/hide rocketbar transitions v2

This is quite painful to watch.

What we agreed between engineering, UX and Product last week was...

> The ideal scenario:
> 
> Scrollgrab is used on an outer div wrapping an inner iframe and the animation of
> the expanding/collapsing Rocketbar is a function of the scroll of the outer div.
> 
> Fallback 1:
> 
> If the animation feature doesn't land in time, we would still use scrollgrab but
>  it would trigger a time-based animation instead of being a function of scroll position.
> 
> Fallback 2:
> 
> If scrollgrab is too buggy (see bugs 1040087 and 1040094 with scrollgrab + overscroll I 
> discovered while prototyping this week) then we will use a time-based animation and 
> translate the iframe using a CSS transition as we currently do in the browser app.
> 
> Fallback 3:
> 
> If this approach has performance problems, which it did when I implemented it before 2.1,
> we may need to fall back to a static Rocketbar which is always expanded and can never be hidden.


The ideal scenerio depends on bug 1042258 which is now very unlikely to land in time for 2.1.

We already had an implementation of fallback 1 last week with my original patch, which Chris built on to create his original patch. Here is a video of my patch in action https://www.youtube.com/watch?v=k24oW7qaXVE (You'll notice it also has a working system browser where you can actually view and edit the URL!)

The current patch on this bug is a weird combination of a time-based animation and a scroll based animation which just looks odd.

Until we have bug 1042258 I don't think there's any value in trying to create an effect that looks like it has a scroll driven animation by putting the browser chrome inline with the iframe inside the scrollgrab div, I think we should go back to fallback 1 as planned.

Chris, how difficult do you think it is to do that with the new app title bar?

Also, please take a look at Eric's flow for system browser https://mozilla.app.box.com/s/g2of03hha34htbc4keep and bear in mind that we also need to be able to resize the bubble to make space for back, forward and overflow buttons. We previously had that working in bug 941174 but it looks like we will have to take a new approach with the app title bar.

Chris, this f- is not for you, it's for the fact that we're having to re-visit solved problems.

Comment 18

[Chris Lord [:cwiiis]]

(In reply to Ben Francis [:benfrancis] from comment #17)
> Comment on attachment 8461133 [details]
> The ideal scenerio depends on bug 1042258 which is now very unlikely to land
> in time for 2.1.
> 
> We already had an implementation of fallback 1 last week with my original
> patch, which Chris built on to create his original patch. Here is a video of
> my patch in action https://www.youtube.com/watch?v=k24oW7qaXVE (You'll
> notice it also has a working system browser where you can actually view and
> edit the URL!)
> 
> The current patch on this bug is a weird combination of a time-based
> animation and a scroll based animation which just looks odd.

I agree with this. I'm not certain that there isn't a situation where this combination can work, but I'm uncertain as to whether it's worth spending time on.

> Until we have bug 1042258 I don't think there's any value in trying to
> create an effect that looks like it has a scroll driven animation by putting
> the browser chrome inline with the iframe inside the scrollgrab div, I think
> we should go back to fallback 1 as planned.
> 
> Chris, how difficult do you think it is to do that with the new app title
> bar?

Very easy, the delta for putting it inline was actually quite small (and your original approach is what I implemented first). I can sort this out in another branch, but I want to wait for Etienne's patch to land, or at least stabilise first to avoid multiple painful rebases. At least I'm going to work on another assigned bug for a while to avoid wasting time.

> Also, please take a look at Eric's flow for system browser
> https://mozilla.app.box.com/s/g2of03hha34htbc4keep and bear in mind that we
> also need to be able to resize the bubble to make space for back, forward
> and overflow buttons. We previously had that working in bug 941174 but it
> looks like we will have to take a new approach with the app title bar.

I think we have a lot of scope to change how the expansion transition works, especially if we *don't* go with the combination of scroll+time as you're suggesting (i.e. I think what you're suggesting makes things easier and is probably better for now).

> Chris, this f- is not for you, it's for the fact that we're having to
> re-visit solved problems.

Understood :)

Comment 19

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: animation.browser.wip.patch

Here is a wip rebased on top of the latest changes that has happened in master, and on top of some of the work I have in my local tree (see bug 1044423). Note: it may apply easily on master but I didn't tried yet.

I didn't rebase the touch events related part, as well as the animation when the 'title' is clicked.

It basically removes the .bubble in the titlebar, and use only the .chrome DOM to animate it. It shows how complex the animation should be in order to work smoothly with all the chrome buttons.

It is also compatible with theme-color and does not need to change window_layout.css too much.

Chris, let me know what you think. It goes in the invert direction from your original patch, which tries to move the .bubble as the core of the UI, and instead use the .chrome part as the core of the UI.

There is still a lot more love that is needed to move the localization part of AppTitleBar into AppChrome, but philosophically this is the same idea as what you did in your earlier version. The AppChrome DOM is moved out of the scrollable area, and instead is absolutely positioned.

I also think the scroll events related bits needs to move into AppChrome and not into AppWindow. The changes to AppWindow should be more related to passing the right flags to AppChrome in order to have a nice default Chrome UI for apps and navigable content.

Comment 20

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: animation.browser.wip.patch

Same wip but hide the navigation controls. I'm pretty sure AppChrome DOM will need to be optimized to make it easy to animate everything at the same time. For now the CSS starts to be pretty messy.

Comment 21

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: animation.browser.wip.patch

I changed the DOM and make the CSS a bit easier to maintain / expand in bug 1044423.
I also moved all the parts that are unrelated to this patch in bug 1044423. For example the mozbrowsermetachange color event listening on AppChrome makes more sense in bug 1044423. So this one is really about the animation, and some CSS adjustments to works properly.

Comment 22

[David Scravaglieri [:scravag]]

Video to show the Rocketbar animation when scrolling a web content
http://bit.ly/1qGhem9

Comment 23

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: animation.browser.wip.patch

Latest revision.

Comment 24

[Chris Lord [:cwiiis]]

(In reply to David Scravaglieri [:scravag] from comment #22)
> Video to show the Rocketbar animation when scrolling a web content
> http://bit.ly/1qGhem9

This video looks nice :) It's missing the transition of collapsed rocketbar into rocketbar search screen, but that's easily remedied.

Comment 25

[Chris Lord [:cwiiis]]

Comment on attachment 8461133 [details]
Show/hide rocketbar transitions v2

Removing f? from this, seems things have changed quite a bit. Feel free to re-add if I've misinterpreted the situation.

Comment 26

[Chris Lord [:cwiiis]]

There's not much point in this being assigned to me.

Comment 27

[Ben Francis [:benfrancis]]

Great, looks like this also covers bugs 941176 (view title), 941174 (back/forward), 941173 (reload) and 941172 (stop) - all the work currently assigned to me. So I have re-assigned these to Vivien as well. Thanks.

Comment 28

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: smooth.animation.title.urlbar.patch

Hey Alive,
asking you to review as Etienne is on PTO. It seems like a big patch, but this is mostly because I merged back app_titlebar.js into app_chrome.js.

The rest is some CSS changes in order to translate the element correctly.

Comment 29

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Fwiw there is a lot of UI loves needed here in order to make the minimized version of the urlbar readable for regular users. But this change should probably be done once the statusbar height is set up to 30px instead of 24px. But this is too dangerous to do that in this patch!

Comment 30

[(inactive after 6/18) Alive Kuo [:alive]]

* Will read tomorrow, and FYI I am having a big patch in bug 927862 for you soon, too.

Comment 31

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #30)
> * Will read tomorrow, and FYI I am having a big patch in bug 927862 for you
> soon, too.

oh no! :)

Comment 32

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

For review purpose, please consider that every time you see: app.manifestURL or app.config.manifest I will use app.isBrowser().

Comment 33

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: smooth.animation.title.urlbar.patch

I fixed some tests failures...

Comment 34

[(inactive after 6/18) Alive Kuo [:alive]]

Not reviewed yet..seeing |TypeError: this.navigation is null app_chrome.js:396| after rebasing to master today...is it covered in your patch?

Comment 35

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #34)
> Not reviewed yet..seeing |TypeError: this.navigation is null
> app_chrome.js:396| after rebasing to master today...is it covered in your
> patch?

It is covered in my latest patch (I actually forgot to |git add -u| before resubmitting the patch yesterday :/) The try result is here: https://tbpl.mozilla.org/?tree=Gaia-Try&rev=b3d0a2fbd2ca

I'm actually working on fixing the latest test.

In order to fix the this.navigation error, I change the code in such a way that all the app_chrome.js events that are dedicated to navigation are not set if we use the combinedChrome view.

Comment 36

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: smooth.animation.title.urlbar.patch

I guess it will be clearer if I attached the right patch.

Comment 37

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #36)
> Created attachment 8464602 [details] [diff] [review]
> smooth.animation.title.urlbar.patch
> 
> I guess it will be clearer if I attached the right patch.

OK. This is green now: https://tbpl.mozilla.org/?tree=Gaia-Try&rev=34abfb88303f

Comment 38

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #37)
> (In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails,
> needinfo? please) from comment #36)
> > Created attachment 8464602 [details] [diff] [review]
> > smooth.animation.title.urlbar.patch
> > 
> > I guess it will be clearer if I attached the right patch.
> 
> OK. This is green now:
> https://tbpl.mozilla.org/?tree=Gaia-Try&rev=34abfb88303f

Oups. Fullscreen_statusbar_test.js fails. For some reasons it works locally. Still investigating but it should not prevent this patch to be reviewed.

Comment 39

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #38)
> (In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails,
> needinfo? please) from comment #37)
> > (In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails,
> > needinfo? please) from comment #36)
> > > Created attachment 8464602 [details] [diff] [review]
> > > smooth.animation.title.urlbar.patch
> > > 
> > > I guess it will be clearer if I attached the right patch.
> > 
> > OK. This is green now:
> > https://tbpl.mozilla.org/?tree=Gaia-Try&rev=34abfb88303f
> 
> Oups. Fullscreen_statusbar_test.js fails. For some reasons it works locally.
> Still investigating but it should not prevent this patch to be reviewed.

Yeah! I can reproduce locally if I rebased the patch to master. Seems like something has landed in-between.

Comment 40

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Just realized the fullscreen issues comes become this patch nows collapse with the changes you made in bug 1043599.

Comment 41

[Kevin Grandon :kgrandon]

Comment on attachment 8464602 [details] [diff] [review]
smooth.animation.title.urlbar.patch

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

The code looks good to me, nothing major but please fix a few things before landing:

- Rename the maximized function as it's an action. 

- Ensure that swiping from a fullscreen app continues to work. It looks like we crossed paths there, hopefully it's minor.

::: apps/system/js/app_chrome.js
@@ +556,5 @@
>    AppChrome.prototype.handleLoadEnd = function ac_handleLoadEnd(evt) {
>      this.containerElement.classList.remove('loading');
>    };
>  
> +  AppChrome.prototype.maximized = function ac_maximized(callback) {

nit: when reading this code I see maximized and think that this might be called as a callback, or after a 'maximize' action. I suppose this is because maximized is past tense. Because this actually triggers the actual expansion of the rocketbar, I would recommend renaming this to either 'maximize' or 'expand'. The className, 'maximized' can stay the same. (Unless we want to switch to naming the function 'expand', then the class name should switch to 'expanded'.)

@@ +584,5 @@
> +    window.removeEventListener('rocketbar-overlayopened', this);
> +    this.element.classList.remove('maximized');
> +  };
> +
> +  AppChrome.prototype.isMaximized = function ac_isMaximized() {

I had a nit above, but this naming can stay the same as we are just checking state.

::: apps/system/js/home_searchbar.js
@@ +149,5 @@
>            });
>          };
>  
> +        if (app && app.appChrome && !app.appChrome.isMaximized()) {
> +          app.appChrome.maximized(function() {

Just making a note that this is going to conflict with bug 1045758 (combining home searchbar and rocketbar). Updating is fairly simple so I don't care who lands first, just one of us will need to rebase.

::: apps/system/test/unit/home_searchbar_test.js
@@ +300,5 @@
>            window.dispatchEvent(new CustomEvent('global-search-request'));
>            sinon.assert.calledWith(setInputStub, 'foo');
>          });
>  
> +        test('should not expand the title bar but activate the focus',

Just making a note that this file is going away in bug 1045758. Seems like it should be fairly easy to move this new test case over to rocketbar_test.js though.

::: shared/js/font_size_utils.js
@@ +137,5 @@
>       *
>       * @param {HTMLElement} domNode
>       */
>      _registerHeadersInSubtree: function(domNode) {
> +      if (!domNode) {

Is this already failing in master?

Comment 42

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://github.com/mozilla-b2g/gaia/commit/97858243558614a4ba9d74601f802e2899588427 after 2 fun rebase.

Comment 43

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kevin Grandon :kgrandon from comment #41)
> ::: shared/js/font_size_utils.js
> @@ +137,5 @@
> >       *
> >       * @param {HTMLElement} domNode
> >       */
> >      _registerHeadersInSubtree: function(domNode) {
> > +      if (!domNode) {
> 
> Is this already failing in master?

I will open a followup to investigate the issue. Pretty sure this will go away by itself once .navigation will be removed.

Comment 44

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Seems like I pushed a perma-orange. https://github.com/mozilla-b2g/gaia/commit/a855f29d773cfe21dc15392e0675819833c7409b