AprilSylph / XKit-Rewritten

🧰 The enhancement suite for Tumblr's new web interface
GNU General Public License v3.0
294 stars 48 forks source link

Vanilla Video: May cause posts to overlap #1646

Open marcustyphoon opened 6 days ago

marcustyphoon commented 6 days ago

Platform

MacOS 14.4.1 arm64

Browser

Chrome 130.0.6723.70

Addon version

v1.0.4

Details

I'm not 100% certain this is a Vanilla Video problem—honestly root-cause-wise it probably isn't, if I had to guess—but it's possible for posts to wind up overlapping each other in certain cases when endless scrolling is turned on and the upper post contains a video. I've definitely observed this with Vanilla Video enabled, and tweaking its timing (ex: addedNode.parentElement?.matches(cellSelector) || addedNode.querySelector('video') in mutations.js' run-callbacks-instantly query) seems to make it occur a lot more. (I seem to recall it happening with Vanilla Videos disabled, though, too.)

If I had to guess at the cause of this, the virtual scroller cell code sets isVisible on a delay, its onResize method doesn't fire if it's not set yet, something something race condition. Turning off the browser cache seems(?) to make it occur less, so maybe if a video source is in the cache, our code and the cache are fast enough to have the post at the correct height before the isVisible set rAF delay, which means the resize observer never fires... well, I don't know why that would result in the post being the wrong height, so it's probably more complicated than that if it is that.

In any case, as you can probably infer, I don't have an entirely reliable reproduction for this, unfortunately.

If my inferences based on "it seems to happen more/less if x" are actually right, the "fix" might be (in a vague sense) slowing down our processing, but I don't really want to do that in the sense that I don't want to let Tumblr's video element render if it's avoidable. I would rather add a delayed task that triggers Tumblr's resize code, if possible (although actually, say, adding a pixel to the post height and making everything shift would be bad).

edit: the virtual scroller cell code sets isVisible on a delay, its onResize method doesn't fire if it's not set yet doesn't seem like the correct race condition, potentially, but I haven't finished looking into it by editing the frontend code yet.

marcustyphoon commented 6 days ago

Yep, this seems real. Quickly scrolling up and down the search page results for "video" by grabbing the scroll bar triggers it quite reliably, with no changes to the extension code.

Fixed with

  newVideoElement.style.setProperty('padding-bottom', '1px');
  setTimeout(() => newVideoElement.style.removeProperty('padding-bottom'), 0);

but the visual thrashing is annoying.

Also fixed with

  const cell = videoElement.closest(keyToCss('cell'));
  setTimeout(() => cell && cell.isConnected && inject('/main_world/trigger_reflow.js', [], cell), 0);
// trigger_reflow.js
export default function triggerReflow () {
  const cell = this;
  const reactKey = Object.keys(cell).find(key => key.startsWith('__reactFiber'));
  let fiber = cell[reactKey];

  while (fiber !== null) {
    if (fiber.stateNode?.nodeRef === cell && fiber.stateNode?.onResize) {
      fiber.stateNode.onResize([{ target: cell }]);
      return;
    }
    fiber = fiber.return;
  }
}

but that's quite a hacky way to poke the redpop internals. If onResize took no arguments and used its own nodeRef property internally, I would be more okay with this.