Implement pasting external content with plain and styled text #13841

mkevins · 2019-02-12T18:31:34Z

Description

This PR aims to implement paste handling (targets 1 and 2) for the Mobile Gutenberg project. It is part of the work to address #238. The goal is to respond to a paste event from ReactNative code and transform the data for use with Gutenberg's existing paste handling implementations.

How has this been tested?

This PR is a work in progress, and so far testing has been manual (some manual tests done for Android and known issues are described here). When the implementation is more complete / stable, mock events will be created, as well as tests for the implementations.

A local test environment has been set up using the ./bin/setup-local-env.sh script.

DOM implementation on mobile

In order to implement these changes, it was necessary to modify several areas of the codebase. This mostly involved changes to *.native.js (Mobile Gutenberg only) files, but some shared code as well. In particular, a few extra safety checks have been added for situations where properties or methods are null or undefined in the ReactNative environment due to an incomplete implementation of DOM classes.

The codepaths utilizing some DOM methods can be hard to follow at times. As an example, when the pasteHandler function calls deepFilterHTML, it uses the specialCommentConverter filter. The raw-handling/special-comment-converter.js file has a dependency on @wordpress/dom, and makes use of the remove and replace functions from there.

There is a comment in the rawHandler function describing the use of the specialCommentConverter filter there: // Needed to create more and nextpage blocks. As pasteHandler uses the same filter and calls through to the same function (deepFilterHTML) I believe this comment's description applies to the filter's use in pasteHandler as well.

Deeper into this codepath, where deepFilterNodeList makes use of the filters (the forEach loop), is where the need arises for the contains method (which had to be patched into jsdom-jscore). In the forEach loop, item is actually the filter function.

Screenshots

Pasting styled text:

Pasting styled text with image:

Types of changes

For more flexibility in code-sharing, I refactored pasteHandler from raw-handling/index.js into its own file.
I added safety checks for jsdom methods/properties.
I refactored splitContent for use with onEnter and onPaste, and implemented createRecord for converting native content to the RichText format used in several other methods.
I implemented onPaste for plain and styled text.
I mirrored the implementation of onReplace from edit.js in edit.native.js, and pasted image blocks are now functional in the demo app.

Notes

Originally, I had planned on approaching this task in stages as described in the original issue. However, I realized upon implementing plain text paste, that styled text was implemented with very little additional effort.

Edit: Similarly, images are now working with the above approach, after implementing onReplace.

Related PRs

First steps to implement pasting external content wordpress-mobile/gutenberg-mobile#553

diegoreymendez · 2019-02-14T14:05:03Z

@mkevins - I'll be testing these changes through gutenberg-mobile, and will let you know if there's anything.

One suggestion I wanted to make for future PRs, is to avoid moving large blocks of code between files, unless the PR is specifically about that.

The reason behind this suggestion is that during the review the diff becomes a mix of code-changes and code-movements that becomes hard to differentiate, and as such, hard to validate.

mkevins · 2019-02-14T15:35:50Z

@mkevins - I'll be testing these changes through gutenberg-mobile, and will let you know if there's anything.

Thank you for testing it out!

One suggestion I wanted to make for future PRs, is to avoid moving large blocks of code between files, unless the PR is specifically about that.

The reason behind this suggestion is that during the review the diff becomes a mix of code-changes and code-movements that becomes hard to differentiate, and as such, hard to validate.

Thanks for the suggestion. I was torn on this, as it wasn't clear which would be more noisy, splitting on PRs with multiple related PRs across projects, or reducing the number of PRs and clarifying the changes in the commit message. I refactored (the big code movements) in the first commit and isolated those changes by commit. I'll keep this in mind in future PRs, and try to keep them smaller.

If you have any questions, or want clarification on any part of the code, I'll be happy to help.

mkevins · 2019-02-15T10:36:23Z

I added a commit to refactor rawHandler back into raw-handling/index.js (rather than its own file), as it is not required on the native side for this implementation. Hopefully, this will make reviewing the PR easier.

hypest · 2019-02-15T12:45:15Z

👋 @iseulde , can you make a pass on this PR? It touches some files used from the web too so, just want to make sure we don't break anything there. Thanks!

packages/block-library/src/paragraph/edit.native.js

hypest · 2019-02-15T14:07:14Z

packages/blocks/src/api/raw-handling/phrasing-content-reducer.js

@@ -4,7 +4,9 @@
 import { wrap, replaceTag } from '@wordpress/dom';

 export default function( node, doc ) {
-	if ( node.nodeName === 'SPAN' ) {
+	// In jsdom-jscore, 'node.style' can be null.


Kinda a nit but, perhaps it's not too clear what jsdom-jscore means in this context. Of course, jsdom-jscore it's the DOM parser we're using in the native mobile (React Native) version of Gutenberg, but I think I'd suggest add that bit to the comment to, to help out web-side friends read the code more easily.

So, my suggestion is to refer with In native mobile (jsdom-jscore), ... instead of the current. What do you think? Same for a couple other similar references in this same PR.

On a less of a nit thing, I wonder about the (emphasis mine on the "can"):

'node.style' *can* be null.

Does that mean that sometimes node.style is undefined and some other it's not?

In the cases where it's undefined, is it OK to just ignore it like we're doing currently in the PR? I wonder if being undefined denotes an unrecoverable error and we should let the user know about it, in some way.

Should we at least log an error/warning?

There are a few places in the PR to ask the same question, but let's discuss it here and then we apply accordingly.

Yes, good suggestion! I'll edit those, and keep that in mind for future comments.

Does that mean that sometimes node.style is undefined and some other it's not?

With the change of wording in the comment to: In native mobile (jsdom-jscore), ..., it's probably more accurate to say that node.style is undefined.

Should we at least log an error/warning?

In the codepath where this is used, node is actually an HTMLElement, as the condition is satisfied that the element's nodeName is SPAN. HTMLElement nodes are supposed to have a style property which returns a read-only CSSStyleDeclaration object, which can be used to retrieve the inline styles applied to the element via the style attribute. This is then used to wrap the node in appropriate tag(s) (strong, em, etc.). In the case where the DOM implementation does not support the style property on a node, I believe it is safe to ignore, but that we may lose style information that was associated with the content via inline styles on a span's attribute.

The property seems to have had wide support for a long time, but I wonder if there are any browsers that don't support this property, or that leave it undefined or null when no style attribute was present on the element. My reason for wording the comment somewhat "vaguely" is that I tend to assume that there could be such a case in the wild, and thus the safety check could be useful even in the browser implementation. I checked the browser compatibility for it on MDN, and it lists a few question marks, but that just means unconfirmed AFAIK.

TL;DR:

The check will not lead to an unrecoverable error, but will gracefully degrade the feature that normalizes some inline styles on span tags. I don’t have a strong opinion on whether that warrants logging a warning; I suppose it depends on the verbosity desired (e.g. web: “please use a better browser :)”, mobile: "mobile currently lacks support for this feature").

mobile: "mobile currently lacks support for this feature"

I think that's a good case to actually log since, these things are useful while offering support to users.

While at it, since this is a known limitation, let's add a "Known Limitations" section to wordpress-mobile/gutenberg-mobile#553 and mention this there. WDYT?

packages/blocks/src/api/raw-handling/paste-handler.js

packages/editor/src/components/rich-text/index.native.js

packages/dom/package.json

hypest · 2019-02-15T14:59:55Z

packages/editor/src/components/rich-text/index.native.js

@@ -92,35 +98,32 @@ export class RichText extends Component {
 	 * handler.
 	 *
 	 */
-	splitContent( htmlText, start, end ) {
+	splitContent( currentRecord, blocks = [], isPasted = false ) {
 		const { onSplit } = this.props;

 		if ( ! onSplit ) {


Not strictly related to this PR but, it seems that the onSplit prop is already marked as deprecated on the web side so, since we're touching the splitContent method, what do you think about mirroring this (https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/components/rich-text/index.js#L84-L93) here too?

I'd suggest a follow up PR with that since it's not a blocker for this PR and adding more diff to this is not desirable :)

hypest · 2019-02-15T15:06:01Z

packages/editor/src/components/rich-text/index.native.js

@@ -92,35 +98,32 @@ export class RichText extends Component {
 	 * handler.
 	 *
 	 */
-	splitContent( htmlText, start, end ) {
+	splitContent( currentRecord, blocks = [], isPasted = false ) {


isPasted = false

I understand that we just need this flag here but, may I suggest copying what the web side is providing the corresponding method with? The idea is to stay as close as possible to the web code, wishing we can refactor and re-use the same code eventually.

I this case, let's pass a context object with a paste (boolean) field. WDYT?

(By the way, only reason I'm not just recommending extracting this method into a common file with the web is that there's this lastEventCount trigger in this one, and extraction would make the code complex.)

hypest

👋 @mkevins , great work here, works really nicely!

I left a few comments which I think could use some minor work. Good thing is, I don't see any blockers for this PR in any of those! I'd suggest we tackle them all in follow up PR(s) and go ahead and merge this one. Let me know if that's OK with you too.

If OK with you, then please open a ticket with a checklist of all the known issues and additional work needed so we can more easily work against them.

hypest

Looks good to me! Nice work @mkevins, thanks!

I'll wait for the restarted Travis job to get green and I will merge this PR.

When you get the chance, please open a ticket in the Gutenberg-mobile repo with a checklist of all the known issues and additional work needed so we can more easily work against them.

mkevins · 2019-02-20T15:19:30Z

When you get the chance, please open a ticket in the Gutenberg-mobile repo with a checklist of all the known issues and additional work needed so we can more easily work against them.

I've added this issue to track the related work:
wordpress-mobile/gutenberg-mobile#624

* Refactor raw-handler and paste-handler into their own files * Add undefined and null checks for jsdom-jscore node methods/properties * Implement createRecord method for converting native content to RichText * Refactor splitContent and onEnter for common interface with onPaste * Begin implementing onPaste for plain and styled text * Fix some lint errors and some invalid references. * Add undefined check for jsdom-jscore node in phrasing-content-reducer * Add onReplace method to ParagraphEdit * Refactor raw-handler back into index.js * Remove unused parameters from RichText create method

mkevins requested review from aduth, gziolo, noisysocks, oandregal and youknowriad as code owners February 12, 2019 18:31

mkevins mentioned this pull request Feb 12, 2019

First steps to implement pasting external content wordpress-mobile/gutenberg-mobile#553

Merged

daniloercoli requested review from hypest and daniloercoli and removed request for youknowriad, oandregal, noisysocks, gziolo and aduth February 12, 2019 18:40

gziolo added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Feb 12, 2019

mkevins added 6 commits February 13, 2019 21:33

Refactor raw-handler and paste-handler into their own files

a9533d0

Add undefined and null checks for jsdom-jscore node methods/properties

9260ba0

Implement createRecord method for converting native content to RichText

2ea3c66

Refactor splitContent and onEnter for common interface with onPaste

0c66980

Begin implementing onPaste for plain and styled text

be1328d

Fix some lint errors and some invalid references.

304978b

mkevins force-pushed the try/pasting-external-content branch from 4b0b0bf to 304978b Compare February 13, 2019 11:38

mkevins added 2 commits February 14, 2019 01:32

Add undefined check for jsdom-jscore node in phrasing-content-reducer

cb03073

Add onReplace method to ParagraphEdit

280bb69

hypest requested review from diegoreymendez and removed request for hypest February 14, 2019 09:36

Refactor raw-handler back into index.js

3e74445

hypest requested a review from ellatrix February 15, 2019 12:44

hypest reviewed Feb 15, 2019

View reviewed changes

packages/block-library/src/paragraph/edit.native.js Show resolved Hide resolved

Merge branch 'master' into try/pasting-external-content

ef2f265

hypest reviewed Feb 15, 2019

View reviewed changes

ellatrix reviewed Feb 15, 2019

View reviewed changes

packages/blocks/src/api/raw-handling/paste-handler.js Show resolved Hide resolved

ellatrix reviewed Feb 15, 2019

View reviewed changes

packages/editor/src/components/rich-text/index.native.js Outdated Show resolved Hide resolved

hypest reviewed Feb 15, 2019

View reviewed changes

packages/dom/package.json Show resolved Hide resolved

hypest reviewed Feb 15, 2019

View reviewed changes

hypest approved these changes Feb 15, 2019

View reviewed changes

mkevins added 2 commits February 19, 2019 00:35

Remove unused parameters from RichText create method

df68769

Merge branch 'master' into try/pasting-external-content

510818b

hypest approved these changes Feb 19, 2019

View reviewed changes

hypest merged commit 23e59d1 into WordPress:master Feb 19, 2019

mkevins mentioned this pull request Feb 20, 2019

Pasting external content - known issues wordpress-mobile/gutenberg-mobile#624

Open

20 tasks

youknowriad added this to the 5.2 (Gutenberg) milestone Mar 4, 2019

ellatrix mentioned this pull request Mar 4, 2019

Bump plugin version to 5.2.0-rc.1 #14210

Merged

hypest mentioned this pull request Dec 17, 2019

Convert to blocks: preserve alignment #19097

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement pasting external content with plain and styled text #13841

Implement pasting external content with plain and styled text #13841

mkevins commented Feb 12, 2019 •

edited

Loading

diegoreymendez commented Feb 14, 2019

mkevins commented Feb 14, 2019

mkevins commented Feb 15, 2019

hypest commented Feb 15, 2019

hypest Feb 15, 2019

hypest Feb 15, 2019 •

edited

Loading

mkevins Feb 15, 2019

mkevins Feb 18, 2019

hypest Feb 18, 2019

hypest Feb 15, 2019

hypest Feb 15, 2019 •

edited

Loading

hypest left a comment

hypest left a comment

mkevins commented Feb 20, 2019

Implement pasting external content with plain and styled text #13841

Implement pasting external content with plain and styled text #13841

Conversation

mkevins commented Feb 12, 2019 • edited Loading

Description

How has this been tested?

DOM implementation on mobile

Screenshots

Pasting styled text:

Pasting styled text with image:

Types of changes

Notes

Related PRs

diegoreymendez commented Feb 14, 2019

mkevins commented Feb 14, 2019

mkevins commented Feb 15, 2019

hypest commented Feb 15, 2019

hypest Feb 15, 2019

Choose a reason for hiding this comment

hypest Feb 15, 2019 • edited Loading

Choose a reason for hiding this comment

mkevins Feb 15, 2019

Choose a reason for hiding this comment

mkevins Feb 18, 2019

Choose a reason for hiding this comment

hypest Feb 18, 2019

Choose a reason for hiding this comment

hypest Feb 15, 2019

Choose a reason for hiding this comment

hypest Feb 15, 2019 • edited Loading

Choose a reason for hiding this comment

hypest left a comment

Choose a reason for hiding this comment

hypest left a comment

Choose a reason for hiding this comment

mkevins commented Feb 20, 2019

mkevins commented Feb 12, 2019 •

edited

Loading

hypest Feb 15, 2019 •

edited

Loading

hypest Feb 15, 2019 •

edited

Loading