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

onCopy not firing for a collapsed selection using React 17 in firefox #4513

Open
bryanph opened this issue Sep 13, 2021 · 7 comments · May be fixed by #4775
Open

onCopy not firing for a collapsed selection using React 17 in firefox #4513

bryanph opened this issue Sep 13, 2021 · 7 comments · May be fixed by #4775

Comments

@bryanph
Copy link
Contributor

bryanph commented Sep 13, 2021

Description
See title and codesandbox, been trying to debug this in a react example without luck so it does seem a bug in slate although it only happens when using react 17 and only in firefox

Sandbox
https://codesandbox.io/s/slate-react-17-oncopy-bug-t54vc?file=/index.js
(make sure to open the example in a new window in firefox

Steps

  1. Go to above sandbox in firefox
  2. Open the example in a new window
  3. Click somewhere in the editor
  4. Ctrl-c 😄
  5. You don't see the console.log
  6. Do the same thing in chrome and notice the console.log does fire

Expectation
onCopy is triggered

Environment

  • Slate Version: current master
  • Browser: Firefox
@bryanph bryanph changed the title onCopy not firing for a collapsed selection using React 17 onCopy not firing for a collapsed selection using React 17 in firefox Sep 13, 2021
@dylans
Copy link
Collaborator

dylans commented Sep 13, 2021

The code for onCopy is pretty terse:

onCopy={useCallback(
(event: React.ClipboardEvent<HTMLDivElement>) => {
if (
hasEditableTarget(editor, event.target) &&
!isEventHandled(event, attributes.onCopy)
) {
event.preventDefault()
ReactEditor.setFragmentData(editor, event.clipboardData)
}
},
[attributes.onCopy]
)}

My best guess is that the following is true in FF but also in other browsers:

if (
  hasEditableTarget(editor, event.target) &&
  !isEventHandled(event, attributes.onCopy)
) {

which then prevents the default event, and then calls setFragmentData, but if the selection is empty, setFragment data returns:

const { selection } = e
if (!selection) {
return
}

@bryanph
Copy link
Contributor Author

bryanph commented Sep 13, 2021

@dylans thanks for looking into it 😄. If I add an event listener breakpoint for the copy event firefox it doesn't get triggered as well though so it seems to me like it doesn't get triggered at all. I've been trying to reproduce this independently from slate without success though (just react 17 and some content editable code) so I might be missing something here.

As for the setFragment there, the selection isn't null in the case I was trying to describe above but rather collapsed, so it shouldn't halt there (I just noticed I didn't really mention that clearly in the steps). The case for which this causes problems is when the cursor is on a void node as then the void node is not copied.

@skogsmaskin
Copy link
Collaborator

skogsmaskin commented Jan 7, 2022

I'm experiencing the same. What is interesting is that it works in the images example (https://www.slatejs.org/examples/images), but not when I try to do the same in my code.

If you only select a void block in Firefox, onCopy and onCut and onCopy events doesn't trig at all, as @bryanph says.

However if I press shift+arrowLeft and keep the single void selected, it will trig the events afterwards (though the editor slate selection object is still the same).

I suspect it is related to there not being a real DOM-range set and Firefox thinks nothing is selected and this will not fire the events.

I see we have some special Firefox code hacks here and there (el.focus()), could it be related? Maybe some special treatment for FF is missing?

@skogsmaskin skogsmaskin linked a pull request Jan 7, 2022 that will close this issue
5 tasks
@skogsmaskin
Copy link
Collaborator

I'm drafting a PR with a potential fix for this: #4775

@bryanph
Copy link
Contributor Author

bryanph commented Jan 7, 2022

@skogsmaskin Do you use react 17.x in your code? The examples in slate are run using 16.x I think. I don't quite remember 100% but I thought this only occurred in react 17 for me. If that's the case then there might be some extra code in react 17 that prevents the oncopy event from being triggered or something. In any case thanks for looking into this 😄

@bryanph
Copy link
Contributor Author

bryanph commented Jan 7, 2022

Also just debugging this is very inconsistent, I have no idea what's going on for example here I have the exact same codesandboxes and it works in one and not the other? 😕

Working https://codesandbox.io/s/slate-react-17-oncopy-bug-working-twndi?file=/index.js
Not working: https://codesandbox.io/s/slate-react-17-oncopy-bug-not-working-ye09p

(and trying a little later both work again 🤯)

Perhaps it's some kind of async problem?

@bryanph
Copy link
Contributor Author

bryanph commented Jan 19, 2022

From #4775 (comment)

It's a react bug, see facebook/react#23094

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

Successfully merging a pull request may close this issue.

3 participants