Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Misalignment of DecoratorNode due to Insertion of ElementNode or Additional DecoratorNode #54

Open
JaminZhou opened this issue Dec 18, 2023 · 2 comments

Comments

@JaminZhou
Copy link

I've noticed an issue regarding the layout misplacement of DecoratorNode when an ElementNode or another DecoratorNode is inserted before it. Upon debugging, I found out that the misalignment is caused by the system not marking the problematic DecoratorNode as 'dirty', which results in the layout not being recalibrated or updated. As such, I propose a fix that would include the marking of the DecoratorNode as 'dirty' to trigger a layout update whenever a new node is inserted. This should correct the alignment issue and ensure the layout is correctly adjusted for the new node.

Simulator.Screen.Recording.-.iPhone.15.-.2023-12-18.at.14.53.28.mp4
Simulator.Screen.Recording.-.iPhone.15.-.2023-12-18.at.14.55.56.mp4
@amyworrall
Copy link
Contributor

I propose a fix that would include the marking of the DecoratorNode as 'dirty' to trigger a layout update whenever a new node is inserted.

That sounds a good idea. Are you thinking we should mark all subsequent decorator nodes in the document as dirty? I'm wondering if there might be a perf issue if there are loads of them.

If it's just a positioning thing and we don't need to update decorator node content, it might be possible to rely on TextKit's own concept of dirty re-rendering, which should (assuming our code works correctly!) handle re-positioning the decorator nodes. I wonder if there's a bug in our code that integrates with that? If we can nail that, we might be able to get away with not marking all the decorators as dirty inside Lexical. But I agree that correctness is required, so if this solution cannot solve the issue, then marking the decorators as dirty may be the only way to go.

@JaminZhou
Copy link
Author

I agree that directly marking subsequent DecoratorNodes as 'dirty' could indeed affect performance. While the dirty marking solution is relatively direct when not making significant changes to the code, it's not the most elegant approach.

The Decorator's position isn't being updated because in the 'positionAllDecorators' method, the 'decoratorPositionCache' attribute of 'textStorage' isn't updating the CharacterLocation in real time.

This results in the method attribute = textStorage.attribute(.attachment, at: characterIndex, effectiveRange: nil) as? TextAttachment can not fetch the corresponding attachment, hence the DecoratorNode position remains as it was, without any update.

I would like to explore better ways to update the 'decoratorPositionCache'.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants