Virtual Scroller

[marcustyphoon]

Yeah, this is just an excuse to use the tech office category again.

---

Thoughts so far (yay, reading minified react comes in handy for a real use case for once):

• Posts are removed from their cell containers when they go off of the screen and are re-added when the cells come back on the screen. (I think Tumblr's algorithm for this is probably too aggressive at the moment—I would run it more like a scheduled task during downtime than as a declarative effect on scroll—but I digress).
• Cells remember their height when their contents are removed and the vertical position of each cell is set to the sum of all preceding cells.
• XKit Rewritten processes posts when they're added to the page to determine whether to modify them, including hiding them or shortening them. This is sort of instant (it's within one event loop, so the browser doesn't render the page twice, in most cases).
• We don't want the positions of everything to go nuts when we e.g. scroll up.

It seems reasonable that, in theory, XKit's current processing might be able to avoid these scrollbar shenanigans. Specifically: if you scroll up, a post renders in with its full height, the virtual scroll container rerenders (moving everything down), XKit processes the post and hides it, the cell height becomes 0, the resize observer fires, the virtual scroll container rerenders (moving everything back up)... and then the javascript event loop ends and the browser actually renders the page for the first time, then nothing actually changed re: scroll position.

I am not actually 100% clear on whether this is working or not, at the moment. I thought it wasn't, but it actually might be (some things, like Youtube and Spotify embeds, definitely cause visible scrollbar jank at the moment even without XKit—I might just have been observing that).

Either way, it's probably ideal to avoid this code being fired multiple times at all. Placing XKit hidden classes on the cell elements themselves would make the first-time render of a hidden post have the correct height, presumably, which seems better.

[marcustyphoon]

Interesting thought (largely Show Originals-specific): do we want to hide the cells completely? (This does not appear to break j/k scrolling, though we would have to be careful while there's an active A/B).

Consider:

• If we hide a post (its "height" becomes zero), and then it goes "offscreen" and gets its contents unmounted, and then we unhide that post, we want to trigger the visibility change handler and resize observer. This has to work.

• As a further performance optimization, ideally if we hide a post and then it goes "offscreen" and then the user scrolls over it, we don't want to trigger a mount of that component, since we're hiding it anyway.

Assuming putting display:none on the cell itself causes it to no longer trigger the intersection observer used for visibility, that seems better for the second bullet point, so long as it fulfills the first bullet point.

(Oh, also, obviously we need to ensure that hidden posts do indeed have height zero; if we hide a cell and it doesn't update its height, that's bad.)

[marcustyphoon]

Further testing, and something I should have realized: a significant issue is cells not having height zero even though they're hidden. It may be that if one display:nones a cell, it doesn't trigger the resize observer. It's probably not realistic to hide a cell's contents, let React do its thing, then hide the cell, at least unless we implement a fancy timeline element hiding utility.

I think it's probably unsolvable that unhiding elements will do at least some wonky things with the scroll bar; no code can expand all of the hidden elements back to their correct sizes when the elements aren't even there.

Okay, well, that's a lie: if we needed to hide and unhide elements frequently and thus wanted to do so smoothly, we could probably store the post height as the data attribute value and do an unhide cell + hardcode height -> requestAnimationFrame -> remove hardcoded height process when unhiding. There aren't any places I can think of in the current codebase where it matters, though, so whatever. ("toggling Show Originals from "original" to "all" is, conveniently, only something you can do at the top of the timeline, so it's not really noticeable that posts are popping downward as you scroll down after doing so).

(Random aside: Scroll to Bottom is much more eyestrain-triggering with the virtual scroller. Inevitable, I think. Oh well.)

[marcustyphoon]

Problem (that I should have seen coming) and also solution: cells often get their classlists updated (presumably for cellVisible toggling), which overwrites classes added by XKit.

Simple solution: set a data attribute instead. Whee.

(That makes me think, like, oh, if we used data-xkit-hidden="<comma separated list of scripts that hid this post>", and a post hiding utility... no, no, I'll stop.)

[marcustyphoon]

An unfortunate small performance regression:

Some of the experimental virtual scroller code makes use of requestAnimationFrame. This is cool! Unfortunately for us, what this means is that Redpop is doing DOM mutations inside an rAF callback.

Recall that our pageModifications engine, as per #407 (and its predecessor #275):

will compile an array of every mutation between frames, and queue onBeforeRepaint() for before the next frame using Window.requestAnimationFrame()

There is a subtle condition here: requestAnimationFrame will queue its callback to run right before the next frame is rendered... unless you're already in the rAF callback "right before" said frame, at which point it will queue its callback for right before the one after that. This, of course, makes sense: a user repeatedly calling rAF is trying to step through each frame in order to, you know, run an animation.

This means, though, that in the case where the DOM mutations we're trying to react to are called using rAF, and are thus in an rAF callback, we have no way* of scheduling pageModifications after all Redpop mutations but before the DOM gets painted, rendered, etc like we used to. If we use rAF, our modifications will occur one frame later, so the user will see one frame of unmodified content before the modified view (causing a double reflow, etc, as we used to always have before 454b268 and #416 and stuff). If we don't use rAF, we basically have to just handle the mutations "now,"* which would mean running all of the pageModifications callbacks multiple times.

Having a choice between a few milliseconds of wasteful javascript and a frame of UI flicker isn't the worst thing in the world—it's really only noticeable in certain circumstances, like when using Scroll to Bottom—but it is too bad to lose the "best of both worlds" option for such a subtle reason.

*I have a trick involving repeatedly queueing a new microtask; it doesn't really help here.

[marcustyphoon]

Thought: if, during every task on which there is a DOM mutation, one queued a few microtasks and then requested an animation frame, that animation frame callback task might be sequenced after the animation frame callbacks triggered by invocations of requestanimationframe that occurred earlier in the task, i.e. the ones Redpop calls to render a post in a cell. This would be the optimal time to process those posts (instantly and once).

One challenging way to do this would be to create a state machine that can determine if the current requestAnimationFrame callback is indeed that one based on keeping track of rAF calls that have not yet been resolved. A simpler one would probably be calling this particular rAF when the cell elements get rendered. That might be as simple as setting a flag when pageModifications detects an added cell, the flag meaning "do not cancel and re-queue the current animation frame."

Of course, if this works, it would be nearly impossible to explain what the code in question is supposed to do.

There is also some probability that simply never cancelling and re-queueing a queued animation frame would do this, as my original implementation of the requestAnimationFrame-based "debounce" of the pageModifications callback declined to do. This would, arguably, be fairly elegant.

On the other hand, the current code provides a stronger guarentee of instant processing of posts, and I think that's pretty worthwhile.

[marcustyphoon]

We could get a small (and I mean small) performance improvement by not re-processing posts when the virtual scroller remounts them on scroll in scripts that only make changes to the cell element (so, scripts that hide posts, basically).

This would presumably involve something like excludeClass... but we already have something called that, and it does something different, so an obvious way to do this really cleanly doesn't stand out to me.

But in general, this should have essentially no impact and thus I don't think it would even be noticeable. Scenarios where one would consider this basically just involve parsing timelineObject data, which should be effectively instant, and getting that data is pretty quick too.

(This brings up that we could also cache timelineObject data on cells. I did try it and it's rather annoying to code, though, so eh.)