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

Fix bug #329: Copy-pasting does not work on IE11 #330

Merged
merged 3 commits into from
Feb 19, 2016
Merged

Fix bug #329: Copy-pasting does not work on IE11 #330

merged 3 commits into from
Feb 19, 2016

Conversation

YoranBrondsema
Copy link
Collaborator

The Internet Explorers (including Edge) have a non-standard way of interacting with the Clipboard API (see http://caniuse.com/#feat=clipboard). In short, they expose a global window.clipboardData object instead of the per-event event.clipboardData object on the other browsers.

@mixonic
Copy link
Contributor

mixonic commented Feb 16, 2016

Cool @YoranBrondsema! Can you put the clipboard set/get data into a utility file that wraps the browser hack and describes it (as you did in the PR message)?

And since you wrote the test helpers to properly fallback for non-standard API, I don't think you should need to skip the assertions in IE? We should definitely test the paste behavior in IE, or this could regress again and we would still miss it.

@YoranBrondsema
Copy link
Collaborator Author

Cool @YoranBrondsema! Can you put the clipboard set/get data into a utility file that wraps the browser hack and describes it (as you did in the PR message)?

Sure, just added that in the latest commit.

And since you wrote the test helpers to properly fallback for non-standard API, I don't think you should need to skip the assertions in IE? We should definitely test the paste behavior in IE, or this could regress again and we would still miss it.

The thing is that the behavior of pasting is not entirely the same for Internet Explorer browsers now. If you look closely you see that I didn't skip all assertions on IE; I just modified the tests according to the altered behavior (the test suite tests/acceptance/editor-copy-paste-test.js now succeeds on IE11).

The reason for the difference is that IEs don't make a difference between text/plain and text/html when fetching the clipboard data. In order to avoid having HTML code pasted as plain-text, I parse all pasted data as HTML. But this means that text content such as abc\ndef (with the newline) will not be properly parsed as text. On non-IE this is parsed using the TextParser so will turn into <p>abc</p><p>def</p> but because of this limitation on IE it will be parsed by the HTMLParser (because it's always interpreted as HTML) resulting in <p>abc\ndef</p>. The same holds for lists.

I chose to go this route as I didn't think having a regex-style "HTML-detector" that would detect whether to use TextParser or HTMLParser would be robust enough. But I'd be happy to know if there's a better solution to this!

@bantic
Copy link
Collaborator

bantic commented Feb 19, 2016

@YoranBrondsema This is great, thanks. Truly odd that IE only maintains a "Text" property for the clipboard data.

I'm going to merge this in and add two separate issues for follow up:

  • that newlines in textNodes are preserved by the HTML Parser (we should be sanitizing that whitespace)
  • allowing pasting of plain-text in IE

Perhaps in the future we can do something trickier, like retargeting the pasted content into a hidden textarea and reading out whatever (HTML or text) was pasted into it.

bantic added a commit that referenced this pull request Feb 19, 2016
Fix bug #329: Copy-pasting does not work on IE11
@bantic bantic merged commit 5ef7d08 into bustle:master Feb 19, 2016
@YoranBrondsema
Copy link
Collaborator Author

Perhaps in the future we can do something trickier, like retargeting the pasted content into a hidden textarea and reading out whatever (HTML or text) was pasted into it.

I think this is the way to go but I wanted to fix the biggest issues quickly, that's why I didn't end up doing this. Thanks for creating the follow-up issues!

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.

3 participants