959782 - [email][APZ] e-mail's infinite scroll mechanism causes noticeable jumps or jerky behaviour when scrolling under new APZ scrolling behaviour

Status: RESOLVED DUPLICATE of bug 796474

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[Andrew Sutherland [:asuth] (he/him)]

As foreseen by :bkelly, the changes to APZ scrolling behaviour make e-mail sad.  Specifically, when you are scrolling up (towards more recent messages) from older messages, we will try and cram new messages into the DOM as you scroll.  The current implementation inserts new elements into the DOM and performs what it thinks is an atomic update of scrollTop.  (scrollTop += STUFF)

However, it's not actually atomic, because the whole point of async-pan-and-zoom (APZ) is that it's doing super smooth stuff on another thread regardless of what the main thread is up to.  So our scrollTop read is wrong, but our write still has an effect.

The general options are:

1a) No longer play coordinate-space games: implement bug 796474 so we force the page to be large enough to hold all the messages we know about.  Have our infinite scroll-logic continue to pre-load and pre-populate the DOM near the current scrolled position of the viewport, but just at fixed positions.  This requires some minor back-end work for us to know how many messages we've got.

1b) A variation on the above where we never remove coordinate space 'above' our current position.  So our scrollHeight would continually increase as we scroll down.  

2) Find some way to get atomic fix-ups out of the platform.  Like maybe if we used window.scrollBy (https://developer.mozilla.org/en-US/docs/Web/API/window.scrollBy) the platform could just magically do what the code wants.

3) Avoid manipulating scrollTop while we're scrolling.  This assumes we can get reliable information on when we start scrolling and are able to accurately predict if we're going to start moving again soon.  Unless the platform starts giving us a lot more events, this would probably result in worse behaviour.  You'd be scrolling up, you'd hit the "wall" where we show the search box affordance.  Then a bunch of messages would show up out of nowhere.


Also related is that I think our scrollTop touching is doing something weird to the velocity modeling of APZ/scrolling.  When we add more stuff, there seems to be an instantaneous change in velocity (a jerk? :jrburke was a physics major, please weigh in ;).

So I suggest we go with 1a and avoid hacktown.  But if there's a way to get the platform to magically keep supporting what we do right now, we should stick with that.

Requesting blocking since there's no way someone else doesn't file this as a blocker.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

I agree that it would nice to implement scrollBy so that it played nicely with the APZ code. I can file a bug on that. However it won't be easy to do and will probably have other side-effects that we'll have to sort through.

(In reply to Andrew Sutherland (:asuth) from comment #0)
> Also related is that I think our scrollTop touching is doing something weird
> to the velocity modeling of APZ/scrolling.  When we add more stuff, there
> seems to be an instantaneous change in velocity (a jerk? :jrburke was a
> physics major, please weigh in ;).

Yeah we currently have bug 921020 on file, which will actually stop the fling entirely when you set the scroll offset. Currently it just carries on with the fling animation but the velocity curve will be disrupted by the scroll change. Another reason to go with 1a, or to tie scrollBy into the animation code so that it smoothly continues from where it was before.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Filed bug 959793 for scrollBy changes.

Comment 3

[Preeti Raghunath(:Preeti)]

moving to 1.4? for thorough investigation

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Making this depend on bug 959793 as it would benefit from that. However I think implementing that would be very tricky to get right and would probably regress other use cases so I'd very much prefer going with approach 1a from comment 0.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

I think our consensus at our weekly meeting yesterday is now just to fix bug 796474.  We think we can potentially do this in the v1.4 timeline now that we are probably bumping background sending to v1.5.

Adjusting the dependency since magic scrollBy sounds hard for platform to implement and still would just be papering over a deficiency in how email is doing things.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

This is indirectly blocking QC CS 1.4.  I don't think it can be backlog.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Milan Sreckovic [:milan] from comment #6)
> This is indirectly blocking QC CS 1.4.  I don't think it can be backlog.

No - per a drivers' decision, QC cannot claim all bugs under a meta bug are blockers. Individual bugs need to be marked as CS blockers in order for them to block.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Andrew, since this is on Gaia side, let me know if e-mail can hit the performance goals for 1.4 without this fixed.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

We are going to dupe this to the bug that is fixing this problem to reduce confusion.