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.

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.

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.

Merged: https://github.com/mozilla/fireplace/commit/429071e If you want this cherrypicked for Tuesday, say something

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

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.

Added this commit as restoring the min-height changes left out an if statement: https://github.com/mozilla/fireplace/commit/ce5f5f695c05e6232d235f98b64b925ae06fbf94

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

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.

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

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.

(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.

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

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).

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.

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

Can product set priority here please?

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

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

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.

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.

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.

Tested and merged.

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

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.

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

(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.

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.

(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.

verified fixed on fxos and desktop.