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

Editor: Fix regression introduced in CopyHandler #12543

Merged
merged 2 commits into from Dec 5, 2018

Conversation

Projects
None yet
3 participants
@gziolo
Copy link
Member

gziolo commented Dec 3, 2018

Description

Fixes #12506.

Regression introduced in #11851.

Reported by @afercia:

  • go in any post
  • select some text in a block editable area, either with mouse or keyboard doesn't matter, and try to paste it in another block or in an external editor
  • nothing happens (if you have previous content in your clipboard, that will be pasted)
  • try to cut text: nothing happens
  • try also in the post title: same as above
  • try also with an embed, for example a vimeo embed: paste https://vimeo.com/22439234 then edit it e.g. remove a few numbers, copy, paste it: the previous full URL get pasted
  • try to cut the vimeo URL: nothing happens
  • try even to select some text in the editor UI, outside of any block, for example the paragraph block description:
screenshot 2018-12-01 at 17 49 27
  • copy and paste it: same as above
  • switch to Code Editor mode: everything works normally

Basically any basic Copy / Cut operation in the Visual Editor is currently broken.

Initially, I tried to fix it by returning the information whether copy handler override was triggered but it is not possible to return a value from the dispatch handler exposed by withDispatch. It seems like a design decision?

I ended up with moving event handling to the handlers created in withDispatch which removes some lines of code. I know that @aduth wanted to avoid that in #11851 as discussed in #11851 (comment). An alternative would be to move event handling to the callback. I'm happy to iterate if you think it'd be cleaner.

How has this been tested?

  • go to any post
  • select some text in a block editable area, either with mouse or keyboard doesn't matter, and try to paste it in another block or in an external editor
  • try to cut text
  • try also in the post title
  • try also with an embed, for example a vimeo embed: paste https://vimeo.com/22439234 then edit it e.g. remove a few numbers, copy, paste it: the previous full URL get pasted
  • try to cut the vimeo URL
  • try even to select some text in the editor UI, outside of any block, for example the paragraph block description:

screenshot 2018-12-01 at 17 49 27

  • copy and paste it
  • switch to Code Editor mode

All of that should basically work.

@gziolo gziolo added this to the 4.7 milestone Dec 3, 2018

@gziolo gziolo self-assigned this Dec 3, 2018

@gziolo gziolo requested review from afercia , aduth and WordPress/gutenberg-core Dec 3, 2018

@gziolo gziolo changed the title Edior: Fix regression introduced in CopyHandler Editor: Fix regression introduced in CopyHandler Dec 3, 2018

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Dec 3, 2018

Sadly, it looks like it isn't possible to add e2e tests which would prevent this regression in the future. Such tests would only work when puppeteer operates with a real browser. It won't work in the headless mode which is the most expected way of running those tests. A related issue where this limiation is explained #2147.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Dec 3, 2018

To be clear, the bug is that we'd call event.preventDefault in cases where we weren't handling block copying behaviors?

@aduth
Copy link
Member

aduth left a comment

Another implementation which might be preferable is that the event handlers are bound and unbound in response to the change in condition applying. i.e. we don't bind on mount, but rather when the selectedBlockClientIds.length > 0 && ( hasMultiSelection() || ! documentHasSelection() ).

For the purposes of resolving the regression, I'm fine with the general approach here. I think the main blocker for me would be the reference of onCopy passed to addEventListener / removeEventListener needing a stronger guarantee to be the same.

@@ -54,7 +37,7 @@ export default compose( [
const selectedBlockClientIds = selectedBlockClientId ? [ selectedBlockClientId ] : getMultiSelectedBlockClientIds();

This comment has been minimized.

@aduth

aduth Dec 3, 2018

Member

I should have noted this in the previous pull request: But why do we do work here on selecting data, rather than within the callbacks? I don't think we should have any function calls in the upper wrapper aside from dispatch or select.

This comment has been minimized.

@gziolo

gziolo Dec 4, 2018

Member

It is evaluated at the time when a method is called so it doesn't matter from the technical standpoint. There will be some code duplication otherwise. I can move it to methods if you prefer to have it implemented this way.

This comment has been minimized.

@aduth

aduth Dec 4, 2018

Member

It's evaluated when the component mounts as well. I'm fine with (in-fact, prefer) the code duplication.

This comment has been minimized.

@gziolo

gziolo Dec 4, 2018

Member

Yep, updated to be placed inside functions, not that much code added to be honest 👍

onCut( dataTransfer ) {
this.onCopy( dataTransfer );
onCut( event ) {
this.onCopy( event );

This comment has been minimized.

@aduth

aduth Dec 3, 2018

Member

I don't trust this here. I should have noted it in the prior pull request too. Can we just have onCopy declared as a proper function in the scope, and call upon that here?

function onCopy() {}

return {
	onCopy,
	onCut() {
		onCopy();

		// ...
	},
};

This comment has been minimized.

@gziolo

gziolo Dec 4, 2018

Member

Yes, I wasn't sure about it either. It works but might break in the future 👍

onCut( event ) {
this.props.onCut( event.clipboardData );
event.preventDefault();
document.removeEventListener( 'copy', this.props.onCopy );

This comment has been minimized.

@aduth

aduth Dec 3, 2018

Member

I don't really trust that the reference of onCopy prop passed in componentDidMount is the same as the one in componentWillUnmount. In practice, it probably is, based on the current implementation of withDispatch. But I don't think it's a guarantee any component should rely on that a prop reference from mount is the same as in unmount.

In other words, an instance's onCopy = ( event ) => this.props.onCopy( event ); , while seemingly redundant, seems a much safer reference to rely upon.

This comment has been minimized.

@gziolo

gziolo Dec 4, 2018

Member

Yes, in retrospect I think you are right, if there are 2 instances of this component they would reference the same function with this implementation. I will fix it.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Dec 3, 2018

To be clear, the bug is that we'd call event.preventDefault in cases where we weren't handling block copying behaviors?

Yes, correct 👍

@gziolo gziolo force-pushed the fix/copy-handler-event-handling branch from 0db78ba to c9447c3 Dec 4, 2018

@aduth

aduth approved these changes Dec 4, 2018

Copy link
Member

aduth left a comment

Looks good 👍

const selectedBlockClientIds = getSelectedBlockClientId() ?
[ getSelectedBlockClientId() ] :
getMultiSelectedBlockClientIds();

removeBlocks( selectedBlockClientIds );
}

This comment has been minimized.

@aduth

aduth Dec 4, 2018

Member

Just to confirm: We're relying on the event.preventDefault() to have already occurred in onCopy?

This comment has been minimized.

@youknowriad youknowriad merged commit 20edbf2 into master Dec 5, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the fix/copy-handler-event-handling branch Dec 5, 2018

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Jan 4, 2019

Editor: Fix regression introduced in CopyHandler (WordPress#12543)
* Edior: Fix regression introduced in CopyHandler

* Refactor CopyHandler to be less dependent on withData implementation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment