Bug 174165 - DataTransfer shouldn't contain text/html when performing Paste and Match Style
Summary: DataTransfer shouldn't contain text/html when performing Paste and Match Style
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: Safari Technology Preview
Hardware: Macintosh macOS 10.12.4
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 124391
  Show dependency treegraph
 
Reported: 2017-07-05 10:59 PDT by Javan Makhmali
Modified: 2017-10-07 14:12 PDT (History)
7 users (show)

See Also:


Attachments
Paste event clipboard data inspector (1.39 KB, text/html)
2017-07-05 10:59 PDT, Javan Makhmali
no flags Details
Fixes the bug (17.56 KB, patch)
2017-10-04 22:13 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (4.01 MB, application/zip)
2017-10-04 23:45 PDT, Build Bot
no flags Details
Fixes builds (18.55 KB, patch)
2017-10-04 23:55 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (4.00 MB, application/zip)
2017-10-05 01:32 PDT, Build Bot
no flags Details
Patch for landing (19.93 KB, patch)
2017-10-05 18:39 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed the test on iOS (19.42 KB, patch)
2017-10-05 19:16 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Javan Makhmali 2017-07-05 10:59:09 PDT
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
Comment 1 Radar WebKit Bug Importer 2017-07-05 11:01:53 PDT
<rdar://problem/33138027>
Comment 2 Wenson Hsieh 2017-09-19 08:03:31 PDT
@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.
Comment 3 Javan Makhmali 2017-09-19 08:19:48 PDT
(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. 💖
Comment 4 Ryosuke Niwa 2017-10-04 22:13:34 PDT
Created attachment 322781 [details]
Fixes the bug
Comment 5 Build Bot 2017-10-04 23:45:47 PDT
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
Comment 6 Build Bot 2017-10-04 23:45:49 PDT
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
Comment 7 Ryosuke Niwa 2017-10-04 23:55:12 PDT
Created attachment 322794 [details]
Fixes builds
Comment 8 Build Bot 2017-10-05 01:32:27 PDT
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
Comment 9 Build Bot 2017-10-05 01:32:28 PDT
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
Comment 10 Wenson Hsieh 2017-10-05 15:21:30 PDT
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.
Comment 11 Wenson Hsieh 2017-10-05 15:26:33 PDT
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?)
Comment 12 Ryosuke Niwa 2017-10-05 18:15:27 PDT
(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.
Comment 13 Wenson Hsieh 2017-10-05 18:36:35 PDT
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).
Comment 14 Ryosuke Niwa 2017-10-05 18:37:18 PDT
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;
    }
Comment 15 Ryosuke Niwa 2017-10-05 18:39:59 PDT
Created attachment 322960 [details]
Patch for landing
Comment 16 Ryosuke Niwa 2017-10-05 18:40:12 PDT
Comment on attachment 322960 [details]
Patch for landing

Wait for EWS.
Comment 17 Ryosuke Niwa 2017-10-05 18:43:14 PDT
Actually, let's not add that code but rather add a failing test expectation on iOS and file a new bug about that.
Comment 18 Ryosuke Niwa 2017-10-05 19:10:37 PDT
(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.
Comment 19 Ryosuke Niwa 2017-10-05 19:16:43 PDT
Created attachment 322966 [details]
Fixed the test on iOS
Comment 20 Ryosuke Niwa 2017-10-05 20:24:40 PDT
Comment on attachment 322966 [details]
Fixed the test on iOS

cq+ since mac-debug is 35 commits behind.
Comment 21 WebKit Commit Bot 2017-10-05 20:51:39 PDT
Comment on attachment 322966 [details]
Fixed the test on iOS

Clearing flags on attachment: 322966

Committed r222956: <http://trac.webkit.org/changeset/222956>
Comment 22 WebKit Commit Bot 2017-10-05 20:51:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Darin Adler 2017-10-07 13:33:02 PDT
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?
Comment 24 Ryosuke Niwa 2017-10-07 14:12:06 PDT
(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.