Fix Cannot find DOM for readonly void elements

[heyitsaamir]

Description
Modified the logic for the editor to gracefully handle selections inside void elements of readonly editors.
Changed the isTargetInsideNonReadonlyVoid to be isTargetInsideReadonlyVoid because double negatives are confusing :)

PENDING: Tests based on feedback from @dylans.

Issue
Fixes: #4886

Example
Before:

Screen.Recording.2022-03-14.at.8.35.01.AM.mov

After:

Screen.Recording.2022-03-14.at.8.35.27.AM.mov

Context
The logic previously was:

const anchorNodeSelectable =
          hasEditableTarget(editor, anchorNode) ||
          isTargetInsideNonReadonlyVoid(editor, anchorNode)

The node selection wouldn't go inside the second if-statement because the first clause would return true (hasEditableTarget checks if this node target is in the same editor, which is true). The second clause also needs to be checked as well to see if the target is inside a readonly-void element. If it is, then we should avoid selection.

I'm not sure if this is the right long term approach. Void elements should be selectable too, but the behavior should probably be up to the element-renderer.

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

[changeset-bot]

🦋 Changeset detected

Latest commit: ad63bba

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]

Makes sense. We’re just starting to add tests for slate-react so it would be great if you can add a test (I won’t block for that, just want to start building up a decent test suite). Thanks.

[heyitsaamir]

Makes sense. We’re just starting to add tests for slate-react so it would be great if you can add a test (I won’t block for that, just want to start building up a decent test suite). Thanks.

Yup will do. I noticed that we use react-test-renderer to add unit tests. I've found using react-testing-library to be way more intuitive and simpler to build tests. Obviously not saying we need to add that in for this, but would be happy to add that in in a separate PR if you think it's a good idea.

EDIT: Tried to do this, and I found it hard to atleast create a reasonable unit test that involves clicking in the void elements. I can do it with cypress, but I don't know if we want to modify examples and make the e2e tests more sophisticated (or they should just be simple tests as they are). I think the react-testing-library should help here. LMK what you think.

@dylans

[dylans]

Makes sense. We’re just starting to add tests for slate-react so it would be great if you can add a test (I won’t block for that, just want to start building up a decent test suite). Thanks.

Yup will do. I noticed that we use react-test-renderer to add unit tests. I've found using react-testing-library to be way more intuitive and simpler to build tests. Obviously not saying we need to add that in for this, but would be happy to add that in in a separate PR if you think it's a good idea.

EDIT: Tried to do this, and I found it hard to atleast create a reasonable unit test that involves clicking in the void elements. I can do it with cypress, but I don't know if we want to modify examples and make the e2e tests more sophisticated (or they should just be simple tests as they are). I think the react-testing-library should help here. LMK what you think.

@dylans

Let me think about it a bit and ask a few of the other maintainers for their thoughts. There is https://github.com/mwood23/slate-test-utils which leverages react-testing-library, but there has been reasonable reluctance to stray too far from the approach used in slate core.

[heyitsaamir]

Makes sense. We’re just starting to add tests for slate-react so it would be great if you can add a test (I won’t block for that, just want to start building up a decent test suite). Thanks.

Yup will do. I noticed that we use react-test-renderer to add unit tests. I've found using react-testing-library to be way more intuitive and simpler to build tests. Obviously not saying we need to add that in for this, but would be happy to add that in in a separate PR if you think it's a good idea.
EDIT: Tried to do this, and I found it hard to atleast create a reasonable unit test that involves clicking in the void elements. I can do it with cypress, but I don't know if we want to modify examples and make the e2e tests more sophisticated (or they should just be simple tests as they are). I think the react-testing-library should help here. LMK what you think.
@dylans

Let me think about it a bit and ask a few of the other maintainers for their thoughts. There is https://github.com/mwood23/slate-test-utils which leverages react-testing-library, but there has been reasonable reluctance to stray too far from the approach used in slate core.

Makes sense. It's definitely non-trivial to test the behavior right now given the existing framework in the repo.

[ahoisl]

Just to mention: The logic with void elements changed in this PR was already there before my changes for readonly editors in #4727. So I guessed back then that some selection for void elements was indeed intended and decided to just avoided selection when the editor is readonly.

In other words: I think, your PR changes behavior not only for readonly editors, but also for void elements in editable editors. Is this intended?

Doing some git blaming, the initial code comes from PR #3649, fixing issue #3648. For testing, I removed the isTargetInsideNonReadonlyVoid locally and selection across void elements seems to work just fine - and also my use cases keep on working without errors. So it might be that the selection issue was fixed by something else and the code involving void elements can be removed altogether.

That said, even with a good set of automated tests I would be hesitating to remove some code I have no clue about, even worse without tests... 🤞

[heyitsaamir]

Thank you for looking at this with a keen eye!

Just to mention: The logic with void elements changed in this PR was already there before my changes for readonly editors in #4727. So I guessed back then that some selection for void elements was indeed intended and decided to just avoided selection when the editor is readonly.

That's true and it makes sense that you should be able to select elements when you're editing them.

In other words: I think, your PR changes behavior not only for readonly editors, but also for void elements in editable editors. Is this intended?

Hm, I don't think so. If the editor is in readonly mode, that clause will return false and that clause should be ignored, right?

hasEditableTarget(editor, anchorNode) &&
          !isTargetInsideReadonlyVoid(editor, anchorNode)

If isTargetInsideReadonlyVoid is false (which it should be for all non-readonly editors), then it should have no effect in the above statement, right?

Fwiw I also played around with the images and mentions examples and they seem to be working normally. Lmk if you think otherwise!

Doing some git blaming, the initial code comes from PR #3649, fixing issue #3648. For testing, I removed the isTargetInsideNonReadonlyVoid locally and selection across void elements seems to work just fine - and also my use cases keep on working without errors. So it might be that the selection issue was fixed by something else and the code involving void elements can be removed altogether.

Hm, I don't see that locally. This is what I see without the isTargetInsideReadonlyVoid clause. (This is on safari, but same behavior on FF)

Screen.Recording.2022-03-14.at.9.38.07.PM.mov

[ahoisl]

In other words: I think, your PR changes behavior not only for readonly editors, but also for void elements in editable editors. Is this intended?

Hm, I don't think so. If the editor is in readonly mode, that clause will return false and that clause should be ignored, right?

hasEditableTarget(editor, anchorNode) &&
          !isTargetInsideReadonlyVoid(editor, anchorNode)

If isTargetInsideReadonlyVoid is false (which it should be for all non-readonly editors), then it should have no effect in the above statement, right?

Assume that there is a void element where hasEditableTarget returns false. If the editor is readonly, then your change does not make a difference. However, if the editor is editable (not readonly), then before your changes the result of isTargetInsideNonReadonlyVoid would have been true, consequently the result of the boolean operation (=anchorNodeSelectable and focusNodeSelectable) would have been true and the if clause would have been executed. With your changes, the if clause is not executed any more.

I'm just saying that your changes cause a change in behavior also for non-readonly/editable editors, not just for readonly ones and I'm unsure if this is intended.

From your issue description, I thought that your changes should only affect readonly editors.

I guess that your void element has an editable target, causing the first part of the boolean operation to always be true. If you really want your changes to only affect readonly editors, then I think some change like the following (pseudo-code) would be needed, where isTargetInsideVoid would not check readonly any more:
const anchorNodeSelectable = readonly ? hasEditableTarget && !isTargetInsideVoid : hasEditableTarget || isTargetInsideVoid

Hm, I don't see that locally. This is what I see without the isTargetInsideReadonlyVoid clause. (This is on safari, but same behavior on FF)

I only tried out my use case with image void elements, so your use case may still fail and that's a good reason to keep as much use cases unchanged as possible, I think - we don't know all the use cases the existing code is necessary for 😄

[heyitsaamir]

Assume that there is a void element where hasEditableTarget returns false. If the editor is readonly, then your change does not make a difference. However, if the editor is editable (not readonly), then before your changes the result of isTargetInsideNonReadonlyVoid would have been true, consequently the result of the boolean operation (=anchorNodeSelectable and focusNodeSelectable) would have been true and the if clause would have been executed. With your changes, the if clause is not executed any more.

Ah that's true, but I guess it's unexpected for hasEditableTarget to be false when the click target is inside the editor, right? It's very possible that I'm misunderstanding the code :). But you're right, it does change the behavior in that scenario.

guess that your void element has an editable target, causing the first part of the boolean operation to always be true. If you really want your changes to only affect readonly editors, then I think some change like the following (pseudo-code) would be needed, where isTargetInsideVoid would not check readonly any more:
const anchorNodeSelectable = readonly ? hasEditableTarget && !isTargetInsideVoid : hasEditableTarget || isTargetInsideVoid

I didn't do anything special to make my void element an editable target. Check out this sandbox for what the element looks like.

It is curious though why the images element doesn't crash. 🤔 Here's the image example that doesn't crash

EDIT: Aha, there's a contentEditable={false} in the div that contains the image. So the first clause returns false. I'm not sure if that's correct though. Thoughts?

[ahoisl]

EDIT: Aha, there's a contentEditable={false} in the div that contains the image. So the first clause returns false. I'm not sure if that's correct though. Thoughts?

Seems to be the correct way with contentEditable={false} according to the comment from cmmartin in #4886. If that fixes your error, I would be reluctant to integrate this MR, because of the change for non-readonly editors - but it's not my decision 😄

[heyitsaamir]

Closing this because setting contentEditable to false prevents this issue:
#4886 (comment)