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

Remove TinyMCE paste plugin #9091

Merged
merged 7 commits into from Aug 21, 2018

Conversation

Projects
None yet
4 participants
@iseulde
Member

iseulde commented Aug 17, 2018

Description

We no longer need the TinyMCE paste plugin because we have our own paste logic in Gutenberg. The only reason why it is still there is to facilitate getting the pasted content. Initially I thought we should copy over the paste bin technique, but it seems all modern browsers support clipboardData. I haven't tested IE11 and Android, so I'd be grateful if someone could try this out there. If either browser does not support clipboardData, we can add the paste bin for these browsers as a fallback.

How has this been tested?

Ensure all paste functionality still works.

  • Copy HTML from anywhere and paste it in a rich text field.
  • Copy plain text from e.g. a text editor and paste it.
  • Copy an image from the web and paste it.
  • Try paste in different ways:
    • Use the browser menu bar.
    • Use the right-click menu.
    • Use the keyboard shortcut.

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

iseulde added some commits Aug 17, 2018

@iseulde iseulde requested a review from WordPress/gutenberg-core Aug 17, 2018

@tofumatt

The code here seems fine, but if I paste in HTML to a RichText field, it stays RichText in IE 11:

2018-08-17 18 48 46

It looks like IE11 doesn't support the HTML type for paste https://caniuse.com/#search=clipboardData

If we're okay with IE 11 not accepting HTML paste then maybe it's fine? Or we could use the "paste bin" fallback you mentioned, but I don't know what that is 😅

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Aug 17, 2018

Member

I think IE11 (and Edge?) uses a nonstandard clipboardData on the window global:

https://stackoverflow.com/a/30126108/995445

Member

aduth commented Aug 17, 2018

I think IE11 (and Edge?) uses a nonstandard clipboardData on the window global:

https://stackoverflow.com/a/30126108/995445

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Aug 20, 2018

Member

Okay, let's first try the global etc. I'll add those back. I'd rather avoid the "paste bin" mess if possible.

Member

iseulde commented Aug 20, 2018

Okay, let's first try the global etc. I'll add those back. I'd rather avoid the "paste bin" mess if possible.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Aug 20, 2018

Member

Okay, that is suggesting:

  • window.clipboardData to work in IE.

Previously we also had:

  • event.dataTransfer
  • document.dataTransfer

as fallbacks (also used by TinyMCE).

Let's try the first and see if it works, then try the others.

Member

iseulde commented Aug 20, 2018

Okay, that is suggesting:

  • window.clipboardData to work in IE.

Previously we also had:

  • event.dataTransfer
  • document.dataTransfer

as fallbacks (also used by TinyMCE).

Let's try the first and see if it works, then try the others.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Aug 20, 2018

Member

@tofumatt Does the latest change work with IE11?

Member

iseulde commented Aug 20, 2018

@tofumatt Does the latest change work with IE11?

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Aug 20, 2018

Member

I just tested it in IE11 and it still pastes HTML (the contents of the Gutenberg Demo source) into an empty RichText block as text, not HTML (same as my GIF before).

I think the issue is because IE11 doesn't support the HTML paste type for the clipboardData object. So either one won't support HTML paste 😞

Member

tofumatt commented Aug 20, 2018

I just tested it in IE11 and it still pastes HTML (the contents of the Gutenberg Demo source) into an empty RichText block as text, not HTML (same as my GIF before).

I think the issue is because IE11 doesn't support the HTML paste type for the clipboardData object. So either one won't support HTML paste 😞

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Aug 20, 2018

Member

Weird. I'll get a VirtualBox copy and try to fix it. MDN says it's supported in IE too... https://developer.mozilla.org/en-US/docs/Web/API/ClipboardEvent/clipboardData

Member

iseulde commented Aug 20, 2018

Weird. I'll get a VirtualBox copy and try to fix it. MDN says it's supported in IE too... https://developer.mozilla.org/en-US/docs/Web/API/ClipboardEvent/clipboardData

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Aug 20, 2018

Member

Oh, so clipboardData is supported but if you check https://caniuse.com/#search=clipboarddata, it says (for IE11):

Only supports Text and URL data types and uses a non-standard method of interacting with the clipboard.

So I think that means the HTML data type won't get pasted/picked up properly.

Member

tofumatt commented Aug 20, 2018

Oh, so clipboardData is supported but if you check https://caniuse.com/#search=clipboarddata, it says (for IE11):

Only supports Text and URL data types and uses a non-standard method of interacting with the clipboard.

So I think that means the HTML data type won't get pasted/picked up properly.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Aug 20, 2018

Member

Hm, okay, so we event.clipboardData actually works, it just throws an invalid argument error. So no way to check support other than browser check.

Member

iseulde commented Aug 20, 2018

Hm, okay, so we event.clipboardData actually works, it just throws an invalid argument error. So no way to check support other than browser check.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Aug 20, 2018

Member

So either we just paste the plain text we are given in IE, or we don't support paste, or I try adding the "paste bin" fix.

Pasting doesn't seem to work in master either atm. Nor does copying blocks.

Member

iseulde commented Aug 20, 2018

So either we just paste the plain text we are given in IE, or we don't support paste, or I try adding the "paste bin" fix.

Pasting doesn't seem to work in master either atm. Nor does copying blocks.

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Aug 20, 2018

Member
Member

tofumatt commented Aug 20, 2018

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 20, 2018

Contributor

Not for this PR, just a question regarding...

Pasting plain to avoid all that code in IE11 doesn’t seem like the worst thing to me

Can we detect the combination of IE11 and the paste action and throw up a modal with a "did you know" prompt suggesting users upgrade to a modern browser if they can, or paste in HTML mode if they can't?

Contributor

jasmussen commented Aug 20, 2018

Not for this PR, just a question regarding...

Pasting plain to avoid all that code in IE11 doesn’t seem like the worst thing to me

Can we detect the combination of IE11 and the paste action and throw up a modal with a "did you know" prompt suggesting users upgrade to a modern browser if they can, or paste in HTML mode if they can't?

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Aug 20, 2018

Member

Can we detect the combination of IE11 and the paste action and throw up a modal with a "did you know" prompt suggesting users upgrade to a modern browser if they can, or paste in HTML mode if they can't?

Yes, we can do this, but it might be annoying if it pops up every time. Maybe pop up per page reload?

Member

iseulde commented Aug 20, 2018

Can we detect the combination of IE11 and the paste action and throw up a modal with a "did you know" prompt suggesting users upgrade to a modern browser if they can, or paste in HTML mode if they can't?

Yes, we can do this, but it might be annoying if it pops up every time. Maybe pop up per page reload?

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 20, 2018

Contributor

Even just once. Could be tied to the tips state.

Contributor

jasmussen commented Aug 20, 2018

Even just once. Could be tied to the tips state.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Aug 20, 2018

Member

UC Browser for Android also does not support it, which probably also means all our other copy logic doesn't work there such as copying blocks. Not sure if there's more things that Gutenberg uses that they don't support. I don't know much about Android and this browser, it doesn't seem like a default one? But it has significant market share. I'll try to test it tonight on an Android device.

Member

iseulde commented Aug 20, 2018

UC Browser for Android also does not support it, which probably also means all our other copy logic doesn't work there such as copying blocks. Not sure if there's more things that Gutenberg uses that they don't support. I don't know much about Android and this browser, it doesn't seem like a default one? But it has significant market share. I'll try to test it tonight on an Android device.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 20, 2018

Contributor

I've never heard of "UC browser" before. Not being mean, but I think if we can support mobile Safari, chrome and Firefox on Android, we're probably good.

Contributor

jasmussen commented Aug 20, 2018

I've never heard of "UC browser" before. Not being mean, but I think if we can support mobile Safari, chrome and Firefox on Android, we're probably good.

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Aug 20, 2018

Member
Member

tofumatt commented Aug 20, 2018

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 21, 2018

Contributor

UC Browser is a Chinese browser, it’s huge.

Damn. I stand corrected.

If there's no way we can support copy features on that browser, then we could either detect the browser and hide those features, or throw up a notice, or something.

Contributor

jasmussen commented Aug 21, 2018

UC Browser is a Chinese browser, it’s huge.

Damn. I stand corrected.

If there's no way we can support copy features on that browser, then we could either detect the browser and hide those features, or throw up a notice, or something.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Aug 21, 2018

Member

Tested UC Browser with current Gutenberg and TinyMCE, and it will only paste plain text.

Member

iseulde commented Aug 21, 2018

Tested UC Browser with current Gutenberg and TinyMCE, and it will only paste plain text.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Aug 21, 2018

Member

Added a fix for IE11. Now both IE11 and UC browser should just paste plain text.

Member

iseulde commented Aug 21, 2018

Added a fix for IE11. Now both IE11 and UC browser should just paste plain text.

@iseulde iseulde added this to the 3.7 milestone Aug 21, 2018

@tofumatt

Largely looks good but I have a few code quality comments and a question about that nested try/catch.

iseulde and others added some commits Aug 21, 2018

@tofumatt

Awesome 👍

I tested locally in IE11 and everything works well. 🚢

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Aug 21, 2018

Member

Thanks a lot @tofumatt!

Member

iseulde commented Aug 21, 2018

Thanks a lot @tofumatt!

@iseulde iseulde merged commit 1d92884 into master Aug 21, 2018

2 checks passed

codecov/project 50.89% (+0.05%) compared to 2318842
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@iseulde iseulde deleted the try/drop-paste-plugin branch Aug 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment