1120884 - If the homescreen grid animates a reduction in size when at the extent of its scroll, the scroll position changes jankily

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Homescreen, defect)

Description

[Chris Lord [:cwiiis]]

If you make a change to the grid that reduces it in size while the grid is at the extent of its scroll position, the scroll position will update to the new, reduced size.

This is a problem if an animating element causes the overflow to reduce, as the main thread can't keep up with async animations in this case.

This is easily reproduced if you have a group at the bottom of the screen with one row, you scroll to the bottom, then collapse that group.

Comment 2

[GitHub Autolander Bot]

Attachment: [gaia] Cwiiis:bug1120884-smooth-grid-shrinking > mozilla-b2g:master

Comment 3

[Chris Lord [:cwiiis]]

Comment on attachment 8573227 [details] [review]
[gaia] Cwiiis:bug1120884-smooth-grid-shrinking > mozilla-b2g:master

Sorry for all these reviews, trying to hit my polish bug quota :)

Comment 4

[Chris Lord [:cwiiis]]

Kevin's review:

"This is looking pretty good code-wise (there is a jshint issue that you should fix), but I think we need just a little bit more tuning before landing.

The 750ms timeout is quite big, and something I tried cause the page to jump back to where it thinks it should be. Try this - with a bunch of expanded groups, scroll down the page slowly, collapsing groups as you're scrolling. The page will sort of jump to weird areas. I'm wondering if we can clear the timeout on scroll somehow. Maybe either with a scroll handler, or a temporary touchstart handler."

So, we can't cancel the timeout because the work being done in the timeout is the work we can't skip (the shrinking of the view). I suppose though that we could delay it further if the user is actively scrolling, that might work. Let me give that a go...

Comment 5

[Chris Lord [:cwiiis]]

Comment on attachment 8573227 [details] [review]
[gaia] Cwiiis:bug1120884-smooth-grid-shrinking > mozilla-b2g:master

This is about as good as I can do I think... Every time the container resizes, any async scroll, including scroll-behaviour smooth scrolling, is cancelled and the container will jump to whatever the last sync scroll position was.

I work around this by listening to the scroll signal and having a generous 1-second cool-down after scrolling before resizing the grid. Irritatingly, any number smaller than about a second pretty much didn't work, it seems content is either not updated often, or is really busy during expanding/collapsing/scrolling (the latter I think is the case).

I'm very much open to ideas to improve this, this feels ok pretty much, except when the grid shrinks, you can just scroll down to the now-empty space, and if you leave it there, it'll (after a second) jump to the correct, smaller size.

This wouldn't really be a problem if resizing the view didn't cancel scrolling, so I'm going to open a bug for that.

Comment 6

[Kevin Grandon :kgrandon]

Comment on attachment 8573227 [details] [review]
[gaia] Cwiiis:bug1120884-smooth-grid-shrinking > mozilla-b2g:master

To be honest I think I actually prefer the weirdness of the first approach compared to the scroll listener ant timeout. This one is more weird because you can potentially be scrolling around in a large area where the group used to be, before it collapses.

If you feel that this is better though - go ahead and flag me and I can take another look. I think I'd rather just land the first approach though.

Two things I'm curious about:

1 - What if we just update the height after a timeout? It seems the only time the user might experience something weird is if they try to scroll down after the smooth scroll?

2 - Would it help at all, or is it performant, to update the grid height inside of an rAF until we finish the transition?

Comment 7

[Chris Lord [:cwiiis]]

Comment on attachment 8573227 [details] [review]
[gaia] Cwiiis:bug1120884-smooth-grid-shrinking > mozilla-b2g:master

ok, I think this approach trumps both prior tries. The issue I found was that setting the height of the container *after* calling scrollTo with the 'smooth' behaviour cancels out the scrollTo. So we need to set the height before calling scrollTo to have this work reliably, but we obviously need to delay the actual height change... So I do this by adding a CSS transition of 0s with a 0.5s delay.

If you scroll down immediately after collapsing the group, you can get the scroll position to jump back up to the group (and similarly, I think it's possible to scroll down into the empty space before the height is set), but now the window of time within which you have to do that is tiny and it's actually quite difficult.

The animation looks smooth and completes in about 0.5s, which is a pretty tiny window of opportunity to mess with it, I think.

Comment 8

[Kevin Grandon :kgrandon]

Comment on attachment 8573227 [details] [review]
[gaia] Cwiiis:bug1120884-smooth-grid-shrinking > mozilla-b2g:master

Ok, I think this works for me. I'm still getting some jumpiness, but I think that is related to the other group collapsing/attention patch. This looks good to me, thanks!

Comment 9

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/6f45c01746a2d0aeae63e68e1d4e5aa465a02f94

Comment 10

[Shine(Leave from Mozilla)]

Hi reporter,
I can't fully understand the phenomenon of issue. Could you please provide a repro video?
Thanks!

Comment 11

[Chris Lord [:cwiiis]]

(In reply to Shine from comment #10)
> Hi reporter,
> I can't fully understand the phenomenon of issue. Could you please provide a
> repro video?
> Thanks!

I can't easily record a video right now, but STR:

1- Scroll to the bottom of the home-screen
[1.1] - If a group collapse arrow is not visible, drag the last icon into a new group at the bottom and start from step 1.
2- Press collapse arrow

Expected:

The vertical scroll position will smoothly animate to the new lowest position (which will be shorter than the previous one due to the collapsed group).

Prior to the patch:

The vertical scroll position would jankily animate to the new lowest position.


There should be a very obvious difference between the two, at least on a Flame device (or similar).

Comment 12

[Hermes Cheng (inactive after July 27, 2015)]

(In reply to Chris Lord [:cwiiis] from comment #11)
> (In reply to Shine from comment #10)
> > Hi reporter,
> > I can't fully understand the phenomenon of issue. Could you please provide a
> > repro video?
> > Thanks!
> 
> I can't easily record a video right now, but STR:
> 
> 1- Scroll to the bottom of the home-screen
> [1.1] - If a group collapse arrow is not visible, drag the last icon into a
> new group at the bottom and start from step 1.
> 2- Press collapse arrow
> 
> Expected:
> 
> The vertical scroll position will smoothly animate to the new lowest
> position (which will be shorter than the previous one due to the collapsed
> group).
> 
> Prior to the patch:
> 
> The vertical scroll position would jankily animate to the new lowest
> position.
> 
> 
> There should be a very obvious difference between the two, at least on a
> Flame device (or similar).

I tried Flame master and 2.2, and the collapsing animation is much smoother at device with master build which has this patch.  As video [1], 2.2 without this patch has bad animation experience. I think it is the main difference, isn't it.

Besides, I also found that when expanding the last group, the behavior is a little different between master and 2.2. As video [2], the position of the last group will be moved after expanding. Do you know which bug's patch is the reason?

[1] collapse at 2.2: https://www.youtube.com/watch?v=9yyMTw488N8
[2] expand at mater: https://www.youtube.com/watch?v=aip_F5eh3lY

Comment 13

[Chris Lord [:cwiiis]]

(In reply to Hermes Cheng[:hermescheng] from comment #12)
> (In reply to Chris Lord [:cwiiis] from comment #11)
> I tried Flame master and 2.2, and the collapsing animation is much smoother
> at device with master build which has this patch.  As video [1], 2.2 without
> this patch has bad animation experience. I think it is the main difference,
> isn't it.

Spot on :)

> Besides, I also found that when expanding the last group, the behavior is a
> little different between master and 2.2. As video [2], the position of the
> last group will be moved after expanding. Do you know which bug's patch is
> the reason?
> 
> [1] collapse at 2.2: https://www.youtube.com/watch?v=9yyMTw488N8
> [2] expand at mater: https://www.youtube.com/watch?v=aip_F5eh3lY

This is bug 1106544.

Comment 14

[Hermes Cheng (inactive after July 27, 2015)]

Chris, thanks! 

(In reply to Shine from comment #10)
> Hi reporter,
> I can't fully understand the phenomenon of issue. Could you please provide a
> repro video?
> Thanks!

Shine, as Chris said, it is not easy to verify, but I think you can compare the difference between master and 2.2. If the collapsing animation is much smoother when using master, you can mark this bug as verified.

Comment 15

[Shine(Leave from Mozilla)]

Attachment: Verify video

Thanks a lot, Hermes.
The verified result is Pass. I have compared the difference between master and Flame 2.2. On master, the collapsing animation is much smoother now.
See attachment: Verify_video.MP4
Rate 0/10

Flame 3.0 build: (Pass)
Build ID               20150315160203
Gaia Revision          d4177902b04b8fedcb7df9a30ae6e9677e03d2d4
Gaia Date              2015-03-13 15:58:35
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/af68c9c0e903
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150315.192711
Firmware Date          Sun Mar 15 19:27:22 EDT 2015
Bootloader             L1TC000118D0

Compared build:
Flame 2.2:
Build ID               20150315162500
Gaia Revision          a6b2d3f8478ec250beb49950fecbb8a16465ff6f
Gaia Date              2015-03-15 14:33:22
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/18619f8f6c5c
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150315.195030
Firmware Date          Sun Mar 15 19:50:42 EDT 2015
Bootloader             L1TC000118D0

Comment 16

[Chris Lord [:cwiiis]]

Comment on attachment 8573227 [details] [review]
[gaia] Cwiiis:bug1120884-smooth-grid-shrinking > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: Janky animation when collapsing the last group on the homescreen
[Testing completed]: Manual testing, been on master for a while with no reports of bad behaviour
[Risk to taking this patch] (and alternatives if risky): Low risk
[String changes made]: None

Comment 17

[Hermes Cheng (inactive after July 27, 2015)]

NI myself for following verification.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/12a4d57860f45c0ee5148af8238b6491de5ff0aa

Comment 19

[Hermes Cheng (inactive after July 27, 2015)]

verify 2.2 with below build

uild ID               20150323162503
Gaia Revision          e54c4ed1cc188f70ddf1156534d364005dc45490
Gaia Date              2015-03-23 19:09:26
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/7ba1778d237b
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150323.200543
Firmware Date          Mon Mar 23 20:05:54 EDT 2015
Bootloader             L1TC000118D0