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

Add support for read-only and non-selectable elements #5374

Merged
merged 3 commits into from
Apr 3, 2023

Conversation

12joan
Copy link
Contributor

@12joan 12joan commented Mar 20, 2023

Description

  • Add isElementReadOnly (default false, open to suggestions for alternative names) to editor. A read-only element behaves much like a void with regard to selection and deletion, but renders its children the same as any other non-void node.
  • Add isSelectable to editor (default true). A non-selectable element is skipped over when navigating using arrow keys.
    • Add ignoreNonSelectable to Editor.nodes, Editor.positions, Editor.after and Editor.before (default false)
    • Transforms.move ignores non-selectable elements

Issue
Fixes #5372

Example
https://user-images.githubusercontent.com/4272090/226684680-6c685c4d-85e1-4e45-aa52-413cf6c4892c.mov

Context
The goal of this PR is to support contentEditable={false} on inline nodes without the problems described in #5372. While this is already possible by making the node void, doing so changes the way in which the node is rendered, which is a problem for toggling between editable and non-editable states.

Checks

  • The new code matches the existing patterns and styles.
  • The tests pass with yarn test.
  • The linter passes with yarn lint. (Fix errors with yarn fix.)
  • The relevant examples still work. (Run examples with yarn start.)
  • You've added a changeset if changing functionality. (Add one with yarn changeset add.)

fix delete while selected

fix type while selected

fix failing test

add tests

add e2e test

linter fixes

add changeset
@changeset-bot
Copy link

changeset-bot bot commented Mar 20, 2023

🦋 Changeset detected

Latest commit: b842575

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
slate Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@12joan 12joan force-pushed the fix/non-contenteditable-inlines branch from 95ce385 to 2b1499c Compare March 20, 2023 17:43
@12joan 12joan marked this pull request as draft March 20, 2023 18:00
@12joan 12joan force-pushed the fix/non-contenteditable-inlines branch from 3f77a7e to f8270f5 Compare March 21, 2023 16:49
@12joan 12joan changed the title Add support for read-only non-void elements Add support for read-only and non-selectable elements Mar 21, 2023
@12joan 12joan force-pushed the fix/non-contenteditable-inlines branch from f8270f5 to 0fb0763 Compare March 21, 2023 16:54
@12joan 12joan marked this pull request as ready for review March 21, 2023 16:57
@12joan 12joan force-pushed the fix/non-contenteditable-inlines branch from 0fb0763 to b842575 Compare March 21, 2023 16:59
@dylans dylans merged commit b52e08b into ianstormtaylor:main Apr 3, 2023
@github-actions github-actions bot mentioned this pull request Apr 3, 2023
@hernansartorio
Copy link
Contributor

After trying to use isSelectable to prevent selecting hidden elements and not being able to I came here and realized that the naming is a bit misleading, it should have probably been called something like isInlineElementNavigatableWithArrowKeys.

@12joan
Copy link
Contributor Author

12joan commented Oct 5, 2023

@hernansartorio What you describe is a feature we really need in Slate, but which wasn't my main focus when implementing this. Would you be interested in working on a PR to extend isSelectable to apply to all types of element?

@dylans Any thoughts on this?

@hernansartorio
Copy link
Contributor

Got it! Just wanted to leave a note for anyone running into the same. I'd be open to if only I could figure out an approach that makes sense.

@hernansartorio
Copy link
Contributor

I’ll revisit this in the future as it’s not a critical feature for me yet, but after experimenting a bit I found that, for starters, we'd need to set user-select: none and contentEditable={false} to any non-selectable items, which combined with the current isSelectable flag produces some reasonable results when selecting elements with keyboard and mouse.

Then it would be a matter of figuring out what should happen when editing contiguous elements (i.e. should hitting backspace in the next element ignore the non-selectable one instead of merging into it?). And probably other edge cases I haven't thought about.

@12joan
Copy link
Contributor Author

12joan commented Oct 6, 2023

There are definitely a lot of edge cases to consider, and different use cases will likely want different behavious in those edge cases.

When you revisit this, you could start by handling each edge case in whatever way makes most sense at the time. We can add options to configure the behaviour further when the need arises.

@beorn
Copy link
Contributor

beorn commented Oct 12, 2023

Some thoughts as this is a problem I often have:

Ideally we should document best practices on how to render non-editable content from (1) SlateJS inline/element nodes, and (2) not from SlateJS nodes. It would be good to get advise on how to do that without breaking SlateJS:

  • navigation - cursor navigation should probably always work, skipping over the elements. If needed I guess a specific [data-slate-cursor-boundary] could be added to stop cursor navigation if that's ever required.
  • deletion - should elements be deletable? I guess if they're readOnly then they shouldn't be, and so any range-deletion across them should fail?
  • selection - should selection ranges be allowed to span this content or not?

Interactions between:

  • renderElement:
    • user-select: none
    • contentEditable={false}
  • editor plugin
    • isSelectable()
    • isReadOnly()

Not sure, but is some of this complexity due to the way SlateJS tries to stay in sync with the DOM selection — could we perhaps rely more on the browser and CSS's user-select: none instead of requiring isSelectable()?

@12joan
Copy link
Contributor Author

12joan commented Oct 27, 2023

@beorn Sorry for the late reply. You're right that further research and documentation is required to understand how non-editable content (and, by extension, invisible content) can be handled at the consumer level.

The user-select property doesn't do much in the context of a native contenteditable. It doesn't prevent the user from selecting and deleting a range containing the element; the only effect is to prevent selections within the element itself. There's also a styling inconsistency between Chrome and Firefox as to whether the selection highlight is visible, although in both browsers the element is included in the selection.

In a native contenteditable, setting a child to contenteditable="false" causes the cursor to skip over it. Slate doesn't behave this way for void elements because such elements are required to contain an editable children, which makes the void element a valid point for the cursor to rest.

I can't recall what happens if a void element fails to render its children prop, but if Slate could be modified to support this case, this could be a valid way of making an entire element non-selectable. Of course, this property couldn't be discerned from Slate's data model (which has no knowledge of HTML), and this could present problems when searching for a selectable point in the document, for example.

recording.mp4
<div contenteditable>
  Before
  <span contenteditable="false" style="user-select: none">[contenteditable="false" user-select: none]</span>
  After
</div>

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.

Support contentEditable={false} in (inline) elements
5 participants