993346 - Show/hide Rocketbar by scrolling the page

Status: VERIFIED FIXED

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

Description

[Ben Francis [:benfrancis]]

As a user I want to show/hide the Rocketbar by scrolling a page so that the Rocketbar just gets out of the way.

Comment 1

[Ben Francis [:benfrancis]]

This is going to be tricky to implement well.

We can implement it in a similar way to the current browser app where when we hide the address bar we resize the content iframe all in one go, off the bottom of the screen, then translate it up the screen as we animate the address bar away. This works but is pretty janky.

A better way might be to use scrollgrab and have an outer scrollable div which extends all the way to the top of the page, behind the Rocketbar. That way we don't need to resize and translate the content frame, we just scroll the outer element first and listen for scroll events from that. We just collapse the Rocketbar when these scroll events pass a certain threshold.

Unfortunately this approach is probably blocked by bug 950934 because it relies on using APZC in the main process which is not currently possible.

Comment 2

[Ben Francis [:benfrancis]]

WIP patch here https://github.com/benfrancis/gaia/commit/bef4f85a036141488db30a3566517276955d0143#diff-5699b1000bc807368718bfddf7ef58c3R410

This doesn't yet include resizing and shifting the AppWindow down the page. This is going to be tricky because the transforms are likely to conflict with the transforms used by edge gestures. Going to chat more with Etienne in the morning.

Comment 3

[Ben Francis [:benfrancis]]

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

OK, this isn't perfect but I think it's a pretty cool implementation for user testing.

I managed to cheat my way around not conflicting with the sheets transitions by transforming the AppWindow's iframe rather than the AppWindow element itself, which actually works pretty well and has the side effect of not breaking the current bottom navigation bar.

As per the UX spec, there's a bit of a buffer when scrolling up before an expand transition starts to allow for user self-correction (and to prevent some fun bouncing address bar bugs).

Browser content shows the Rocketbar expanded by default, whereas apps have it collapsed by default. There are some edge cases where you'll see some glitches when switching between sheets of different types or different scroll positions, but I hope we can iron out those wrinkles after user testing.

I actually think the sheet transitions look nicer if we transform the whole #windows div, but the performance is really bad, and besides this is closer to what UX asked for.

This patch has the nice side effect that it fixes one part of the jank of the task manager transition by making the app screenshot not overlap the Rocketbar when entering the task manager, but there's still room for improvement there.

I'm not currently resizing the content frame when shifting it down the page and I actually think it works pretty well. But if UX play with this and don't like that then we could address that in a follow-up.

Let me know what you think.

Comment 4

[Dale Harvey (:daleharvey)]

What happens when you take out the homescreen exception? we are always gong to have an expanded rocketbar on the homescreen right?

There is a bug with momentum scrolling, do a big scroll up, focus the rocketbar and the momentum scrolling will hide it again, I assume we will want to cancel the active scroll when the rocketbar expands

Another issue is that sites are going to be able to 'hijack' the behaviour by scrolling an inner frame right? I dont think thats a problem we need to worry about now though, it doesnt block the user from doing anything.

It follow UX spec, but I dont think / know why we expand the rocketbar on scroll up at all, I guess when the back button is there it 'may' be slightly more convenient but its fairly annoying for stuff to keep moving while you scroll the page (either way irrelevant to the patch, will file a follow up)

We are going to want to revisit a few ways of doing thing, moving the content under the users finger is distracting and it is somewhat janky, but the implementation is small doesnt touch a lot which is really nice and it performs pretty well, its certainly good enough for user testing imo

If you could take a look at the momentum scrolling I think thats worth fixing before landing, I am gonna give it a more thourough test in the morning but this is looking great

Comment 5

[Ben Francis [:benfrancis]]

(In reply to Dale Harvey (:daleharvey) from comment #4)
> What happens when you take out the homescreen exception? we are always gong
> to have an expanded rocketbar on the homescreen right?

You see the position of the homescreen animate along the y axis when you switch the homescreen. The expection is to turn of the transition. We do need the Rocketbar to be expanded on the homescreen but it's a different kind of expanded (with statusbar icons shown) and needs to be behave slightly differently so I think we need to address that separately (soon) when we sort out the resize of the homescreen frame.
> 
> There is a bug with momentum scrolling, do a big scroll up, focus the
> rocketbar and the momentum scrolling will hide it again, I assume we will
> want to cancel the active scroll when the rocketbar expands

As discussed in IRC we can't cancel the scroll, but we can cancel the Rocketbar transition. I've added a fix for that. I also added a temporary fix to make sure we call deactivate when you focus on a web page so that we don't get stuck in this.active, but the proper fix for that is adding in the semi-transparent scrim.

> Another issue is that sites are going to be able to 'hijack' the behaviour
> by scrolling an inner frame right? I dont think thats a problem we need to
> worry about now though, it doesnt block the user from doing anything.

The great thing about this design is that there's always an on-screen override because you can manually drag the Rocketbar.

> It follow UX spec, but I dont think / know why we expand the rocketbar on
> scroll up at all, I guess when the back button is there it 'may' be slightly
> more convenient but its fairly annoying for stuff to keep moving while you
> scroll the page (either way irrelevant to the patch, will file a follow up)

Being able to show the Rocketbar without scrolling to the top of the page was one of the most requested features which may be one reason. We do that in Fennec. But in this case you can do it manually by swiping the Rocketbar so I don't think it's so important. But this is a UX call.

> We are going to want to revisit a few ways of doing thing, moving the
> content under the users finger is distracting and it is somewhat janky, but
> the implementation is small doesnt touch a lot which is really nice and it
> performs pretty well, its certainly good enough for user testing imo

Yeah I agree and I discussed this with Kats and Botond. This is the approach I know will work. The next stage might be to try using scrollgrab and have an outer scrolling div outside the iframe with padding at the top behind the Rocketbar. There are some edge cases where that doesn't work brilliantly. The best long term solution might be linking CSS properties with scroll position, but that isn't coming any time soon http://chrislord.net/index.php/2013/12/16/linking-css-properties-with-scroll-position-a-proposal/

> If you could take a look at the momentum scrolling I think thats worth
> fixing before landing, I am gonna give it a more thourough test in the
> morning but this is looking great

Done, let me know what you think.

Comment 6

[Dale Harvey (:daleharvey)]

Comment on attachment 8407846 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/18371

Looking good, nit comment in there about newTabPage, we should start leaning away from defensive coding, but its minor and if it cant be taken out right now I can take a look with the scrim

Comment 7

[Etienne Segonzac (:etienne)]

Comment on attachment 8407846 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/18371

Pretty good!

Here are my comments (nits are on github):
* on-homescreen and rockerbar-expanded should be set on the #screen element to be consistent with .locked, .utility-tray, .software-button-disabled, .active-statusbar and the likes. And the Rocketbar and HomescreenWindow should do those changes respectively (not the AppWindowManager).

* ideally we shouldn't need the .on-homescreen class (I think Alive will feel pretty strongly that we don't). The homescreenopened and appopened even should suffice.

* something is racy/weird regarding fullscreen apps, I can pretty easily get it to show an empty space on top of a fullscreen app. Might be due to the fact that you're removing the rule matching the fullscreen app's element (as opposed to its iframe)

Comment 8

[Etienne Segonzac (:etienne)]

To clarify, I think we should have only 1 added class for the expanded rocket bar.
And this being really similar to the .active-statusbar I think the 2 should be handled the same way (both on #screen or both on #windows).

I'm leaving the final word for Alive :)

Comment 9

[Ben Francis [:benfrancis]]

Comment on attachment 8407846 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/18371

Thanks for the review Etienne,

I had addressed your comments. HomescreenLauncher now sets .on-homescreen and Rocketbar sets .rocketbar-focused on #screen. I can't think of a simple way to do away with .on-homescreen entirely.

I have fixed the bug with fullscreen apps.

Comment 10

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

My 2$:
1. Let Rocketbar put rocketbar-related classes.
2. Do not swallow event in a middleware. Let LayoutManager decide it's time to ask AppWindowManager to resize current app or not.

Others are what etienne said.
If we cannot avoid to have a global class please put all of them on #screen.

Comment 11

[Etienne Segonzac (:etienne)]

Comment on attachment 8407846 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/18371

I'll have a quick last look once Alive's comment are addressed too.
We all agree on the solution now, should be straightforward enough :)

Feel free to ping me on IRC to expedite the review.

Comment 12

[Ben Francis [:benfrancis]]

Thanks for the review Alive.

(In reply to Alive Kuo [:alive][NEEDINFO!][God bless Taiwan.] from comment #10)
> 1. Let Rocketbar put rocketbar-related classes.

Done.

> 2. Do not swallow event in a middleware. Let LayoutManager decide it's time
> to ask AppWindowManager to resize current app or not.

I didn't add this hack here, I think it has been around for a long time. I'm not sure this is the right approach so I filed bug 999463 to discuss this as a follow-up. I hope that's OK.

> If we cannot avoid to have a global class please put all of them on #screen.

Done. This also simplifies the statusbar logic.

Comment 13

[Ben Francis [:benfrancis]]

Comment on attachment 8407846 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/18371

Hi Etienne, could you take another look to make sure this is OK?

Comment 14

[Etienne Segonzac (:etienne)]

Comment on attachment 8407846 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/18371

I'm off for tonight, but gaia never sleeps :)
Forwarding the review to Alive so the final review can happen during the Taipei shift :)

Comment 15

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

Comment on attachment 8407846 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/18371

r+ with nits on github addressed.
Do we need to enable scroll for popup as well?
If so, we will need listen to popupscroll if bug 916709 is completed.

Comment 16

[Ben Francis [:benfrancis]]

Thanks for the review Alive. I addressed all your comments this morning, Travis passed, but then the tree closed. The tree has just re-opened but now there are conflicts with bug 997876.

I have rebased and resolved the conflicts so we should now wait for Travis to pass again before merging.

Comment 17

[Ben Francis [:benfrancis]]

Lint failure and some new merge conflicts. Let's try again.

Comment 18

[Ben Francis [:benfrancis]]

Merged into master https://github.com/mozilla-b2g/gaia/commit/3e718bb478027f6b68f75972d03bdef7e574d0f7