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

Firefox: fix wrong text highlighting with double-click (revived PR) #5275

Merged
merged 4 commits into from
Jan 30, 2023

Conversation

12joan
Copy link
Contributor

@12joan 12joan commented Jan 29, 2023

Description
This is a revival of PR #4908, which itself is a revival of #3374, both of which are currently inactive.

This PR fixes the bug in Firefox where double-clicking a word creates a selection whose anchor is inside the preceeding text node. (This is similar to the concept of a "hanging range", but not an example of it as per #4852.)

Issue
Fixes #3336
Fixes #3840
Fixes #4634
Fixes #1987 (Plate)
Fixes #1897 (Plate)

Example
From #4908:

Example

Context
PR #4908 had two requested changes (left by @BitPhinix) which were never addressed.

  1. Relating to type signatures

I've fixed this and the resulting type errors. If anyone more experienced with TypeScript than myself knows a more elegant way of handling the casts on lines 806 and 811, please let me know.

  1. "Maybe we should replace this with a Editor.hangRange helper"

There's still no clear concensus on what Editor.unhangRange should and should not be responsible for. In the comments on #4703, @e1himself suggested that it should be modified to support text nodes and cases where the starting point of the range needs adjusting.

Adding a separate Editor.hangRange helper would probably confuse the terminology even further. (If to "unhang" a range is to move the end point inside some text node, then to "hang" it should probably mean to reverse that, which isn't the intended behaviour here.)

My suggestion would be to first add support for text nodes to Editor.unhangRange, perhaps conditional on a texts: boolean option, and second to add a side: 'start' | 'end' | 'both' option, defaulting to 'end' if compatibility is a concern, controlling whether the start or end of the range is adjusted.

This should certainly happen in a separate PR, however. For now, I'd suggest we leave the logic in toSlateRange as it is unless anyone can spot a flaw in it.

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 Jan 29, 2023

🦋 Changeset detected

Latest commit: a1a58a6

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

@dylans
Copy link
Collaborator

dylans commented Jan 30, 2023

Thanks @12joan . Agree on fixing the smaller issue here, and revisiting the API changes/flexibility separately. If you have interest let me know.

@dylans dylans merged commit 5bc69d8 into ianstormtaylor:main Jan 30, 2023
@dylans
Copy link
Collaborator

dylans commented Jan 30, 2023

GitHub actions is not cooperating tonight, will see if this is a momentary glitch or if something is wrong with our release script tomorrow.

    at /home/runner/work/_actions/changesets/action/v1/dist/index.js:192:1285
    at processTicksAndRejections (node:internal/process/task_queues:96:5) {
  status: 403,
  response: {```

@12joan
Copy link
Contributor Author

12joan commented Jan 30, 2023

https://github.com/ianstormtaylor/slate/actions/runs/4040806744/jobs/6946963489#step:6:68

Looks like a rate limit on the GitHub search API. Re-running the workflow should probably fix it?

@12joan 12joan deleted the firefox_selection branch January 30, 2023 09:34
@zbeyens
Copy link
Contributor

zbeyens commented Jan 30, 2023

Happens in plate too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment