-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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: [#21810] Allow default pasting behaviour in FontSizePicker #21812
fix: [#21810] Allow default pasting behaviour in FontSizePicker #21812
Conversation
@@ -11,6 +11,10 @@ import { compose } from '@wordpress/compose'; | |||
*/ | |||
import { getPasteEventData } from '../../utils/get-paste-event-data'; | |||
|
|||
function isNativePasteEvent( event ) { | |||
return event.type === 'paste' && event.target.tagName === 'INPUT'; |
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.
Is this the right check here, won't we have the same issues for contenteditables, textareas....
I feel the intent here is more:
- trigger the native paste behavior if the target is inside the editor canvas
I believe the main issue here is that there's React event bubbling happening instead of native DOM event bubbling triggering the onPaste handler when it shouldn't.
One option to solve this is to avoid the React handlers or to check the event.target
inside the handlers.
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.
I was thinking it was more of an isolated case hence treating the symptoms rather than the cause. Will look into your suggestions.
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.
Updated the PR.
aa78533
to
073594c
Compare
__experimentalCanUserUseUnfilteredHTML: canUserUseUnfilteredHTML, | ||
} = getSettings(); | ||
|
||
const handler = ( event ) => { |
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.
There's one small subtle difference that potentially can have performance impact. Since this is an inline function, potentially, this could rerender children often. I think it's good to start like you did (without any memoization) but we just need to check that the component is not being rerendered on each "store change".
For instance you can add a "console log" inside the component and type on a paragraph and see if the logging is repeated or not as you type.
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.
While typing console.log is not executed. When copying/pasting there are two statements in console.
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.
That's fine :) Thanks for the tests
073594c
to
76d8efc
Compare
Description
Fixes: #21810
Default paste behavior is not allowed for input fields.
Types of changes
Bug fix, non-breaking change.
Handling the case when you want to paste in an input field.
Checklist: