Apply hideCaret to LayoutTree when indent changes

[ethan-james]

Fixes #4

Animating transform doesn't update the caret position correctly in iOS Safari. Hiding the caret during the animation will prevent this visual bug.

[ethan-james]

@raineorshine This isn't quite working because usePrevious is maybe not quite the right tool for the job. I wanted to get it in front of you in case you have thoughts on the overall approach.

[raineorshine]

I like the idea of using the css transition to control the visibility and not messing with js timers. But I haven't really used this technique before, so I'm unsure exactly what will trigger it at the right time.

Another option is something like AnimationEnd event, though I haven't used that either.

[ethan-james]

onAnimationEnd looks like it will do what I want, but for some reason it fires immediately when the animation is happening in reverse (indent level is decreasing). The current state of things (useState to control animation flag) is the only method that I have been able to make work, with a minimum of caret flicker.

[ethan-james]

@raineorshine Is it possible/OK to write custom CSS classes? I know we're trying to avoid doing that, but I think that adding a class like indent-0.9 or indent-1.8 and then targeting those with a CSS animation ([class*=indent-]) might help the animation figure out when it should run again.

[raineorshine]

There should be a PandaCSS way to accomplish that. The App.css has been fully deprecated at this point.

That doesn't means that css can't be applied... it just needs to be applied through the typesafe way of Panda recipes.

[ethan-james]

Here is a CSS animation-based solution that uses 2 identical animations to trick the element into replaying the animation whenever the indent level changes.

[raineorshine]

Thanks! Very clever and elegant.

Let's apply this only on iOS Safari since that is where the problem occurs.

isTouch && isSafari()

The only problem with the even/odd distinction is that it doesn't work when tapping two thoughts down.

trim.720E70DC-C232-4A37-8908-6BB638841551.MOV

[ethan-james]

Thanks! Very clever and elegant.

Let's apply this only on iOS Safari since that is where the problem occurs.

isTouch && isSafari()

The only problem with the even/odd distinction is that it doesn't work when tapping two thoughts down.

trim.720E70DC-C232-4A37-8908-6BB638841551.MOV

Hmm, yeah, that is a problem with the entire named animation approach. Is there a modulo that you think will cover the entire visible area? Maybe 10 or 16 different named animations? I would love to think of something a little less hacky, but I haven't been able to do so.

The other option is to try to remove the animation after it completes, and then add it back when the indent changes again. Challenges include:

1 getting onAnimationEnd to work properly
2. removing/re-applying the animation when they interrupt an animation (ArrowDown then ArrowUp) which would probably result in at least 1 frame of caret flicker

[ethan-james]

It looks like it will show a maximum of 12 one-line thoughts at a time? Maybe that depends on screen height?

[raineorshine]

The number of thoughts on the screen will depend on screen height and font size. Practically speaking, trees don't usually exceed a depth of 12–15. That said, if we're creating a named animation for each depth anyway, then there's not much downside to defining enough to cover all reasonable cases. I think 16 would be fine.

I agree that removing + re-adding the animation is likely to lead to other troubles.

[ethan-james]

@raineorshine This works, but is pretty unpleasant to look at. Any attempt that I made to package this code or make it dynamic (including an inlined IIFE) broke Panda's static analysis. I'll try to poke at it some more later to see if I can get it to work better. If you have a preferred way to abstract logic like this while keeping static analysis, please share.

[raineorshine]

Yes, good point. I'm thinking either a recipe with variants would work (possibly programmatically generated), or switching to a style attribute and accessing the animation through token.

[raineorshine]

@yangchristina Could I get your input on this? Would recipe variants be able to select on the indentation level at runtime?

We have to trick the browser into thinking the animation has changed at each indentation level, so that's why each animation has its own name.

[yangchristina]

@raineorshine Yes, recipes should be fine to use, just make sure you add staticCss: ['*']. You can also use cva since this is only used in one place, and you won't need staticCss: ['*'] for cva (it'll automatically generate all variants)

[raineorshine]

@yangchristina Great, thanks for confirming!

@ethan-james Would you mind looking into cva as a way to clean this up? Reach out to me personally if this looks like a lot of trouble, as I want to be respectful of your time.

[ethan-james]

@yangchristina @raineorshine Thanks for your guidance! The latest commit represents how I think I want it to work, but it still doesn't seem to be generating the classes for the recipe variant. If you think there's a specific piece that I can tweak to make it work, please let me know.

[ethan-james]

Now it's no longer the most recent commit. Better to just look at the whole diff.

[yangchristina]

hideCaretAnimationNames must be in the same file as cva. hideCaret might also have to be directly in LayoutTree.tsx instead of imported.
I tried with both hideCaret and hideCaretAnimationNames in LayoutTree.tsx and it did generate the classes.

[yangchristina]

You can check whether the classes have been generated with npx panda debug src/components/LayoutTree.tsx

[ethan-james]

@yangchristina Thanks! Keeping them in a separate file seems to work, with the only issue being that Panda's config can't pull the constants from getHideCaretAnimationName.ts. It says ERROR: Could not resolve "../../styled-system/css" so I guess there are different stages of the toolchain or something like that. Duplicating the constants in getCaret.config.ts & getHideCaretAnimationName.ts seems like a relatively small price to pay.

[raineorshine]

This is looking better, but I'm not quite satisfied. While the duplication is small in terms of lines of code, it reflects a lack of understanding of the tools. I have a strong feeling that there is a better way. There must be something we are missing.

The fact that both hideCaret.config.ts and getHideCaretAnimationNames both export the same value adds to the confusion.

If you feel that you have made all reasonable attempts and are not sure how to simplify it further, just let me know and I will take over.

[ethan-james]

@raineorshine I tried to use defineRecipe to add it to the main config in the way that the other recipes are defined, but it didn't generate the variants from the list that way either.

The only thing I can think of that might fit Panda's ethos more closely would be to do away with the abstracted list and simply define all of the keyframes by key in panda.config.ts, then all of the variants by key in the recipe in getHideCaretAnimationName.ts or in a new file, ./src/recipes/hideCaret.ts. It would still be two lists but they wouldn't be duplicated across two custom files anymore.

[yangchristina]

@raineorshine have you tested this on safari? From panda debug it doesn't look like the classes are being generated.
Also, instead of isTouch && isSafari() it may be better to use _mobile: { _safari: ... }

[raineorshine]

Sadly, I'm running into some difficulties testing changes, because either vite or the browser is caching the styles. This throws into question my whole refactor. I reverted my changes and am happy to move forward with your implementation.

@ethan-james The only problem is that it the solution does not work after rebasing on main. I tried multiple times and every time it stops working, even when the merge conflict is trivial. Could you look into this? I'm sorry this has gone on this long. I promise to merge it as soon as we can figure out why it has stopped working in main (i.e. after #2581 was merged).

[raineorshine]

Of course, the duration token! Thanks :)

[ethan-james]

@raineorshine That's OK, hopefully I can figure out a cleaner fix if I have some time left over during future milestones. For now, I fixed {durations.layoutSlowShift}, which had changed from {durations.layoutSlowShiftDuration} and now it should be working.