Bug 174165 – DataTransfer shouldn't contain text/html when performing Paste and Match Style

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Javan Makhmali]

Created attachment 314620 [details]
Paste event clipboard data inspector

The ClipboardData for paste events are identical when performing Paste (⌘V) and Paste and Match Style (⌥⇧⌘V). Because they're indistinguishable, it's impossible to programmatically handle pastes correctly in a rich text editor (like https://github.com/basecamp/trix, which I help maintain). There's no way to infer the desired format and pick an appropriate type.

To reproduce, open the attached paste-inspector.html file in Safari and paste as instructed. Note that the logged paste events are identical and both contain "text/html" and "text/plain" types. Chrome and Firefox only return a "text/plain" type when performing Paste and Match Style, which I assume is correct.

This issue is also present in previous versions of Safari.

Screenshots
Safari: https://cl.ly/291S1I3L2B25
Chrome: https://cl.ly/3m3B1P0D0Q2E
Firefox: https://cl.ly/3M3v2R2r2T2H

[Radar WebKit Bug Importer]

<rdar://problem/33138027>

[Wenson Hsieh]

@Javan:

Thanks for reporting this! I'll look into it.

FYI: in the meantime, one way you can work around this is to listen for a beforeinput or input event with the inputType "insertFromPaste" instead. When using Paste and Match Style, getting "text/html" from the event's dataTransfer will yield markup representing the unstyled fragment to be inserted into the DOM. So in both cases of pasting and pasting to match style, if you want to know the markup that is about to be inserted and do something custom with it, you can do something like:

editor.addEventListener("beforeinput", (event) => {
    if (event.inputType === "insertFromPaste") {
        let markupToInsert = event.dataTransfer.getData("text/html");

        console.log(`Inserting markup: ${markupToInsert}!`);
        /* Perform custom handling of the paste. */

        event.preventDefault();
    }
});

...of course, having the clipboard data explicitly contain just "text/plain" in this case would still be helpful, but this might be a viable alternative.

[Javan Makhmali]

(In reply to Wenson Hsieh from comment #2)
> @Javan:
> 
> Thanks for reporting this! I'll look into it.
> 
> FYI: in the meantime, one way you can work around this is to listen for a
> beforeinput or input event with the inputType "insertFromPaste" instead.
> When using Paste and Match Style, getting "text/html" from the event's
> dataTransfer

Cool! I had no idea paste data was available in beforeinput events. 💖

[Ryosuke Niwa]

Created attachment 322781 [details]
Fixes the bug

[Build Bot]

Comment on attachment 322781 [details]
Fixes the bug

Attachment 322781 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4763530

New failing tests:
editing/pasteboard/data-transfer-get-data-on-paste-as-plain-text-when-custom-pasteboard-data-disabled.html

[Build Bot]

Created attachment 322791 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Ryosuke Niwa]

Created attachment 322794 [details]
Fixes builds

[Build Bot]

Comment on attachment 322794 [details]
Fixes builds

Attachment 322794 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4764622

New failing tests:
editing/pasteboard/data-transfer-get-data-on-paste-as-plain-text-when-custom-pasteboard-data-disabled.html

[Build Bot]

Created attachment 322805 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Wenson Hsieh]

Comment on attachment 322794 [details]
Fixes builds

View in context: https://bugs.webkit.org/attachment.cgi?id=322794&action=review

r=me; let's figure out why it's failing EWS.

> LayoutTests/editing/pasteboard/data-transfer-get-data-on-paste-as-plain-text-when-custom-pasteboard-data-disabled.html:11
> +description('Tests that pasting as plain text only exposes "text/plain" in the clipboard.');

It doesn't seem like this description is consistent with the test expectations below.

> LayoutTests/editing/pasteboard/data-transfer-get-data-on-paste-as-plain-text-when-custom-pasteboard-data-disabled.html:13
> +if (!window.internals || !window.internals.settings) {

No braces for single line if statement.

[Wenson Hsieh]

Comment on attachment 322794 [details]
Fixes builds

View in context: https://bugs.webkit.org/attachment.cgi?id=322794&action=review

> LayoutTests/editing/pasteboard/data-transfer-get-data-on-paste-as-plain-text.html:25
> +    if (window.testRunner)

Nit - we don't need this window.testRunner check (unless you meant to use testRunner.execCommand instead?)

[Ryosuke Niwa]

(In reply to Wenson Hsieh from comment #11)
> Comment on attachment 322794 [details]
> Fixes builds
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322794&action=review
> 
> > LayoutTests/editing/pasteboard/data-transfer-get-data-on-paste-as-plain-text.html:25
> > +    if (window.testRunner)
> 
> Nit - we don't need this window.testRunner check (unless you meant to use
> testRunner.execCommand instead?)

Well, I'm doing that because document.execCommand("PasteAndMatchStyle") would be ignored inside the browser.

(In reply to Wenson Hsieh from comment #10)
> Comment on attachment 322794 [details]
> Fixes builds
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322794&action=review
> 
> r=me; let's figure out why it's failing EWS.
> 
> > LayoutTests/editing/pasteboard/data-transfer-get-data-on-paste-as-plain-text-when-custom-pasteboard-data-disabled.html:11
> > +description('Tests that pasting as plain text only exposes "text/plain" in the clipboard.');
> 
> It doesn't seem like this description is consistent with the test
> expectations below.

Fixed.

> > LayoutTests/editing/pasteboard/data-transfer-get-data-on-paste-as-plain-text-when-custom-pasteboard-data-disabled.html:13
> > +if (!window.internals || !window.internals.settings) {
> 
> No braces for single line if statement.

Oops, fixed.

[Wenson Hsieh]

Comment on attachment 322794 [details]
Fixes builds

View in context: https://bugs.webkit.org/attachment.cgi?id=322794&action=review

>>> LayoutTests/editing/pasteboard/data-transfer-get-data-on-paste-as-plain-text.html:25
>>> +    if (window.testRunner)
>> 
>> Nit - we don't need this window.testRunner check (unless you meant to use testRunner.execCommand instead?)
> 
> Well, I'm doing that because document.execCommand("PasteAndMatchStyle") would be ignored inside the browser.
> 
> (In reply to Wenson Hsieh from comment #10)

Right, yea. I was confused because the line of code below this uses `document.execCommand`, not `testRunner.execCommand` (which is what I assumed you meant to use).

[Ryosuke Niwa]

Hm... somehow text/html isn't included in the list of types when internals.settings.setCustomPasteboardDataEnabled(false) is called regardless of whether we do a regular paste or paste as plain text. So this is a regression independent of this code change (likely from r222595).

For now, I'm going to add the following code to addHTMLClipboardTypesForCocoaType but this is a highly indicative or some other bug elsewhere.

    if ([cocoaType isEqualToString:(NSString *)kUTTypeHTML]) {
        resultTypes.add(ASCIILiteral("text/html"));
        return;
    }

[Ryosuke Niwa]

Created attachment 322960 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 322960 [details]
Patch for landing

Wait for EWS.

[Ryosuke Niwa]

Actually, let's not add that code but rather add a failing test expectation on iOS and file a new bug about that.

[Ryosuke Niwa]

(In reply to Ryosuke Niwa from comment #17)
> Actually, let's not add that code but rather add a failing test expectation
> on iOS and file a new bug about that.

Scratch that. Wenson confirmed this is the behavior we had in iOS 11.1.

[Ryosuke Niwa]

Created attachment 322966 [details]
Fixed the test on iOS

[Ryosuke Niwa]

Comment on attachment 322966 [details]
Fixed the test on iOS

cq+ since mac-debug is 35 commits behind.

[WebKit Commit Bot]

Comment on attachment 322966 [details]
Fixed the test on iOS

Clearing flags on attachment: 322966

Committed r222956: <http://trac.webkit.org/changeset/222956>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 322966 [details]
Fixed the test on iOS

View in context: https://bugs.webkit.org/attachment.cgi?id=322966&action=review

> Source/WebCore/ChangeLog:9
> +        When performing Paste and Match Style, only expose the plain text by creating a StaticPasteboard with plain text content.

Are you sure this is right? While we do want to strip styles like bold, we don’t want to strip images, for example. Maybe it’s always been this way?

[Ryosuke Niwa]

(In reply to Darin Adler from comment #23)
> Comment on attachment 322966 [details]
> Fixed the test on iOS
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322966&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        When performing Paste and Match Style, only expose the plain text by creating a StaticPasteboard with plain text content.
> 
> Are you sure this is right? While we do want to strip styles like bold, we
> don’t want to strip images, for example. Maybe it’s always been this way?

That's what I thought as well but Safari/WebKit just paste plain text stripping of images, etc... We may want to re-visit this behavior because some other apps on Mac DOES paste images but we probably need to do that independently of this change.

This bug is about the DOM API which exposes the pasted content, and doesn't affect what gets pasted by WebKit.