Fix Copy/pasting void elements is not working #5121

laufeyrut · 2022-09-13T12:55:23Z

Description
This PR makes it possible to copy/cut void elements.

Issue
Fixes: #4808 and #4806

Example
Before

After:

Context
Created a new function hasSelectableTarget that checks if hasEditableTarget and isTargetInsideNonReadonlyVoid.
Using this function instead of hasEditableTarget so we are not excluding nonReadonlyVoids fx onCopy and onCut and onPaste

I did not add Cypress tests for the new logic since Cypress does not support copy/paste and the only solution I found was to use some package like https://www.npmjs.com/package/clipboardy. If we want to include the package or if there is a better solution for it I'm happy to add the tests.

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.)

…ableTarget. Fixes Copy/pasting void elements is not working ianstormtaylor#4808

changeset-bot · 2022-09-13T12:55:28Z

🦋 Changeset detected

Latest commit: 03234e5

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

This PR includes changesets to release 1 package

Name	Type
slate-react	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

dylans

Looks fine to me... will plan to include in a release this week unless others point out something I'm missing. Thanks!

dylans

Potential issue is the use case of custom copy/paste within a void element. This change probably breaks current approaches to that. Will think through this more.

…test for editing editable void

laufeyrut · 2022-09-20T11:41:04Z

@dylans
After reading your comment I did some more testing and found one bug in my approach regarding editing the editable voids, I fixed that and added a cypress test for it.

But I see the issue with copy-pasting inside the editable voids and I'm not sure how it is best to solve that 😞 Do you have any ideas?

SmilinBrian · 2022-09-20T17:03:02Z

Looking forward to this one as it will solve some problems for our Slate use. We have a number of different void nodes that users expect to be able to Copy and Paste while the void is showing as 'selected'.

I'm not sure I understand what the 'blocking' issue is… is it that you might have an <input type=text> inside the void?

dylans · 2022-09-20T21:21:57Z

Looking forward to this one as it will solve some problems for our Slate use. We have a number of different void nodes that users expect to be able to Copy and Paste while the void is showing as 'selected'.

I'm not sure I understand what the 'blocking' issue is… is it that you might have an <input type=text> inside the void?

Yes, for example in Plate, we have a void that contains an Excalidraw drawing inside it. I need time to think through how to make this not break that scenario.

SmilinBrian · 2022-09-20T23:04:28Z

I'm not sure I understand what the 'blocking' issue is… is it that you might have an <input type=text> inside the void?

Yes, for example in Plate, we have a void that contains an Excalidraw drawing inside it. I need time to think through how to make this not break that scenario.

I see. I wonder if it would make sense for withReact(editor) to add hasSelectableTarget() and/or hasEditableTarget() to the editor, and that way customizers / plug-ins could override them to fine-tune the decision about whether or not "Slate" should try to deal with event X?

laufeyrut · 2022-09-21T16:30:38Z

packages/slate-react/src/components/editable.tsx

+export const hasSelectableTarget = (
+  editor: ReactEditor,
+  target: EventTarget | null
+): boolean => {
+  return (
+    hasEditableTarget(editor, target) ||
+    isTargetInsideNonReadonlyVoid(editor, target)
+  )
+}
+


@dylans
This works but the linter is not happy and I feel like I'm breaking some priciples.
Using Editor.isInline and Editor.isBlock does not work

Suggested change

export const hasSelectableTarget = (

editor: ReactEditor,

target: EventTarget | null

): boolean => {

return (

hasEditableTarget(editor, target) ||

isTargetInsideNonReadonlyVoid(editor, target)

)

}

export const hasSelectableTarget = (

editor: ReactEditor,

target: EventTarget | null

): boolean => {

const node =

hasTarget(editor, target) && ReactEditor.toSlateNode(editor, target)

return (

hasEditableTarget(editor, target) ||

editor.isInline(node) ||

editor.isBlock(node)

)

}

evasteingrims · 2022-10-06T09:54:19Z

Looking forward to this one as it will solve some problems for our Slate use. We have a number of different void nodes that users expect to be able to Copy and Paste while the void is showing as 'selected'.
I'm not sure I understand what the 'blocking' issue is… is it that you might have an <input type=text> inside the void?

Yes, for example in Plate, we have a void that contains an Excalidraw drawing inside it. I need time to think through how to make this not break that scenario.

Hi @dylans. Have you had some time to think about this?

dylans · 2022-10-07T17:58:08Z

Looking forward to this one as it will solve some problems for our Slate use. We have a number of different void nodes that users expect to be able to Copy and Paste while the void is showing as 'selected'.
I'm not sure I understand what the 'blocking' issue is… is it that you might have an <input type=text> inside the void?

Yes, for example in Plate, we have a void that contains an Excalidraw drawing inside it. I need time to think through how to make this not break that scenario.

Hi @dylans. Have you had some time to think about this?

I have but I don't have a good answer yet. Need a few more days to try out other examples and see the potential consequences of this.

zbeyens · 2022-10-07T18:23:11Z

I see. I wonder if it would make sense for withReact(editor) to add hasSelectableTarget() and/or hasEditableTarget() to the editor, and that way customizers / plug-ins could override them to fine-tune the decision about whether or not "Slate" should try to deal with event X?

Definitely this. In Plate I would have added a plugin field that handles copy/paste onKeyDown events so this behavior should be controllable.

The question here is, should we copy the void node by default? I'd say yes as it's the most common case, just note that would be a breaking change.

dylans · 2022-10-07T18:47:48Z

I see. I wonder if it would make sense for withReact(editor) to add hasSelectableTarget() and/or hasEditableTarget() to the editor, and that way customizers / plug-ins could override them to fine-tune the decision about whether or not "Slate" should try to deal with event X?

Definitely this. In Plate I would have added a plugin field that handles copy/paste onKeyDown events so this behavior should be controllable.

The question here is, should we copy the void node by default? I'd say yes as it's the most common case, just note that would be a breaking change.

This is one way I was roughly thinking could work. What do you think @laufeyrut?

laufeyrut · 2022-10-10T12:51:14Z

I see. I wonder if it would make sense for withReact(editor) to add hasSelectableTarget() and/or hasEditableTarget() to the editor, and that way customizers / plug-ins could override them to fine-tune the decision about whether or not "Slate" should try to deal with event X?

Definitely this. In Plate I would have added a plugin field that handles copy/paste onKeyDown events so this behavior should be controllable.
The question here is, should we copy the void node by default? I'd say yes as it's the most common case, just note that would be a breaking change.

This is one way I was roughly thinking could work. What do you think @laufeyrut?

Yes, I think this could work! I don't know of a better idea. @dylans
I'm not sure how that would be implemented though

I see. I wonder if it would make sense for withReact(editor) to add hasSelectableTarget() and/or hasEditableTarget() to the editor, and that way customizers / plug-ins could override them to fine-tune the decision about whether or not "Slate" should try to deal with event X?

Definitely this. In Plate I would have added a plugin field that handles copy/paste onKeyDown events so this behavior should be controllable.

The question here is, should we copy the void node by default? I'd say yes as it's the most common case, just note that would be a breaking change.

I think being able to copy the void nodes should be the default behavior 🙏

alex-vladut · 2022-11-07T17:17:55Z

Hey @laufeyrut great job on adding these fixes in place 👍
I looked at extracting those methods to be easily overridable from withReact and you can find the changes on this branch https://github.com/ianstormtaylor/slate/compare/main...alex-vladut:slate:fix-copy-editable-voids?expand=1. What I did was mainly to move the methods into the ReactEditor and reference them from there. Let me know if you think it makes sense and maybe you can incorporate those changes into this PR to get some feedback

laufeyrut · 2022-11-15T00:06:06Z

Hey @laufeyrut great job on adding these fixes in place 👍 I looked at extracting those methods to be easily overridable from withReact and you can find the changes on this branch https://github.com/ianstormtaylor/slate/compare/main...alex-vladut:slate:fix-copy-editable-voids?expand=1. What I did was mainly to move the methods into the ReactEditor and reference them from there. Let me know if you think it makes sense and maybe you can incorporate those changes into this PR to get some feedback

I really like this change, Thank you for the help @alex-vladut 🙏

@dylans I look forward to hearing what you think about this

Create new function hasSelectableTarget and use it instead of hasEdit…

31de43f

…ableTarget. Fixes Copy/pasting void elements is not working ianstormtaylor#4808

Add changeset

fa11f69

laufeyrut changed the title ~~Copy/pasting void elements is not working~~ Fix Copy/pasting void elements is not working Sep 14, 2022

dylans approved these changes Sep 19, 2022

View reviewed changes

dylans reviewed Sep 19, 2022

View reviewed changes

Revert a change that made editable void not editable and add cypress …

7993315

…test for editing editable void

laufeyrut commented Sep 21, 2022

View reviewed changes

Extract methoods into easily overridable with help from @alex-vladut

03234e5

dylans merged commit 06942c6 into ianstormtaylor:main Nov 17, 2022

github-actions bot mentioned this pull request Nov 17, 2022

Version Packages #5198

Merged

dylans mentioned this pull request Feb 21, 2023

Can't paste into void elements #5294

Closed

dpizzo-at-aops mentioned this pull request Mar 16, 2023

Cannot Copy Text Inside an Input in a Void Element #5366

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Copy/pasting void elements is not working #5121

Fix Copy/pasting void elements is not working #5121

laufeyrut commented Sep 13, 2022 •

edited

Loading

changeset-bot bot commented Sep 13, 2022 •

edited

Loading

dylans left a comment

dylans left a comment

laufeyrut commented Sep 20, 2022 •

edited

Loading

SmilinBrian commented Sep 20, 2022

dylans commented Sep 20, 2022

SmilinBrian commented Sep 20, 2022

laufeyrut Sep 21, 2022 •

edited

Loading

evasteingrims commented Oct 6, 2022 •

edited

Loading

dylans commented Oct 7, 2022

zbeyens commented Oct 7, 2022 •

edited

Loading

dylans commented Oct 7, 2022

laufeyrut commented Oct 10, 2022 •

edited

Loading

alex-vladut commented Nov 7, 2022

laufeyrut commented Nov 15, 2022 •

edited

Loading

Fix Copy/pasting void elements is not working #5121

Fix Copy/pasting void elements is not working #5121

Conversation

laufeyrut commented Sep 13, 2022 • edited Loading

changeset-bot bot commented Sep 13, 2022 • edited Loading

🦋 Changeset detected

dylans left a comment

Choose a reason for hiding this comment

dylans left a comment

Choose a reason for hiding this comment

laufeyrut commented Sep 20, 2022 • edited Loading

SmilinBrian commented Sep 20, 2022

dylans commented Sep 20, 2022

SmilinBrian commented Sep 20, 2022

laufeyrut Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

evasteingrims commented Oct 6, 2022 • edited Loading

dylans commented Oct 7, 2022

zbeyens commented Oct 7, 2022 • edited Loading

dylans commented Oct 7, 2022

laufeyrut commented Oct 10, 2022 • edited Loading

alex-vladut commented Nov 7, 2022

laufeyrut commented Nov 15, 2022 • edited Loading

laufeyrut commented Sep 13, 2022 •

edited

Loading

changeset-bot bot commented Sep 13, 2022 •

edited

Loading

laufeyrut commented Sep 20, 2022 •

edited

Loading

laufeyrut Sep 21, 2022 •

edited

Loading

evasteingrims commented Oct 6, 2022 •

edited

Loading

zbeyens commented Oct 7, 2022 •

edited

Loading

laufeyrut commented Oct 10, 2022 •

edited

Loading

laufeyrut commented Nov 15, 2022 •

edited

Loading