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.

Sign up for GitHub

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

Jump to bottom

Firefox: fix wrong text highlighting with double-click #4908

Closed
wants to merge 2 commits into from
Closed

Firefox: fix wrong text highlighting with double-click #4908

wants to merge 2 commits into from

Conversation

Jawell
Copy link
Contributor

@Jawell Jawell commented Mar 23, 2022

Description
Firefox had an another response of Document.getSelection() method. After double-clicking on a word with some mark, you can't unmark this word.

Issue
Fixes: #3336 #3374

Example

Old behavior ![Untitled](https://user-images.githubusercontent.com/21054523/159647954-fdcdcbcf-1e00-4df1-9d71-d7c516e7c397.gif)
New behavior ![Untitled](https://user-images.githubusercontent.com/21054523/159650729-fe733364-7e0d-40c4-9093-41c4853391f1.gif)

Context
I also changed toSlatePoint interface. I don't see any use and reason of this method with old generics. I tried to make a cypress test, but I can't do it. Cypress doesn't support highlighting text by double-clicking. Maybe this is not true. But I see that cypress moves the text cursor to the end of the word that was double-clicked. And I can say that the test (cypress/integration/examples/select.ts) is not correct. Because cursor pointer is seated at the end of a paragraph after triple-click

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 23, 2022

🦋 Changeset detected

Latest commit: 246db12

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 Major

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

@Jawell
Copy link
Contributor Author

Jawell commented Mar 24, 2022 •
edited
Loading

I see that the problem of integration test in:

  it('hovering toolbar appears', () => {
    cy.get('div')
      .eq(12)
      .should('not.exist')

I checked git history. That .eq(12) was .eq(13). But I don't understand why it was used this indexes

@dylans
Copy link
Collaborator

dylans commented Mar 24, 2022

I see that the problem of integration test in:

  it('hovering toolbar appears', () => {
    cy.get('div')
      .eq(12)
      .should('not.exist')

I checked git history. That .eq(12) was .eq(13). But I don't understand why it was used this indexes

It's just timing out in our CI environment. We need to improve our integration tests, but I don't think it's related to your change.

@Jawell
Copy link
Contributor Author

Jawell commented Mar 26, 2022

@dylans
Hi, so, what about this PR? :)

@dylans
Copy link
Collaborator

dylans commented Mar 29, 2022

@dylans Hi, so, what about this PR? :)

I've been traveling so I'm a bit behind on my reviews. I will get to it soon.

@e1himself e1himself mentioned this pull request Mar 31, 2022
Open
@Jawell
Copy link
Contributor Author

Jawell commented Apr 8, 2022

Hey, can we merge this?

BitPhinix
BitPhinix suggested changes Apr 25, 2022
View reviewed changes
Copy link
Contributor

@BitPhinix BitPhinix left a comment

Choose a reason for hiding this comment

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

Just 2 small suggestions, otherwise LGTM. I'm a bit worried that it will potentially break stuff in unexpected places, but in the worst case, we can revert this

packages/slate-react/src/plugin/react-editor.ts
Comment on lines 473 to +477
options: {
exactMatch: T
suppressThrow: T
exactMatch: boolean
suppressThrow: boolean
}
): T extends true ? Point | null : Point {
): Point | null {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a huge fan of the signature change, it makes sense that it returns a non-nullable value when not using suppressThrow (although the current definition is wrong as exactMatch should be boolean and not T since it doesn't influence the 'nullability' of the return, as far as I can see). The same is the case for toSlateRange.

packages/slate-react/src/plugin/react-editor.ts
Comment on lines +687 to +701
if (IS_FIREFOX && !isCollapsed && anchorNode !== focusNode) {
const isEnd = Editor.isEnd(editor, anchor, anchor.path)
const isStart = Editor.isStart(editor, focus, focus.path)

if (isEnd) {
const after = Editor.after(editor, anchor as Point)
// Editor.after() might return undefined
anchor = after || anchor
}

if (isStart) {
const before = Editor.before(editor, focus as Point)
focus = before || focus
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should replace this with a Editor.hangRange helper

dylans
dylans requested changes May 7, 2022
View reviewed changes
Copy link
Collaborator

@dylans dylans left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please see the feedback within the PR.

This was referenced Jan 28, 2023
Closed
Merged
@dylans
Copy link
Collaborator

dylans commented Dec 16, 2023

Superseded by #5580

@dylans dylans closed this Dec 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dylans dylans dylans requested changes

@BitPhinix BitPhinix BitPhinix requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Firefox: wrong selection with double click on a text node
3 participants
@Jawell @dylans @BitPhinix