facebook / lexical-ios

Lexical iOS is an extensible text editor framework that integrates the APIs and philosophies from Lexical Web with a Swift API built on top of TextKit.
MIT License
549 stars 41 forks source link

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

Open JaminZhou opened 11 months ago

JaminZhou commented 11 months ago

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.

https://github.com/facebook/lexical-ios/assets/9011374/ee1cd6e9-8428-4076-857d-8a842749ab43

https://github.com/facebook/lexical-ios/assets/9011374/b83e70dd-b974-4c73-b123-81df0998a187

amyworrall commented 11 months ago

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 commented 11 months ago

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