Fix for HTML pasting not working in IE #882

mahlero · 2017-06-14T14:55:03Z

This PR aims at closing the Pasting HTML doesn't work issue of #663

Basically it works by capturing HTML inserted by the browser's native paste action.

Successfully tested with the Paste HTML example (and with own project) in IE 11.

ianstormtaylor

Hey @mahlero thanks so much for taking the time to write up this PR! I wrote a bunch of review comments inline, and some questions for you too.

Thanks!

ianstormtaylor · 2017-06-21T22:11:45Z

examples/index.js

@@ -1,4 +1,4 @@
-
+import 'babel-polyfill'


What is this needed for?

At first, I could not get the examples running in IE 11. Importing babel-polyfill at that location helped. I think it had to do with the loading order of react-dom and babel-polyfill, similar to what is described here: https://stackoverflow.com/a/40928047.

Now after rebuilding node_modules it works, seems to be solved with react-dom 15.5.6. I could dig further into this and reinstall the old version, but for now I reverted that commit.

ianstormtaylor · 2017-06-21T22:13:34Z

src/components/content.js


-    debug('onPaste', { event, data })
-    this.props.onPaste(event, data)
+    if (doesNotSupportHtmlFromClipboard) {


Can you change this to just if (IS_IE) {?

And then can you add a comment about explaining exactly why we need this code, so that for future editing we don't break it. (Search the codebase for COMPAT: comments to see examples of how much detail to use, and include today's date.)

ianstormtaylor · 2017-06-21T22:14:07Z

src/utils/get-html-from-native-paste.js

+ * If pasted html can be retreived, it is added to the given `data` object, setting the `type` to `html`.
+ * As this method is asynchronous, a callback is needed to return the `data` object.
+ *
+ * Solution adapted from http://stackoverflow.com/a/6804718).


Thanks for pointing out.

ianstormtaylor · 2017-06-21T22:15:53Z

src/utils/get-html-from-native-paste.js

+import { findDOMNode } from 'react-dom'
+
+/**
+ * Get clipboard html data by capturing the html inserted by the browser's native paste action.


For all comments (including this one): Can you capitalize HTML, and make them formatted as paragraphs instead of individual line breaks, and always have punctuation at the end of the sentences.

ianstormtaylor · 2017-06-21T22:20:20Z

src/components/content.js

@@ -698,15 +699,25 @@ class Content extends React.Component {
    if (this.props.readOnly) return
    if (!this.isInEditor(event.target)) return

-    event.preventDefault()
+    const handleData = (data) => {


I'd like to refactor this code always have handleData be called either synchronously, or always on the next tick (setTimeout), instead of varying depending on the browser. I think this will lead to more predictable debugging of issues/plugins/etc.

It seems like we need the behavior to be synchronous, otherwise we don't have the opportunity to change the pasted content—either to prevent the paste, or to paste using the internal data model.

Is there any way for IE to be fixed without letting the paste occur already?

Ah wait, I think that's why you've got the remove/re-add logic for the node. Does that logic work even though those elements are controlled by React?

In that case I totally get the sync vs. async sometimes. That's unfortunate since it seems like a way for bugs to creep in, but makes sense.

I understand that handling the paste event in a async way for IE 11 and synchronously for other browsers increases the complexity and makes debugging more difficult. But I don't know of any other workaround for this.

That logic with the temporary cloned and focused contenteditable element does work and as far as I know, there is no interference with react as the native paste action happens directly on the next tick after the onPaste event fires.

ianstormtaylor · 2017-06-21T22:21:18Z

src/utils/get-html-from-native-paste.js

+function getHtmlFromNativePaste(component, data, callback) {
+  const contentNode = findDOMNode(component)
+
+  const clipboardNode = contentNode.cloneNode()


Can you change these variable names to contentNode -> el and clipboardNode -> clone?

Makes sense.

ianstormtaylor · 2017-06-21T22:22:32Z

src/utils/get-html-from-native-paste.js

+  clipboardNode.setAttribute('class', '')
+  clipboardNode.setAttribute('style', 'position: fixed; left: -9999px')
+
+  contentNode.parentNode.insertBefore(clipboardNode, contentNode)


Should this be inserted before the other one is removed? Or is there a reason why to not do the switch at the same point in time?

It could be done with appendChildas well. The original element is not removed, the cloned element is just a safe copy that receives the native paste action so the original element is left untouched.

ianstormtaylor · 2017-06-21T22:24:13Z

src/utils/get-transfer-data.js

+ */
+
+function getHtml(transfer) {
+  return transfer.types && transfer.types.length ? transfer.getData('text/html') || null : null


Is there a reason that these ones use && transfer.types.length and the ones above use && transform.types.indexOf(type)?

Not really, I unified this in my refactoring.

ianstormtaylor · 2017-06-21T22:24:37Z

src/utils/get-transfer-data.js

@@ -81,6 +81,61 @@ function getTransferType(data) {
 }

 /**
+ * Get fragment from transfers's `data` if possible, otherwise return null


Could all of these helpers be replaced by a single getType(transform, type) helper instead?

Yes, I refactored that.

This reverts commit a840507.

mahlero · 2017-06-23T16:10:39Z

Hey @ianstormtaylor, thanks a lot for your detailed comments! I adjusted my PR accordingly and gave you some replies - curious to hear what you think. Thank you!

ianstormtaylor

Hey @mahlero looks good, just one last change I think. Thanks for being responsive!

ianstormtaylor · 2017-06-23T19:36:02Z

src/components/content.js

@@ -698,15 +699,27 @@ class Content extends React.Component {
    if (this.props.readOnly) return
    if (!this.isInEditor(event.target)) return

-    event.preventDefault()
-    const data = getTransferData(event.clipboardData)
+    const handleData = (data) => {


Can you refactor this such that handleData is removed, by pulling these two lines out to above the if:

data.isShift = !!this.tmp.isShifting debug('onPaste', { event, data })

And by then just using this.props.onPaste directly in both places. Thanks!

No problem. In what way would you call this.props.onPaste:

from a callback given to the utility function like this
getHtmlFromNativePaste(this, data, dataWithHtml => this.props.onPaste(event, dataWithHtml))

or slightly different getHtmlFromNativePaste(this, htmlData => this.props.onPaste(event, { ...data, htmlData })) with callback({ html, type: 'html' }) from inside the utility function

or getHtmlFromNativePaste(this, event, data) and calling directly from inside the utility function this.props.onPaste(event, { ...data, html, type: 'html' })

The first two versions still use a 'proxy' callback, in the last version, it's not obvious that there is an asynchronous handling without looking into the utility function.

Actually we could probably have the callback just return html, and if it's null then have the logic call handled in the component layer. So maybe something along the lines of:

getHtmlFromNativePaste(this, (html) => { ... })

But even that makes me wonder if we shouldn't be using event.target instead of passing around this.

Thanks for your suggestions. I produced a corresponding version and hope we can solve this now!

…g not working in IE

jwoldan · 2017-08-01T15:28:35Z

Thanks for both of your hard work on this- is there any way I can help to get this merged? We could really use this fix!

ianstormtaylor · 2017-08-01T16:09:09Z

Thanks so much for this @mahlero, looks great! Sorry for the slowness.

jwoldan · 2017-08-01T20:27:06Z

Thank you both!

* imported babel-polyfill to make examples run in IE * Fix for HTML pasting not working in IE * Revert "imported babel-polyfill to make examples run in IE" This reverts commit a840507. * Refactored and corrected comments of fix: HTML pasting not working in IE * Removed handleData and tuned getTransferData call of fix: HTML pasting not working in IE

* ianstormtaylor/master: (61 commits) 0.21.2 alphabetize package.json scripts Speed up getting blocks at a range Remove instanceOf checks to allow Slate objects to be identifiable across module instances (ianstormtaylor#930) Refactor render arrow functions (ianstormtaylor#969) update prosemirror description in readme update prosemirror description in docs update prosemirror comparison in readme 0.21.1 Fix for HTML pasting not working in IE (ianstormtaylor#882) Remove unneeded check. (ianstormtaylor#961) Upgrade react-frame-component. (ianstormtaylor#962) adding forced-layout-example (ianstormtaylor#954) Reckon with inconsistencies between parse5 and native DOMParser (ianstormtaylor#952) Omit version on gzip size badge (ianstormtaylor#947) update issue template and contributing docs add reboo editor to resources add showcase to resources 0.21.0 update changelog ...

kumar2013 · 2017-08-18T13:19:25Z

Thank you for this amazing stuff and I have noticed 2 bugs (kind of) regarding this fix.

1). Every time when I paste something in the middle (or at the end) of the paragraph it messes up the content of the paragraph. Instead of pasting the new content at the right cursor position, it splits the content and put it in the wrong order.

For e.g: Based on the Current Cursor Position(CCP) while pasting we can split the content into contentBeforeCCP and contentAfterCCP. Once we get the new content from the clipboard the right order to insert should be contentBeforeCCP + newClipboardContent + contentAfterCCP.

However, it inserts in a wrong order like newClipboardContent + contentAfterCCP + contentBeforeCCP. In Draft.js there is a way to fix this but I don’t know how it works in slate.js. This behavior only occurs in Internet Explorer.

2). Pasting HTML content in IE works fine by just clicking on the right position and then pasting it (renders the HTML content but in the wrong order). However, if I press enter and then paste it on the new line then it pastes the text instead of the HTML. Also, it renders the expected HTML inside a break tag as the last child. I think we need to add browser native support like onPaste to get the HTML content.

Which is explained in the link you mentioned: https://stackoverflow.com/questions/2176861/javascript-get-clipboard-data-on-paste-event-cross-browser/6804718#6804718

mahlero · 2017-08-21T09:50:25Z

@kumar2013 although I spent a lot of time building an editor based on slate which is fully IE 11 compatible and provides working HTML copy/paste, I never experienced what you describe in 1) and 2). Maybe you could give some more hints and provide some code to explain your use case.

kumar2013 · 2017-08-21T10:26:33Z

@mahlero Thank you for the reply, I have created a new issue with the screenshot for the point 1). , please take a look #1016

kumar2013 · 2017-08-21T14:42:11Z

@mahlero About the point 2). I tried to paste HTML content by pressing enter at the end of the paragraph, this creates a new line and then pasted it. This should create an HTML content instead it created text content. Also, I can see the pasted HTML content separately (hidden inside a span and a break tag) as the last child of the parent element (via inspect element).

After copy & paste in a new line:

Inspect Element shows the expected HTML content for numbered list and table (hidden inside br tag):

This behaviour occurs only in IE 11 (didn't check the lower versions)

kumar2013 · 2017-08-21T14:47:13Z

Here is my onPaste code for your reference,

I didn't use .insertFragment(document) since that also breaks the HTML content I am pasting.

kumar2013 · 2017-08-22T13:08:48Z

@mahlero I can temporarily fix the point 1). by changing 'P' elements to 'Div' element.

About point 2). Every time when I press the enter button it creates a new line (in my case, new div element) which has a 'br' tag inside, this has been received as an argument in getHtmlFromNativePaste(component, callback), here the 'component' receives 'br' tag. Which creates the above-mentioned problem.

I have added a check for 'br' tag in your code and that fixes this problem. Please take a look.

Let me know if you can reproduce this so that we can get this fixed in the next release, thanks!

mahlero added 2 commits June 14, 2017 15:45

imported babel-polyfill to make examples run in IE

a840507

Fix for HTML pasting not working in IE

45c9d77

ianstormtaylor reviewed Jun 21, 2017

View reviewed changes

ianstormtaylor mentioned this pull request Jun 21, 2017

Can't paste into editor in IE 11 #897

Closed

mahlero added 2 commits June 23, 2017 13:44

Revert "imported babel-polyfill to make examples run in IE"

ace8b6d

This reverts commit a840507.

Refactored and corrected comments of fix: HTML pasting not working in IE

196b7b1

ianstormtaylor reviewed Jun 23, 2017

View reviewed changes

mahlero added 2 commits July 27, 2017 17:30

Removed handleData and tuned getTransferData call of fix: HTML pastin…

7690230

…g not working in IE

Merge branch 'master' into ie-11-html-paste-fix

a5a25a1

ianstormtaylor merged commit 6a2de0a into ianstormtaylor:master Aug 1, 2017

mahlero deleted the ie-11-html-paste-fix branch August 2, 2017 09:05

Fix for HTML pasting not working in IE #882

Fix for HTML pasting not working in IE #882

Conversation

mahlero commented Jun 14, 2017

ianstormtaylor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianstormtaylor Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahlero commented Jun 23, 2017

ianstormtaylor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwoldan commented Aug 1, 2017

ianstormtaylor commented Aug 1, 2017

jwoldan commented Aug 1, 2017

kumar2013 commented Aug 18, 2017 • edited Loading

mahlero commented Aug 21, 2017

kumar2013 commented Aug 21, 2017

kumar2013 commented Aug 21, 2017

kumar2013 commented Aug 21, 2017

kumar2013 commented Aug 22, 2017 • edited Loading

ianstormtaylor Jun 21, 2017 •

edited

Loading

kumar2013 commented Aug 18, 2017 •

edited

Loading

kumar2013 commented Aug 22, 2017 •

edited

Loading