Fix missing frame when animating thought up/down

[zukilover]

This PR fixes the missing frame issue at #2498 , which occurs when the editable preventAutoscroll class is applied, causing the thought to render with zero opacity:

em/src/components/Editable.tsx

Lines 595 to 597 in 13f92af

	editableRecipe({
	preventAutoscroll: true,
	}),

The solution ensures that the preventAutoscroll class is omitted during the y transition of the tree node. To achieve this, I refactored the y animation into TreeNodeAnimation.tsx and introduced a new isAnimating flag. Since this flag needs to be passed down through multiple layers (5 levels from LayoutTree to Editable), I implemented a React context to manage the state more effectively.

[zukilover]

@raineorshine, could you please re-run the failed test? It seems like a pipeline issue to me.

[raineorshine]

Thanks!

Can you confirm that you tested preventAutoscroll after this change and it still successfully prevents autoscroll in mobile Safari? I want to make sure we don't introduce any regressions.

We should also do a performance benchmark before/after since this potentially introduces a lot of state changes during a critical rendering phase.

[zukilover]

I tested preventAutoscroll by simulating an up/down movement animation and then focusing on one of the subthoughts. You can view the screen recording here:

ios.mp4

I've confirmed that the flag resets once the transition is complete. You can check the relevant code here:

em/src/components/TreeNodeAnimation.tsx

Lines 33 to 38 in 54ff6d7

	const handleTransitionEnd = () => {
	// Mark as animation done after the transition ends
	requestAnimationFrame(() => {
	isAnimatingRef.current = false
	})
	}

Additionally, I’ve optimized the code by using useRef instead of useState to prevent unnecessary state updates:

em/src/components/TreeNodeAnimation.tsx

Line 24 in 54ff6d7

const isAnimatingRef = useRef(false)

Previously, using useState not only caused unnecessary state changes, but also led to the entire thought rendering with opacity: 0 (triggered by preventAutoScroll due to a re-render).

[raineorshine]

That sounds great. I'll let @trevinhofmann review and test first, but looking forward to trying it out!

[trevinhofmann]

Thank you, @zukilover! This appears to resolve the animation for thoughts moved down:

Screen.Recording.2025-01-15.at.5.48.49.AM.mov

I've also tested on mobile (iOS Safari), and the animations work there as well.

I just have one question regarding the snapshots, otherwise this looks good to me.

[raineorshine]

Thank you both for your work on this tricky issue.

I was finally able to reproduce the layout shift on main outside of the puppeteer tests. See a detailed explanation in #2787. While the instigating cause is not 100% clear, my best guess is that the changes in this PR change the animation timing enough to trigger the rendering discrepancy. It is likely that the paste helper injects the thoughts in a way that creates a different render behavior in the tests than when manually testing.

Given this, I will tentatively say that the layout issue is not a regression caused by this PR and can be safely ignored. Needless to say, this does not evoke confidence in the puppeteer tests. But I think that is out of scope for this issue, and can be addressed in the new issue I created.

---

Now, considering the current approach. I do have concerns with the complexity and potential performance impact of this change. It involves requestAnimationFrame, adding/removing event handlers on every thought, and cloneElement. There is a lot going on here to fix a missing frame.

All of this seems to center around the preventAutoscroll behavior, yet I don't see a lot of research into the history and purpose of the original functionality. The new solution is clearly thought out and well tested. Nevertheless, I think it would be worth some due diligence before introducing what I would consider to be a fairly heavy, high risk change.

Question: Is the preventAutoscroll style still needed?

Note that there are two different autoscroll scenarios to be aware of. I regret that they use the same name, as this ambiguity only confuses the matter:

• The preventAutoscroll editable recipe variant (i.e. animation: 0.01s 1 preventAutoscroll) was originally added in 24afba8 in December 2020. Based on this StackOverflow answer. While it does not link to an issue, the commit messages points out that it was specifically added to prevent autoscroll during the tutorial: "Tutorial: Prevent autoscroll to editable after new thought." The tutorial covers the top part of the screen and the user can interact with the editor below, so the purpose of this is to avoid scrolling the tutorial out of view when the user follows the instructions to create a new thought.
• The preventAutoscroll utility was originally added in 95bc8aa in 2023 to fix [iOS] Prevent autoscroll to focused editable #1628. This was a more challenging problem to solve and involved a JS solution, explained in this StackOverflow answer. Presumably this is not related to this issue (moveThoughtDown: Thought animates up but not down #2498), but I thought it was worth distinguishing in order to avoid confusion. (Though it does raise the question... If the animation trick works in the tutorial, why doesn't it work here?)

Next steps:

• Confirm that the preventAutoscroll style is still needed to prevent autoscroll when creating a new thought on mobile in the Tutorial. If no, remove it and problem solved.
• If yes, do some performance testing to evaluate the impact of the proposed change. e.g. Compare the expansion of a large number of thoughts on main vs in this PR with 4x CPU throttling.)
• Consider any performance optimizations that might make this change less impactful. Do we always have to cloneElement? Can we come up with a narrower condition to apply this? Open to other ideas.

[zukilover]

@raineorshine, here's my conclusion after comparing the behavior of the main branch with this PR:

I tested it on iOS Safari, and it’s clear that if the purpose of adding the hack-ish preventAutoscroll keyframe is to prevent auto-scrolling when focusing on the editable, it doesn’t achieve that effectively. As shown in the videos below, there isn’t much difference between the main branch and this PR. Specifically, when creating a thought, the editable is focused, but parts of the tutorial still go out of view.

The other notable difference is that, in the main branch, adding a subthought on mobile causes the virtual keyboard to cover the editable. However, in this PR, I can focus on the editable without it being obscured by the keyboard.

Main branch:

main.mp4

This PR:

pr.mp4

I’m wondering if we should move forward with this PR by removing the style entirely?

[raineorshine]

Thanks for testing it out. The videos look close to me, though it's a bit hard to compare frame by frame. It even appears that the preventAutoscroll utility (2) may be overriding the preventAutoscroll style (1). Could you try disabling the utility and seeing if the tutorial still autoscrolls? I just want to establish firmly that the style does not have any effect and is no longer needed. @trevinhofmann can also validate.

Thanks so much!

[trevinhofmann]

I did some quick testing on iOS Safari to compare main, this PR, and disabling each of the preventAutoScroll functionalities (it is indeed unfortunate that we have two with the same name).

main

aa-main.mp4

this pull request

aa-pr.mp4

this pull request, with preventAutoScroll style disabled

aa-style.mp4

this pull request, with preventAutoScroll utility disabled

aa-utility.mp4

It seems that disabling the preventAutoScroll utility causes it to scroll most of the tutorial out of view.

[raineorshine]

Thanks @trevinhofmann! I think we're good to remove the preventAutoscroll style then and call this solved.

While existing code is usually there for a reason, this is a good reminder that it doesn't hurt to test those base assumptions. It's a rare exception, nevertheless, and I'm glad we were cautious.

Thanks @zukilover for all your work.