[Web] Fix thread jumps

[gaearon]

Fixes #9095

This logic is luckily web-only.

To repro, you have to navigate to a reply and then navigate to another one (e.g. upwards).

I don't understand the logic quite enough to tell what's happening, but for some reason we were setting shouldHandleScroll.current = false a bit too early. This function runs an extra time in this scenario and we end up missing the last event when we actually should scroll.

I changed the condition to what seems to make it work: only stop attempting after one positive scroll. I don't know if this is a perfect heuristic but seems to work at least on Chrome. You'll probably want to test FF, Safari, and Mobile Safari.

Test Plan

I was doing stuff like this. Previously it would lose scroll position.

fix.mov

[gaearon]

Curiously the bug repro's on main but not in prod. Idk what's up with that.

[estrattonbailey]

Curiously the bug repro's on main but not in prod. Idk what's up with that.

Yeah wth, that's odd

[gaearon]

It's gotta be either dev/prod difference or a regression since last deploy. Would be good to confirm which is that, and why. I'm not entirely comfortable with the > 0 condition because if it never happens (for one or the other reason), the scroll will keep readjusting on each resize.

[estrattonbailey]

Must be something in #8931. RN-for-web doesn't seem to be the culprit. Lot of changes in RN itself of course.

It seems that after setDeferParents(true) runs, onContentSizeChange fires 2x now, whereas before it ran 1x. Basically, I think it's running both before and after the DOM has updated to give us a positive offset for the anchor, and we're disabling scroll handling after the first pass.

The intent here is that if shouldHandleScroll.current === true, then we want to ensure that the anchor offset is reset back to 0. So you nailed that.

I think your concern about the > 0 condition is a good instinct. If there are any parents of a post — including deleted tombstones and such — that offset value will be > 0. It will be < 0 for the root post, but that's handled by the isRoot check.

I could also theoretically happen if we're expecting parents to be prepended and after setDeferParents(true) runs, and the API returns nothing or nothing is prepended for some reason. Then yeah onContentSizeChange will not run and we won't reset shouldHandleScroll. I don't think this case would blow us up though — actually it mostly works if we never reset it, except for the resize case. Subsequent navigations or a content resize should recover the broken state.

I don't want to claim there aren't other edge cases, but I think this this one is unlikely to happen and recoverable in the case it does. I think your fix is good and we can roll with it for now 👍

[estrattonbailey]

Thanks for this! I think it's a solid solve, and it was good to revisit this logic. It's been a few months, nice refresher. We'll keep an eye on this in the near future.