986625 - Improve the speed of scroll position changes

Status: VERIFIED FIXED

(Marketplace Graveyard :: Consumer Pages, defect, P2)

Description

[[e-mail]]

Historically scroll position changes have been needed to be delayed both on b2g and FF android because scrolling too soon during content load causes problems.

For example if you scroll down a page and hit the details page if scrolling to the top happens too soon you end up left halfway down the page.

To fix that there's a delay via setTimeout in place to not call the scroll update too soon. This prevents the issue of being stuck halfway down a new page when the content has been loaded bug does introduce a noticeable delay.

STR from https://bugzilla.mozilla.org/show_bug.cgi?id=976466#c4:

1. Load https://marketplace-dev.allizom.org
2. Scroll down to the bottom of the Popular section.
3. Click the YouTube icon.
4. Even after the detail page is loaded, we wait for a noticeable amount of time at the previous scroll position before jumping up to the top of the page.

This bug is to take a look at whether we can change our approach or look to mitigate the delay so the user has a smooth experience navigating the marketplace on all devices.

Comment 1

[[e-mail]]

I've had another look at this. One possible thought is that we could attempt to scroll immediately and then do the timeout which could check scroll position and only set it again if needed. This means that if it's possible to scroll immediately it will and we can also do it within a short delay if necessary.

Comment 3

[[e-mail]]

PR: is here:

https://github.com/mozilla/fireplace/pull/416/files

QA Notes:

Validating this fix (once it's landed) will require checking the original STR plus ensuring no regressions. Here's a basic STR to ensure setting the scroll position is working as expected:

* Open marketplace. 
* Scroll down
* Note what app is at the top of the current viewport.
* Open a details page
* Ensure details page is opened and you are at the top of the page.
* Press back
* Ensure you are back in the correct position on the homepage.

Comment 4

[Christopher Van Wiemeersch [:cvan]]

Merged: https://github.com/mozilla/fireplace/commit/429071e

If you want this cherrypicked for Tuesday, say something

Comment 5

[[e-mail]]

Additional QA notes: Please ensure this is checked on Android as well as FFOS.

Comment 6

[[e-mail]]

Please try data connections as well as wifi. Slower page loads are likely to impact this:

Currently on android with data I'm seeing the odd mis-fire on the restoration esp on data. But I also discovered bug 989971 which is unrelated. Might mean we should wrap this in caps to keep the timeout for Android, but I'd like to see the outcome for FFOS from QA too.

Comment 7

[[e-mail]]

Added this commit as restoring the min-height changes left out an if statement:

https://github.com/mozilla/fireplace/commit/ce5f5f695c05e6232d235f98b64b925ae06fbf94

Comment 8

[krupa raj[:krupa]]

Since it didn't make it to the tag, moving it to the next milestone.

Comment 9

[[e-mail]]

I've added back the timeout for just android: https://github.com/mozilla/fireplace/commit/5dbabb8475db2346a53f3ee7f43c7c2bc1e1daf7

Platform bug filed here: bug 990525 - I'll file another bug to track removal of the setTimeout blocked by the platform bug.

Comment 10

[[e-mail]]

To summarise where we are with this bug.

This bug comprises three commits:

 * https://github.com/mozilla/fireplace/commit/429071e
 * https://github.com/mozilla/fireplace/commit/ce5f5f6
 * https://github.com/mozilla/fireplace/commit/5dbabb8

As a result of these changes scroll changes with improved perf is available for FFOS and desktop but *not* android.

As a result of this from a QA perspective you should see:

 * Improved perf in scroll position changes in FFOS + desktop e.g. on back etc.
 * Nothing changed in FF Android. 

For android there's a separate bug to track setTimeout removal when the platform bug is fixed.

[1] Android SetTimeout removal bug 990530
[2] Android Platform bug 990525

Comment 11

[Iulian Timis]

The scroll is preserved properly on desktop http://screencast.com/t/3ALFIzI5tPW
but for FF OS, the scroll was not preserved for the following scenario:
1. Perform a search (ex. "app"). Make sure expanded view toggle is on.
2. Scroll to the bottom and press the "More" button.
3. Tap on an app that was loaded after the "More" button was pressed.
4. Tap the back button.

Also, when loading the app details page (on step 3), the page looks like this http://screencast.com/t/GDXpWqs0 , and after we scroll on the phone, the app details page appears.

Comment 12

[[e-mail]]

(In reply to Iulian Timis from comment #11)
> The scroll is preserved properly on desktop
> http://screencast.com/t/3ALFIzI5tPW
> but for FF OS, the scroll was not preserved for the following scenario:
> 1. Perform a search (ex. "app"). Make sure expanded view toggle is on.
> 2. Scroll to the bottom and press the "More" button.
> 3. Tap on an app that was loaded after the "More" button was pressed.
> 4. Tap the back button.
> 

Hmm, ok - so it seems we can't get away from the delay for this. I'm going to talk to cvan about this but we might need to go back to the drawing board on this one.

> Also, when loading the app details page (on step 3), the page looks like
> this http://screencast.com/t/GDXpWqs0 , and after we scroll on the phone,
> the app details page appears.

I don't think the blank page is related. I've seen that prior to these changes on android too. If there isn't an existing bug then we could use one for that thought clearly it's probably a tough one to repro on demand.

Comment 13

[[e-mail]]

I've backed out all the related commits here given it's not working properly without the timeout.

* https://github.com/mozilla/fireplace/commit/6d6db4
* https://github.com/mozilla/fireplace/commit/14fae3
* https://github.com/mozilla/fireplace/commit/c2be87

Comment 14

[Christopher Van Wiemeersch [:cvan]]

I'm still unclear of why this regressed scroll performance on desktop. Every link I click an internal link, the content of the next page loads, and then after ~100-250ms, the scroll position gets reset to the top of the page.

I cannot stress enough how jarring this is (especially considering how this happens every time I click any link).

Comment 15

[[e-mail]]

I think we could certainly look at only doing this just for mobile instead of everything as long as it doesn't cause issues for desktop like it does for mobile without the delay.

That said what we really need to do is work out a solution that fixes this once and for all without any of the hacky timeouts.

I'm un-assigning myself since I don't have bandwidth to look at this presently.

Comment 16

[Christopher Van Wiemeersch [:cvan]]

CCing Dethe because I know he would crush this bug. Feel free to ignore me though :)

Comment 17

[Christopher Van Wiemeersch [:cvan]]

Can product set priority here please?

Comment 18

[Iulian Timis]

I am able to reproduce this issue using the following STR:
1. Load MP-stage app on Inari.
2. Search for an app.
3. Make sure the view toggle is in compact view.
4. Scroll down and tap on an app to load its details page.
5. Hit the back button.

The page is scrolled at the top of the search list.
Ashes ID: dfc01

Comment 19

[Christopher Van Wiemeersch [:cvan]]

PR: https://github.com/mozilla/fireplace/pull/726

Comment 21

[Christopher Van Wiemeersch [:cvan]]

This is still marked cvanwillbuyyouadrink :)

There was a PR that didn't get merged: https://github.com/mozilla/fireplace/pull/726 - muffinresearch had ideas, but we didn't ever get around to talking about them in Portland.

Comment 22

[Kevin Ngo [:ngoke]]

My attempt:

https://github.com/mozilla/fireplace/pull/1016
https://github.com/mozilla/marketplace-core-modules/pull/23/
https://github.com/mozilla/commonplace/pull/60

Now testing on my devices...so not a sure thing yet.

Comment 23

[Kevin Ngo [:ngoke]]

After quick tests on my Flame, my changes seem to work well for both desktop and FirefoxOS without any noticeable jumps or missed scrollTo(0, 0)s.

Comment 24

[Kevin Ngo [:ngoke]]

Tested and merged.

Comment 25

[Valentina Virlics [:vvalentina], Ecosystem QA]

I think there is an issue, related to this fix (https://bugzilla.mozilla.org/show_bug.cgi?id=1133416) that also looks bad in FFOS 1.4(Inari)
STR for FFOS 1.4:
1.go to last app from MP-stage page
2.load app details page 
3.go back to homepage (using go back arrow)
Ashes ID: 662af

Comment 26

[Chuck Harmston [:chuck]]

Closing this, as the bug Valentina reopened this on the basis of in comment 25 was closed as a duplicate of bug 1057103, and is not related to this.

Comment 27

[Valentina Virlics [:vvalentina], Ecosystem QA]

In MP-dev desktop and FF OS 2.2 (Flame)(more obvious) following the steps from comment #0, the scroll speed is improved but the top of the page is displayed for a few seconds and then the position from where the user accessed the app.This is how it should look the fix? Thanks! 
Screencast http://screencast.com/t/qH8O9eDVmNgk

Comment 28

[[e-mail]]

(In reply to ValentinaP from comment #27)
> In MP-dev desktop and FF OS 2.2 (Flame)(more obvious) following the steps
> from comment #0, the scroll speed is improved but the top of the page is
> displayed for a few seconds and then the position from where the user
> accessed the app.This is how it should look the fix? Thanks! 
> Screencast http://screencast.com/t/qH8O9eDVmNgk

Yeah - I think that's due to when the event fires. I wonder if we can try restoring the scroll sooner if the required scrollPosition is less than the height of the content? 

Beyond things like that I think bigger changes would be needed e.g. Each "view" having it's own scroll bar and not relying on the viewport scrollbar. Then we could potentially restore a view complete with it's scroll position as opposed to loading content and subsequently fixing the scroll position in viewport. But it would take some prototyping and investigation. I can imagine view management would be non-trivial.

I'll needsinfo Kevin, but I think it'll make sense to work on improving this in separate bugs.

Comment 29

[Kevin Ngo [:ngoke]]

Given this long history of this bug, it'd be good to file follow-up issues as a separate bug to relieve this one. Looking at the screencast, it looks good enough to me for now.

Comment 30

[Christopher Van Wiemeersch [:cvan]]

(In reply to Stuart Colville [:scolville] [:muffinresearch] from comment #28)
> (In reply to ValentinaP from comment #27)
> > In MP-dev desktop and FF OS 2.2 (Flame)(more obvious) following the steps
> > from comment #0, the scroll speed is improved but the top of the page is
> > displayed for a few seconds and then the position from where the user
> > accessed the app.This is how it should look the fix? Thanks! 
> > Screencast http://screencast.com/t/qH8O9eDVmNgk
> 
> Yeah - I think that's due to when the event fires. I wonder if we can try
> restoring the scroll sooner if the required scrollPosition is less than the
> height of the content? 
> 
> Beyond things like that I think bigger changes would be needed e.g. Each
> "view" having it's own scroll bar and not relying on the viewport scrollbar.
> Then we could potentially restore a view complete with it's scroll position
> as opposed to loading content and subsequently fixing the scroll position in
> viewport. But it would take some prototyping and investigation. I can
> imagine view management would be non-trivial.

On desktop, that gets weird. Because then the user can't use keyboard navigation immediately to navigate up/down on the page. The user has to first click the area for the browser to know that it should get focus. And you can't call `container.focus()` from JS or give it a `tabindex` or anything like that. I was playing with this kind of stuff with flexbox when I was working on the Metropolis demos. Just something to keep in mind, if you wanted to entertain that idea.

Comment 31

[krupa raj[:krupa]]

verified fixed on fxos and desktop.