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

RESOLVED DUPLICATE of bug 796474

Status

RESOLVED DUPLICATE of bug 796474
5 years ago
5 years ago

People

(Reporter: asuth, Unassigned)

Tracking

({perf})

unspecified
x86_64
Linux

Firefox Tracking Flags

(blocking-b2g:1.4+)

Details

(Whiteboard: [c=handye p= s= u=][priority][p=13])

(Reporter)

Description

5 years ago
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.
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.
Filed bug 959793 for scrollBy changes.
moving to 1.4? for thorough investigation
blocking-b2g: 1.3? → 1.4?

Updated

5 years ago
Blocks: 949585
No longer blocks: 950733
blocking-b2g: 1.4? → backlog
Whiteboard: [priority][p=13]
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.
Depends on: 959793
(Reporter)

Comment 5

5 years ago
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.
Depends on: 796474
No longer depends on: 959793
Blocks: 965019
Keywords: perf
Whiteboard: [priority][p=13] → [c=handye p= s= u=][priority][p=13]
This is indirectly blocking QC CS 1.4.  I don't think it can be backlog.
blocking-b2g: backlog → 1.4?
blocking-b2g: 1.4? → 1.4+
(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.
blocking-b2g: 1.4+ → 1.4?
Andrew, since this is on Gaia side, let me know if e-mail can hit the performance goals for 1.4 without this fixed.
Flags: needinfo?(bugmail)
(Reporter)

Comment 9

5 years ago
We are going to dupe this to the bug that is fixing this problem to reduce confusion.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: needinfo?(bugmail)
Resolution: --- → DUPLICATE
Duplicate of bug: 796474
blocking-b2g: 1.4? → 1.4+

Updated

5 years ago
No longer blocks: 949585, 965019
No longer depends on: 796474
You need to log in before you can comment on or make changes to this bug.