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 firefox disconnected selection api usage #5486

Merged

Conversation

WcaleNieWolny
Copy link
Contributor

@WcaleNieWolny WcaleNieWolny commented Jul 25, 2023

Description
This PR fixes the invalid usage of the selection API in firefox. Firefox allows disconnected ranges that cause problems with slate. I originally started to work on this to fix . this issue

Issue
Fixes: (I do not think this fixes any slate issue, however I am not sure. I did not checked)

Example
Before my changes:

2023-07-25.17-10-26.mp4

After my changes:

2023-07-25.17-11-04.mp4

Context
1:
anchorNode, anchorOffset, focusOffset. Do not behave the same in chrome and in firefox. In firefox if there is more then 1 range said values would be invalid. I fixed this by using the getRangeAt function (see toSlateRange changes).
2:
I reversed this pr as it was interfering with my changes (I believe this PR relies on broke selection api in firefox)
3:
I removed the usage of setDomSelection. This is because setBaseAndExtent removed all ranges leaving only 1 range (this removed the ability to select more cells)
4:
I removed the el.focus() because it relied on the setDomSelection
5:
The slight refactoring was at some point done by eslint

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.) (I will soon, this will be the next commit)

@changeset-bot
Copy link

changeset-bot bot commented Jul 25, 2023

🦋 Changeset detected

Latest commit: 6a4b512

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 Minor

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.

Generally looks fine, we'll land and see if it causes any major regressions.

Could you please fix the minor comment typo (colapsed -> collapsed) and somewhere add a link to an explanation of the issue with Firefox so someone in the future can find the relevant background issue?

@WcaleNieWolny
Copy link
Contributor Author

Generally looks fine, we'll land and see if it causes any major regressions.

and somewhere add a link to an explanation of the issue with Firefox so someone in the future can find the relevant background issue?

Is a link to this PR and the explanation I gave in the initial message okay?

@dylans
Copy link
Collaborator

dylans commented Jul 26, 2023

Generally looks fine, we'll land and see if it causes any major regressions.
and somewhere add a link to an explanation of the issue with Firefox so someone in the future can find the relevant background issue?

Is a link to this PR and the explanation I gave in the initial message okay?

yes that's fine

@WcaleNieWolny
Copy link
Contributor Author

So is this ready to be merged?

@dylans
Copy link
Collaborator

dylans commented Jul 26, 2023

So is this ready to be merged?

Our integration tests failed, so I'm running them again to see if it was a service issue or if this has introduced a regression.

@WcaleNieWolny
Copy link
Contributor Author

Our integration tests failed, so I'm running them again to see if it was a service issue or if this has introduced a regression.

It did introduced a regression. Just submitted a fix for that

@dylans dylans merged commit 8b548fb into ianstormtaylor:main Jul 26, 2023
11 checks passed
@github-actions github-actions bot mentioned this pull request Jul 26, 2023
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.

2 participants