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

fix(caret): trailing spaces handling #2741

Merged
merged 16 commits into from
Jun 28, 2024
Merged

fix(caret): trailing spaces handling #2741

merged 16 commits into from
Jun 28, 2024

Conversation

neSpecc
Copy link
Member

@neSpecc neSpecc commented Jun 15, 2024

Problem

If block starts with several spaces, they can't be deleted. Caret jumps to the previous block.

Resolves #2429
Resolves #1382
Resolves #1179
Resolves #2521

Cause

There are two whitespace handling in HTML

  1. Invisible spaces — all regular trailing spaces, tabs, etc
  2. Visible spaces —  

Our Caret.isAtStart method covers only case №1 and treats all whitespaces like "empty".

Solution

I've rewrote isAtStart() and isAtEnd() methods.

  1. They were moved from the Caret Module to the "caret.ts" util.
  2. Their dependency from this.BlockManager has been removed. Now methods accept input as argument. (so we'll be able to expose them to the api or some package)
  3. Their implementation has been simplified. Now we create a range from the block start (or end in case of isAtEndOfInput()) to the caret position, clone its content to the temporary div and check its textContent
  4. Now we handle collapsed and visible whitespaced correctly. Util isCollapsedWhitespaces() is used for it.
  5. Tests for Backspace/Delete/Left/Right when caret at the start/end added
  6. Tests for arrow-navigation through the Delimiter added

@neSpecc neSpecc marked this pull request as ready for review June 19, 2024 18:41
Copy link
Member

@gohabereg gohabereg left a comment

Choose a reason for hiding this comment

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

Looks good overall, just a minor comment

@@ -34,5 +34,8 @@
"unknown": true,
"requestAnimationFrame": true,
"navigator": true
},
"rules": {
"jsdoc/require-returns-type": "off"
Copy link
Member

Choose a reason for hiding this comment

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

👍

}

const { nextInput, currentInput } = currentBlock;
const isAtEnd = currentInput !== undefined ? caretUtils.isAtEndOfInput(currentInput) : undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Similar expression is used several times, maybe worth to extract it to a function. Or allow caret utils to accept undefined

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe that utility methods like isAtStartOfInput() should not handle case when input is undefined . Module should explicitly handle the cases when block has no inputs or there is no currentBlock at all since it is a part of business logic.

src/components/utils/caret.ts Outdated Show resolved Hide resolved
src/components/utils/caret.ts Outdated Show resolved Hide resolved
@neSpecc neSpecc merged commit afa99a4 into next Jun 28, 2024
6 checks passed
@neSpecc neSpecc deleted the fix-caret-start-end branch June 28, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants