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

toSlatePoint should not consider a selection within a void node if the void node isn't in the editor itself. #4885

Merged
merged 1 commit into from
Mar 20, 2022

Conversation

ryanmitts
Copy link
Contributor

@ryanmitts ryanmitts commented Mar 12, 2022

If you have a nested editor setup. For example, one editor has a void node that contains another editor. In this case, a resolution of a selection by the nested editor previously would consider that the selection is for a void node since an ancestor void node does exist. However, the selection is only a void node in the context of this editor if the ancestor void node is contained in the editor.

In the "Editable Voids" example on the docs site, you can see this bug when you clear the text of the nested editor, and type one character. The selection will jump back to the beginning.

Issue
Fixes: (link to issue)

Example

There is a GIF in the original ticket, the new behavior is what would be expected, after inserting one character, the selection is collapsed at the end of the character that was just inserted.

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
Copy link

changeset-bot bot commented Mar 12, 2022

🦋 Changeset detected

Latest commit: e4dfa15

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 Patch

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

@ryanmitts ryanmitts marked this pull request as ready for review March 12, 2022 08:53
Copy link
Collaborator

@dylans dylans left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @ryanmitts . We have just started to add tests to slate-react ( see https://github.com/ianstormtaylor/slate/blob/main/packages/slate-react/test/index.spec.tsx ). If possible it would be great to have a test added that fails before this PR and passes after the PR.

Also please add a changeset (there are instructions in the PR). I would go with patch for this change.

Thanks!

@ryanmitts
Copy link
Contributor Author

ryanmitts commented Mar 13, 2022

@dylans I added a changeset to the project. I tried adding an integration test, but it's my understanding that the react test renderer does not have any DOM implementation, therefore these changes can't be tested in the current test environment as the react<->dom interaction is not there.

I believe the same sort of problems regarding testing brought up in #4887 would apply here.

…sider that a node is only void if it's within the same editor that is resolving the Point.

If you have a nested editor setup. For example, one editor has a void node that contains another editor. In this case, a resolution of a selection by the nested editor previously would consider that the selection is for a void node since an ancestor void node does exist. However, this selection is only a void node in the context of this editor if the ancestor void node is contained in the editor.
@ryanmitts
Copy link
Contributor Author

I tried writing a Cypress integration test for this issue, and although I can get it to happen in the test setup, it only happens sometimes, so the test is pretty useless at testing this issue. This might just only be a issue that can be seen with real keyoard input.

it('should allow deleting then inserting correctly into the nested editor', () => {
    cy.window().then((win: Window) => {
      cy.get(
        'div[data-test-id="nested-editor-container"] > div[data-slate-editor="true"]'
      ).then(el => {
        const range = document.createRange()
        range.selectNodeContents(el.get(0))
        win.getSelection()?.removeAllRanges();
        win.getSelection()?.addRange(range)
        cy.focused().type('{backspace}')
        cy.focused().type('H').then(() => {
          // Sometimes win.getSelection() is correct, sometimes not
          debugger;
        })
      })
    })
  })

@dylans dylans merged commit 07669dc into ianstormtaylor:main Mar 20, 2022
@github-actions github-actions bot mentioned this pull request Mar 20, 2022
DougReeder pushed a commit to DougReeder/slate that referenced this pull request Apr 3, 2022
…sider that a node is only void if it's within the same editor that is resolving the Point. (ianstormtaylor#4885)

If you have a nested editor setup. For example, one editor has a void node that contains another editor. In this case, a resolution of a selection by the nested editor previously would consider that the selection is for a void node since an ancestor void node does exist. However, this selection is only a void node in the context of this editor if the ancestor void node is contained in the editor.
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.

Backwards text typing on empty, nested, editable void Slate Editor
2 participants