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

Allow selection in readonly mode #2920

Merged
merged 1 commit into from
Aug 31, 2022
Merged

Allow selection in readonly mode #2920

merged 1 commit into from
Aug 31, 2022

Conversation

acywatson
Copy link
Contributor

I've come full circle on this, and now I think the best idea here is to allow programmatic updates of the editor state regardless of the value of editor._readOnly. In attempting to implement something in the core that would stop programmatic updates by default and allow overriding that for specific updates via an update option, the intent was to make it easy for people to convert their editors into static content without having to worry about adding separate conditionals in all the places where programmatic updates can be triggered.

Practically, there are a lot of challenges with implementing this in the core. Command listeners are implicitly wrapped in a single editor.update, which causes a lot of problems with trying to apply behavior-changing modifiers (like options or tags) to individual "updates". We can't decouple command listeners from updates because batching can make updates async, which makes it hard to get the return value out of the update closure. It also seems like tags are broken with batching in general, as batched changes will all have the same tag applied to them.

From a conceptual standpoint, it probably makes just as much sense to make this a concern of the UI and/or the Lexical consumer as it does to try to bake a semantic guarantee around updates into the core.

This PR just takes out the check that prevents us from tracking Selection in readOnly mode, which will allow things like Commenting to work in that case.

@vercel
Copy link

vercel bot commented Aug 31, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
lexical ✅ Ready (Inspect) Visit Preview Aug 31, 2022 at 3:57PM (UTC)
lexical-playground ✅ Ready (Inspect) Visit Preview Aug 31, 2022 at 3:57PM (UTC)

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 31, 2022
Copy link
Member

@zurfyx zurfyx left a comment

Choose a reason for hiding this comment

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

Stamp to unblock but I think @trueadm should have a word on this because he was touching these conditions just recently

@trueadm
Copy link
Collaborator

trueadm commented Aug 31, 2022

Do we need any changes in LexicalEvents too, around selectionchange? Also what about copy events, should we let them through still?

@acywatson
Copy link
Contributor Author

Do we need any changes in LexicalEvents too, around selectionchange?

I don't think so? Did you have something specific in mind?

Also what about copy events, should we let them through still?

We can, but it's not important for this to work. AFAICT. Currently, copying still works, it's just plain/text and HTML on the clipboard. If we want to put the native Lexical format on there, we can change how readOnly is handled in LexicalEvents later.

@trueadm
Copy link
Collaborator

trueadm commented Aug 31, 2022

FWIW this is definitely a breaking change – so we should include this in the 0.4 release.

@acywatson acywatson merged commit cbbed60 into main Aug 31, 2022
@acywatson acywatson deleted the read-only-selection branch August 31, 2022 22:40
@acywatson acywatson mentioned this pull request Sep 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants