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

marks on double click selection using firefox fix #5580

Merged
merged 6 commits into from
Dec 13, 2023

Conversation

tdaron
Copy link
Contributor

@tdaron tdaron commented Dec 9, 2023

Description

This PR contains a quick-fix of marks function using Firefox. This was already fixed, but the fix was removed inside of #5486. I put it back but this time only in marks code instead of selection handling one, so it should not affect anything else.

Issue
Fixes: #5574 #5560

Example
Before the fix:

Recording 2023-12-09 at 15 47 57

When fixed:
Recording 2023-12-09 at 15 46 07

Context

This is related to firefox weird selection range when double clicking.
Explanation from original fix:
* suppose we have this document:
*
* { type: 'paragraph',
* children: [
* { text: 'foo ' },
* { text: 'bar' },
* { text: ' baz' }
* ]
* }
*
* a double click on "bar" on chrome will create this range:
*
* anchor -> [0,1] offset 0
* focus -> [0,1] offset 3
*
* while on firefox will create this range:
*
* anchor -> [0,0] offset 4
* focus -> [0,2] offset 0

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

Copy link

changeset-bot bot commented Dec 9, 2023

🦋 Changeset detected

Latest commit: a58c546

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

@tdaron tdaron changed the title fixed: marks on double click selection using firefox marks on double click selection using firefox fix Dec 9, 2023
Copy link
Contributor

@12joan 12joan left a comment

Choose a reason for hiding this comment

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

Nice! Can you add a unit test for the case that affects Firefox, and add a changeset?

packages/slate/src/editor/marks.ts Outdated Show resolved Hide resolved
@tdaron
Copy link
Contributor Author

tdaron commented Dec 9, 2023

Nice! Can you add a unit test for the case that affects Firfox, and add a changeset?

I tried to add one, i hope i did it the right way :D

@12joan
Copy link
Contributor

12joan commented Dec 9, 2023

I think this change should be a patch rather than a minor, since it's a bug fix. Minor usually means non-breaking feature changes.

@12joan
Copy link
Contributor

12joan commented Dec 9, 2023

Thanks for making those changes. Did you want to add a unit test?

@tdaron
Copy link
Contributor Author

tdaron commented Dec 9, 2023

Thanks for making those changes. Did you want to add a unit test?

IDK how to make them the right way..

@12joan
Copy link
Contributor

12joan commented Dec 9, 2023

If you grant me commit access to your fork, I can add one later today. 🙂

@tdaron
Copy link
Contributor Author

tdaron commented Dec 9, 2023

If you grant me commit access to your fork, I can add one later today. 🙂

done !

@12joan
Copy link
Contributor

12joan commented Dec 9, 2023

Thanks. I've added the test.

@12joan
Copy link
Contributor

12joan commented Dec 9, 2023

The integrations test failures are unrelated to this PR and can be safely ignored

@bradcypert
Copy link

Wow, awesome! Thank you for working on a fix for my issue. I had really wanted to use Slate but this was a blocker for me, so I started exploring alternatives -- now I can go back to slate! :D

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.

Attempting to remove a mark after deselecting and reselecting the marked text does not work.
4 participants