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 void node clipboard event handling in Firefox #4775

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

skogsmaskin
Copy link
Collaborator

@skogsmaskin skogsmaskin commented Jan 7, 2022

Description
Firefox will not trig native clipboard events when selecting single void nodes in slate-react.

This will make sure that the DOM range is set to 0-1 (not 1-1) offset so something is selected if this is the case.

This obviously works fine in other browsers, but Firefox seems to need a real range here.

Issue
Fixes #4513

Context
This obviously works, but I'm not 100% certain there will be no side effects. Please help me think.

Also, what is strange it that it's somehow working on the Images example without this fix (and after). But I never get it to work in my code, like others have struggled with too as seen by: #4513

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

Repository owner deleted a comment from changeset-bot bot Jan 7, 2022
@skogsmaskin skogsmaskin changed the title Fix broken single-void-selections in Firefox Fix broken clipboard event handling in Firefox Jan 7, 2022
@skogsmaskin skogsmaskin changed the title Fix broken clipboard event handling in Firefox Fix void node clipboard event handling in Firefox Jan 7, 2022
@changeset-bot
Copy link

changeset-bot bot commented Jan 7, 2022

⚠️ No Changeset found

Latest commit: a7a2a30

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@skogsmaskin
Copy link
Collaborator Author

skogsmaskin commented Jan 7, 2022

Found one side-effect. Arrowing up and down between void blocks now takes two key-presses.

I think it's because we don't handle up/down events in a consistent way, but leaves that up to the default browser behaviour.

Firefox seems to handle this a bit differently than the other browsers.

I've made a new commit that handles these events over void blocks consistently.

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.

Looks great, thanks for fixing this! If you have time please add a changeset. I’ll leave it open for a couple of days for feedback and then land it unless there are objections.

@skogsmaskin
Copy link
Collaborator Author

skogsmaskin commented Jan 7, 2022

@dylans - Updated the first commit to be even more agressive, checking for focus on a void block all together. Now it's working with selection spans as well (where it ends in a void node).

Will add changeset. Thanks for reviewing!

COMPAT: Firefox will not trig clipboard events when selections ends in void nodes.
Make sure that the range has 0-1 (not 1-1) offset so something is selected.
This obviously works fine in other browsers. Related to the zero-value (\uFEFF)?
@bryanph
Copy link
Contributor

bryanph commented Jan 9, 2022

I think it would be a good idea to first come up with a reproducible example that consistently demonstrates this issue. I haven't been able to yet (my failed attempts in #4513 (comment)) and since the void image example works fine we might be looking at the wrong source for this? I don't think it's general behavior in firefox that a collapsed selection does not trigger an oncopy event, I've been able to confirm that much in the codesandbox at least.

Handle up down key navigation over void nodes with this specific behaviour.
In this way we have a consistent way of handling these events, and it will not be up to the default browser behaviour to move the cursor.

Firefox seems to handle this a bit differently than the other browsers.
After doing e96a833 you had to press up/down twice to move the focus up/down between void blocks.
@skogsmaskin
Copy link
Collaborator Author

skogsmaskin commented Jan 10, 2022

Extend selection up/down should also be handled in a consistent way, added that.

@bryanph - please note that the missing clipboard events in FF and this fix is only fixing the issue when the focus is at a void node (where it's consistently failing to fire those events). Might what you are looking at in the sandbox be another issue?
I agree we should put up a sandbox that shows the issue. Will try to do so.

@bryanph
Copy link
Contributor

bryanph commented Jan 10, 2022

@skogsmaskin ah yeah that might be it, I didn't try to isolate to just void node selections. Although that leaves the question of why it does work in the image example. Perhaps some user-select: none setting?

Edit: after trying this again in my editor I can confirm that the onCopy works inconsistently and onCut does not work in my case (both work in chrome)

@skogsmaskin
Copy link
Collaborator Author

skogsmaskin commented Jan 11, 2022

I've made this sandbox that illustrates the problem:

https://codesandbox.io/s/slate-react-17-oncopy-bug-not-working-forked-9n9x9?file=/index.js

I see there are some issues still which the PR doesn't solve when testing against the above code example. I think this needs to be investigated further.

@dylans
Copy link
Collaborator

dylans commented Jan 11, 2022

I've made this sandbox that illustrates the problem:

https://codesandbox.io/s/slate-react-17-oncopy-bug-not-working-forked-0uyde?file=/index.js

I see there are some issues still which the PR doesn't solve when testing against the above code example. I think this needs to be investigated further.

https://codesandbox.io/s/slate-react-17-oncopy-bug-not-working-forked-qg78q?file=/index.js has a couple small fixes to your sandbox.

I'll leave this open for now given your comment on needing to investigate this further.

@skogsmaskin
Copy link
Collaborator Author

skogsmaskin commented Jan 11, 2022

@dylans - sorry I forgot to save it! Now it is there with the latest slate-react

Right one is: https://codesandbox.io/s/slate-react-17-oncopy-bug-not-working-forked-9n9x9?file=/index.js

Could you re-apply any changes you did please?

@skogsmaskin
Copy link
Collaborator Author

skogsmaskin commented Jan 11, 2022

In the sandbox: if one make a shift-arrow-right when a void (only) is selected it will fire onCut events. The selection in slate ramains the same

@bryanph
Copy link
Contributor

bryanph commented Jan 11, 2022

Updated your example a bit to remove some errors https://codesandbox.io/s/slate-react-17-oncopy-bug-not-working-forked-mow9x?file=/index.js.

That seems like a decent reproduction, I'll try and see if the same happens in plain contenteditable

@bryanph
Copy link
Contributor

bryanph commented Jan 11, 2022

In this very simple contenteditable example I also can't get onCut to trigger

Works in react 16: https://codesandbox.io/s/contenteditable-cut-react-16-hnbsd
Not working in react 17: https://codesandbox.io/s/contenteditable-cut-react-17-8qgtt?file=/src/App.js:310-387

So seems like a regression in react 17

I made an issue in the react repo: facebook/react#23094

@skogsmaskin skogsmaskin marked this pull request as draft January 21, 2022 09:23
@skogsmaskin
Copy link
Collaborator Author

Put this back to draft. Not sure if we should try to work around this or wait for React to fix the bug. So far there's not response in the issue that @bryanph filed with them.

@bryanph
Copy link
Contributor

bryanph commented Jan 21, 2022

I can imagine it will take a while to fix on the react side, also who knows it might be some firefox quirk that is hard to resolve. So overall I'd be a proponent of a workaround. Would be nice to get a response from the react team on what the underlying cause might be though, can maybe bump the thread once in a while.

Thinking about the issue a little more I just realized that in react 17 the event handlers are no longer attached to the document root. So I tried to see if it worked if you attach an event to the document root and voila it seems to work, see https://codesandbox.io/s/contenteditable-cut-react-18-working-drqnl?file=/src/App.tsx:179-188

So I guess all we have to do is attach a cut listener to document that doesn't do anything, in that case the onCut event is also fired on the Editable div

@dylans
Copy link
Collaborator

dylans commented Jan 22, 2022

I'd suggest that we fix per @bryanph's most recent note and keep an eye on the React issue.

@skogsmaskin
Copy link
Collaborator Author

Yes, this seems like the way to go! Thanks @bryanph!

@dylans
Copy link
Collaborator

dylans commented Dec 16, 2023

@bryanph Now that we require React 18, perhaps this can be resolved?

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.

onCopy not firing for a collapsed selection using React 17 in firefox
3 participants