-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
base: main
Are you sure you want to change the base?
Fix void node clipboard event handling in Firefox #4775
Conversation
9971a42
to
02898ed
Compare
|
02898ed
to
e96a833
Compare
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. |
There was a problem hiding this 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.
ec47e36
to
1712d1f
Compare
@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! |
1712d1f
to
eb3d35f
Compare
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)?
eb3d35f
to
82e9b9e
Compare
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.
82e9b9e
to
a7a2a30
Compare
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? |
@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 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) |
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. |
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. |
@dylans - sorry I forgot to save it! Now it is there with the latest 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? |
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 |
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 |
In this very simple contenteditable example I also can't get Works in react 16: https://codesandbox.io/s/contenteditable-cut-react-16-hnbsd So seems like a regression in react 17 I made an issue in the react repo: facebook/react#23094 |
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. |
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 |
I'd suggest that we fix per @bryanph's most recent note and keep an eye on the React issue. |
Yes, this seems like the way to go! Thanks @bryanph! |
@bryanph Now that we require React 18, perhaps this can be resolved? |
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
yarn test
.yarn lint
. (Fix errors withyarn fix
.)yarn start
.)yarn changeset add
.)