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

Tighten mark addition logic against zero-length marks. #3005

Conversation

themithy
Copy link
Contributor

@themithy themithy commented Sep 6, 2019

Hi,

When adding marks to multi-line range it might happen that a mark is added to zero-length text node. Although such situation is corrected then by normalization, extra operations are generated unnecessarily.

This fix tightens the logic.

Thanks.

@themithy themithy force-pushed the michal/tighten-zero-length-marks branch from f3806a7 to 1309536 Compare September 6, 2019 13:42
@themithy themithy force-pushed the michal/tighten-zero-length-marks branch from 1309536 to f3444af Compare September 6, 2019 13:48
if (key === end.key) length = end.offset
if (key === start.key && key === end.key)
length = end.offset - start.offset

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this change significant for the fix or is this just trying to tighten up logic?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is slightly different now specifically in the case where (key === start.key && key !== end.key)

Before:
length = node.text.length
Now:
length = node.text.length - start.offset

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure this is needed, if you move the offset you need to truncate the length accordingly - in both cases when key === and !== end.key

@ianstormtaylor
Copy link
Owner

Hey @themithy this looks good to me. Can you add a test to ensure this logic? Might be hard with normalization, not sure.

@ianstormtaylor
Copy link
Owner

I believe that this may be fixed by #3093, which has changed a lot of the logic in Slate and slate-react especially. I'm going to close this out, but as always, feel free to open a new issue if it persists for you. Thanks for understanding.

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

Successfully merging this pull request may close these issues.

3 participants