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

update insertText logic when selection is not collapsed #4804

Merged
merged 3 commits into from
Jan 28, 2022
Merged

update insertText logic when selection is not collapsed #4804

merged 3 commits into from
Jan 28, 2022

Conversation

suilang
Copy link
Contributor

@suilang suilang commented Jan 26, 2022

Description
When I select a paragraph with different styles of text, for example, bold and plain, and then insert the letter q. What I want is to insert a non-bold letter.

Example
this is old code, the q is bold
chrome-capture

this is new code, the q is normal
chrome-capture1

Context

chrome-capture 2

Normally, when we insert the character q between unbold and bold, it is unbold. the style use position of anchor. but when selection is not collapsed, selection be set by focus or end of range.

The processing logic is inconsistent for different actions. also, the cursor should be placed at the starting position. If the deleted area has elements that cannot be deleted, like table, the cursor position will be incorrect

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]

@changeset-bot
Copy link

changeset-bot bot commented Jan 26, 2022

🦋 Changeset detected

Latest commit: 0552d9e

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

This PR includes changesets to release 1 package
Name Type
slate 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

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 @suilang . I'm not sure if it matters here, but have you checked selecting in reverse to make sure we don't break due to the direction of the selection (some parts of the codebase are good about reversing the selection and some are not).

@suilang
Copy link
Contributor Author

suilang commented Jan 28, 2022

@dylans Thank reply. You mean selecting text from back to front? In there i use Range.start to get start point, it make sure that start is always brefore end point。

@dylans dylans merged commit 03861af into ianstormtaylor:main Jan 28, 2022
@github-actions github-actions bot mentioned this pull request Jan 28, 2022
@imbolo
Copy link

imbolo commented Feb 14, 2022

@suilang @dylans
After this change,it throws an error when I select two lines and input any character.

select-two-line-and-input.mov

@TheSpyder
Copy link
Collaborator

The error is because with a selection like that the range start is inside the first paragraph. The paragraph is deleted and so the pointref is too. This doesn't happen when the pointref is based on end because by definition the end cursor position is just collapsed to the start of the range through deletion.

This PR gave me a bad feeling but I didn't have time to confirm whether it was a mistake. Thank you @imbolo for confirming it is. @dylans we need to revert.

I don't know if there's a nice way to fix OP's problem. Maybe a smarter way to pick whether to use the start or end ref? Or perhaps just store two point refs for start and end if start becomes null use the end.

@cmditch
Copy link
Contributor

cmditch commented Feb 23, 2022

Also experiencing errors with this in production. We've reverted back to 0.72.3.

@dylans
Copy link
Collaborator

dylans commented Mar 2, 2022

Reverted as noted above. Hopefully we can find a better solution for the issue identified in this original PR without breaking other behavior.

@suilang
Copy link
Contributor Author

suilang commented Mar 15, 2022

@TheSpyder @dylans I'm sorry it happened. like you say, the paragraph is deleted.

I still think the cursor should be placed at the starting position. As the picture above shows, The text style should be the same as before

I have fix this error, by store two point refs for start and end . like this,

截屏2022-03-15 下午5 25 07
and add test case describe the scene as I see it

截屏2022-03-15 下午5 31 22

I am not sure whether I should submit the PR again. I hope to get your opinion

@dylans
Copy link
Collaborator

dylans commented Mar 15, 2022

@suilang it's ok, rich text editing is not easy. Feel free to raise a new PR with tests and we'll evaluate it.

DougReeder pushed a commit to DougReeder/slate that referenced this pull request Apr 3, 2022
…or#4804)

* update insertText logic when selection is not collapsed

* add changeset

* fix bug when end of range is void

Co-authored-by: zhangpengcheng15 <zhangpengcheng15@jd.com>
DougReeder pushed a commit to DougReeder/slate that referenced this pull request Apr 3, 2022
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.

5 participants