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

First steps to implement pasting external content #553

Merged
merged 18 commits into from
Feb 19, 2019

Conversation

mkevins
Copy link
Contributor

@mkevins mkevins commented Feb 7, 2019

This PR aims to intercept a paste action in ReactAztecText and emit it as an event for consumption in JS. It is part of the work to address #238. The goal is to serialize the clipboard contents (as text and html where possible). The event also includes the current text and the selection/cursor information to be handled in JS.

  • onTextContextMenuItem method is called when the user selects paste from the context menu (typically after a long-press, or a touch of the cursor handle).
  • This method is overridden to conditionally call onPaste if shouldHandleOnPaste is true.
    • shouldHandleOnPaste is mapped to JS and is truthy when the JS onPaste method is defined, and falsy otherwise.
  • onPaste loops through all items in the primary clipboard, concatenating both text and html representations of them, and dispatches a JS event with the data.

I think it is reasonable to have canCoalesce return false as paste events will not be so frequent as to warrant it. Frequent events from native code can be coalesced (merged) into one another to reduce their frequency (and load) on the JS side.

Manual tests

There are currently no automated tests for this implementation, however, here are some manual tests that I've tried:

Copy to the clipboard and paste as follows:

  1. Paste into a new (empty) paragraph block
    • Expected behavior
      • Plain text
        • Inline: text inserted
        • Multi-line1: text inserted as a single paragraph
        • Multi-paragraph2: text split into multiple paragraph blocks
      • Styled text
        • Inline: styled text inserted
        • Multi-line: styled text inserted as a single paragraph
        • Multi-paragraph: styled text split into heading and paragraph blocks
        • Multi-paragraph (starting with heading): paragraph block replaced by heading block
      • Styled text with image
        • Multi-paragraph: styled text split into heading, image, and paragraph blocks
        • Multi-paragraph (starting with image): paragraph block replaced by image block
  2. Paste into a paragraph block with content
    • Expected behavior
      • Plain text / Styled text
        • Inline: text inserted at cursor position
        • Inline (heading): text inserted as plain text at cursor position
        • Multi-line: text inserted at cursor as a single paragraph (splitting current paragraph if necessary)
        • Multi-paragraph: text split into multiple blocks at cursor position (splitting current paragraph if necessary)
      • Styled text with image
        • Multi-paragraph: styled text split into heading, image, and paragraph blocks (splitting current paragraph if necessary)
  3. Paste into a paragraph block with content with some inner content selected
    • Expected behavior
      • Plain text / Styled text / Styled text with image
        • For all scenarios above: selected text removed and paste continued at cursor as above
      • URL
        • Selected text remains and is converted to link upon paste

1 - Multi-line => single paragraph - single '\n' characters
2 - Multi-paragraph => multiple consecutive '\n' characters

Known issues

When bullet lists are copied to the clipboard and pasted to a paragraph block, the bullets are lost. Currently, list blocks are not supported. When support is added, behavior should be tested again.

When pasting into heading blocks

When multi-paragraph / image content is pasted into an empty heading block, the heading block remains, and contains the text null.

When multi-paragraph / image content is pasted into a heading block with all text selected, the text remains, and the clipboard contents are added as blocks after the heading block.

When multi-paragraph / image content is pasted into a heading block with inner text selected, the selected text is removed, the text before the selection remains in the heading block, the clipboard contents are added as blocks after the heading block, and the text after the selection is added to a paragraph block below. I'm not sure whether this is the expected behavior.

When pasting into code blocks

When multi-paragraph / image content is pasted into a code block, the block is not split, and new blocks are not created. Styled text (colored links) keep their color, and images are converted to an object replacement character (which renders as a little square).

Cursor position

After pasting, the cursor position is expected to be at the end of the pasted content. Currently, the cursor position does not follow this behavior. This may be related to #602.

DOM implementation on mobile

Some of the codepaths in the mobile project make use of implementations shared with the web Gutenberg project, where there is an expectation of a more full DOM implementation. The mobile project uses jsdom-jscore, which only supports a limited set of properties and methods for the various DOM classes. This can lead to errors, or degradation of a particular feature. In some cases, the jsdom-jscore implementation can be patched globally to restore the correct behavior, while in other cases, we may need to add an extra check, and log that support is currently limited. An example regarding <span> tags is noted below in the Known limitations section.

As the codepaths utilizing this can be difficult to follow at times, a detailed example can be in the description of the related gutenberg PR.

Known limitations

Third-party keyboard / clipboard managers

Some Android devices have, or come pre-installed with third-party keyboard / clipboard managers which interact through the input method framework in their own way (e.g. Samsung Galaxy). As this implementation intercepts the selection of the standard Android TextView context menu, support for pasting via those methods may not be supported. It may be possible to explore adding support in the future.

Loss of inline style information on span tags

When pasting html containing text with inline styles on a <span> tag, the styles may be lost. For more details about this limitation, see here.

Related PRs

@hypest
Copy link
Contributor

hypest commented Feb 8, 2019

Thanks for the PR @mkevins ! This already starts to look being on the right path!

  • onTextContextMenuItem method is called when the user selects paste from the context menu

I wonder, will that also cover the case of the user pasting via the "Clipboard" button on Galaxy phones? If not can we add support? It doesn't have to be in this same PR though if it's not trivial to add. What do you think?

@mkevins
Copy link
Contributor Author

mkevins commented Feb 8, 2019

Hi @hypest, thanks for taking a look at this PR! Regarding your question:

I wonder, will that also cover the case of the user pasting via the "Clipboard" button on Galaxy phones?

I don't think this PR will address that case, as this intercepts the selection of the standard Android TextView context menu. I think Samsung (and other third party keyboard / clipboard managers) interact through the input method framework in their own way. I think it's possible to add support, but I think it would be non-trivial. Probably best for a separate PR.

@hypest
Copy link
Contributor

hypest commented Feb 11, 2019

I think it's possible to add support, but I think it would be non-trivial. Probably best for a separate PR.

Indeed, makes sense to tackle that in a separate PR 👍

Copy link
Contributor

@diegoreymendez diegoreymendez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't really add much on this review since I focus on iOS development, but I added a question regarding the event we're sending to Javascript.

@diegoreymendez
Copy link
Contributor

@mkevins - Since I'd need to implement the iOS side of this feature, I was wondering if the current state of it is such that I can go ahead and get started. Can you let me know?

Also, could you add to the issue description:

  1. The testing steps for different supported scenarios.
  2. Any info on non-supported scenarios, or known issues.

Thanks!

@mkevins
Copy link
Contributor Author

mkevins commented Feb 14, 2019

@mkevins - Since I'd need to implement the iOS side of this feature, I was wondering if the current state of it is such that I can go ahead and get started. Can you let me know?

Hi @diegoreymendez, I'd love to help you on getting the iOS side of this feature implemented. Yes, it is ready for you to implement, as it is working as expected on the Android side. Please don't hesitate if you have any questions regarding this.

Also, could you add to the issue description:

  1. The testing steps for different supported scenarios.

I've considered creating tests for the Android ReactNative wrapper code, however, was advised to prioritize the implementation itself until a testing setup is in place. I am working on tests for the JS side, and I will add details to the issue description and related PR soon.

  1. Any info on non-supported scenarios, or known issues.

Nothing that I'm aware of.

@diegoreymendez
Copy link
Contributor

I've considered creating tests for the Android ReactNative wrapper code, however, was advised to prioritize the implementation itself until a testing setup is in place. I am working on tests for the JS side, and I will add details to the issue description and related PR soon.

I'm actually not referring to automated tests, but to a description of manual tests the reviewer can do, and what the expected behavior is.

@mkevins
Copy link
Contributor Author

mkevins commented Feb 14, 2019

Ah, I understand. I'll add that now. Thanks for the suggestion!

@daniloercoli
Copy link
Contributor

On a fresh checkout i was getting the error gutenberg-mobile/gutenberg/packages/blocks/src/api/raw-handling/markdown-converter.js: Module showdown does not exist in the Haste module map so i guess you need to add "showdown": "^1.8.6", to dependencies section of package.json.

@hypest
Copy link
Contributor

hypest commented Feb 15, 2019

Thanks for updating the PR description with testing steps and Known Issues @mkevins !

It seems that the issues reported can be worked on in follow up PRs, no need to address them in this one.

@@ -5,6 +5,9 @@ import { createElement } from '@wordpress/element';
import jsdom from 'jsdom-jscore';
import jsdomLevel1Core from 'jsdom-jscore/lib/jsdom/level1/core';

// Import for side-effects: Patches for jsdom-jscore.
import './jsdom-patches';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this seems rather important since we're touching how jsdom-jscore is working. I'm sure we need the patches so, can you add some comments in the code on what do we need the patches for? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I'll add some more comments to this to clarify.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mkevins ! Can we expand on the reason(s) we replace each of the function prototypes? That insight will help anyone reading the code to understand why do we even replace these function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added PR #826 to address this.

@diegoreymendez diegoreymendez self-requested a review February 15, 2019 14:31
@mkevins
Copy link
Contributor Author

mkevins commented Feb 18, 2019

On a fresh checkout i was getting the error gutenberg-mobile/gutenberg/packages/blocks/src/api/raw-handling/markdown-converter.js: Module showdown does not exist in the Haste module map so i guess you need to add "showdown": "^1.8.6", to dependencies section of package.json.

Hi @daniloercoli , I was wondering if you were still having this problem. Is it possible that you have a node_modules folder present in the inner gutenberg directory (the submodule)? This directory is created when running npm install or yarn install from the inner directory; it may be related to that, and might be resolved by simply removing that directory: from within gutenberg-mobile/gutenberg/: rm -rf node_modules.

@hypest
Copy link
Contributor

hypest commented Feb 18, 2019

Is it possible that you have a node_modules folder present in the inner gutenberg directory (the submodule)?

Hmm, I don't have an inner node_modules in the gutenberg submodule folder but I too had to yarn add showdown 🤔. Is a clean checkout working for you OK @mkevins ?

@mkevins
Copy link
Contributor Author

mkevins commented Feb 18, 2019

I ran a clean clone and had the same issue. Adding the showdown dependency as @daniloercoli suggested resolves it. Thanks @daniloercoli and @hypest for helping resolve the showdown issue.

Copy link
Contributor

@diegoreymendez diegoreymendez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the iOS point of view, I think this base should work. We can provide the necessary inputs to make it work.

Copy link
Contributor

@hypest hypest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get this merged! Nice work!

Let's updated this PR when the Gutenberg side PR gets merged, to point to the merge commit, and then we can merge this too!

@hypest hypest merged commit db36808 into wordpress-mobile:develop Feb 19, 2019
@mkevins
Copy link
Contributor Author

mkevins commented Feb 20, 2019

Issues are being tracked here: #624

@koke koke mentioned this pull request Apr 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants