Bug 184271 - [iOS] Paste is missing from callout bar when pasteboard only contains custom data
Summary: [iOS] Paste is missing from callout bar when pasteboard only contains custom ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: Safari 11
Hardware: iPhone / iPad iOS 11
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-04-03 10:51 PDT by Chet Corcos
Modified: 2018-09-30 17:09 PDT (History)
17 users (show)

See Also:


Attachments
Patch (20.08 KB, patch)
2018-08-18 01:59 PDT, Wenson Hsieh
rniwa: review+
Details | Formatted Diff | Diff
Patch for landing (19.99 KB, patch)
2018-08-18 15:19 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
API test fix (1.84 KB, patch)
2018-08-20 08:35 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chet Corcos 2018-04-03 10:51:33 PDT
It appears that in iOS, only a limited set of pasteboard types are allowed. See the supportedWebContentPasteboardTypes method inside https://opensource.apple.com/source/WebCore/WebCore-7604.1.38.0.7/platform/ios/PasteboardIOS.mm.auto.html

Whereas on desktop, you can create whatever pasteboard types that you want.

This is very important in rich text applications that rely on copy-paste of structured content that needs to be treated differently when pasting inside the application vs outside the application.

I've created a simple demonstration of this in a CodePen which you can open in iOS Safari and I've copied the source code below: https://codepen.io/ccorcos/pen/KooVZB?editors=1010

When you create a selection in the contenteditable input and select copy, we save some structured information in a custom pasteboard type, and when we paste, we parse it back out. This works fine in desktop Safari, but does not work on iOS.

```html

<div id="input" contenteditable>Hello world</div>

```


```js

const input = document.getElementById("input")

window.addEventListener("copy", event => {
  event.preventDefault()
  const data = JSON.stringify({data: input.textContent})
  console.log("copy", data)
  event.clipboardData.setData("app/custom-key", data)
})

window.addEventListener("paste", event => {
  event.preventDefault()
  console.log(event)
  const data = event.clipboardData.getData("app/custom-key")
  console.log("pasteData", data)
  if (!data) {
    console.log("no data")
    return
  }
  const result = JSON.parse(data)
  input.innerHTML = result.data
})

```
Comment 1 Radar WebKit Bug Importer 2018-04-06 21:01:41 PDT
<rdar://problem/39256708>
Comment 2 Ryosuke Niwa 2018-04-06 21:03:32 PDT
Is this on iOS 11.0? Have you tried it on iOS 11.3? We just made a lot of improvements on iOS 11.3.
Comment 3 Wenson Hsieh 2018-04-06 23:00:16 PDT
Thanks for the report, Chet! Responses inline.

(In reply to Chet Corcos from comment #0)
> It appears that in iOS, only a limited set of pasteboard types are allowed.
> See the supportedWebContentPasteboardTypes method inside
> https://opensource.apple.com/source/WebCore/WebCore-7604.1.38.0.7/platform/
> ios/PasteboardIOS.mm.auto.html

So this is the special list of types that WebKit provides out-of-the-box support for pasting or dropping in editable web content. Custom pasteboard data types (setData(), getData() with types that are not "text/plain", "text/uri-list" and "text/html") take a totally different codepath than this one (search for "customPasteboardData")

Could you try copying the "3 Custom Copy Types" text on https://whsieh.github.io/examples/custom-pasteboard-data and pasting in the gray dashed area below?

> 
> Whereas on desktop, you can create whatever pasteboard types that you want.
> 
> This is very important in rich text applications that rely on copy-paste of
> structured content that needs to be treated differently when pasting inside
> the application vs outside the application.
> 
> I've created a simple demonstration of this in a CodePen which you can open
> in iOS Safari and I've copied the source code below:
> https://codepen.io/ccorcos/pen/KooVZB?editors=1010
> 
> When you create a selection in the contenteditable input and select copy, we
> save some structured information in a custom pasteboard type, and when we
> paste, we parse it back out. This works fine in desktop Safari, but does not
> work on iOS.
> 
> ```html
> 
> <div id="input" contenteditable>Hello world</div>
> 
> ```
> 
> 
> ```js
> 
> const input = document.getElementById("input")
> 
> window.addEventListener("copy", event => {
>   event.preventDefault()
>   const data = JSON.stringify({data: input.textContent})
>   console.log("copy", data)
>   event.clipboardData.setData("app/custom-key", data)
> })
> 
> window.addEventListener("paste", event => {
>   event.preventDefault()
>   console.log(event)
>   const data = event.clipboardData.getData("app/custom-key")
>   console.log("pasteData", data)
>   if (!data) {
>     console.log("no data")
>     return
>   }
>   const result = JSON.parse(data)
>   input.innerHTML = result.data
> })
> 
> ```

Hm...I just tried this on a recent version of iOS, and cannot paste. I think we may have a bug where the callout action bar doesn't give the user the option to paste if there are only custom types on the pasteboard. If you are testing on iOS 11.3, could you try setting "text/plain" to something as well, and let me know if the problem still reproduces?
Comment 4 Chet Corcos 2018-04-10 16:11:50 PDT
> Is this on iOS 11.0? Have you tried it on iOS 11.3? We just made a lot of improvements on iOS 11.3.

Yes. iOS 11.3


> Hm...I just tried this on a recent version of iOS, and cannot paste. I think we may have a bug where the callout action bar doesn't give the user the option to paste if there are only custom types on the pasteboard. If you are testing on iOS 11.3, could you try setting "text/plain" to something as well, and let me know if the problem still reproduces?

Yes. When I copy "text/plain" in addition to custom keys, the custom keys are not copied to the clipboard.
Comment 5 Chet Corcos 2018-05-01 12:14:12 PDT
Any update on this?
Comment 6 Ryosuke Niwa 2018-08-15 20:12:17 PDT
We still cannot reproduce what you're reporting. We see that the callout / popover doesn't list "paste" as an option. Is that the bug you're reporting?
Comment 7 Chet Corcos 2018-08-16 11:30:45 PDT
Hey @Ryosuke

Have you tried the attached codepen link? The callout / popover does not appear with a paste option because the clipboard data was never copied. If you look at the logs, you will see that the custom keys are never added to the clipboard.

It has been over 4 months since reporting this bug. If you actually plan on fixing this as opposed to kicking the can down the road, feel free to give me a call and I will walk you through everything. 916 548 9415

Cheers,

Chet
Comment 8 Ryosuke Niwa 2018-08-16 12:39:01 PDT
(In reply to Chet Corcos from comment #7)
> Have you tried the attached codepen link? The callout / popover does not
> appear with a paste option because the clipboard data was never copied. If
> you look at the logs, you will see that the custom keys are never added to
> the clipboard.

Right, the callout doesn't show up but the data is copied. In fact, I can paste it in macOS on the same page using Universal Clipboard (ability to copy content in iOS and paste it in macOS) and it shows that the custom data is there.

If the only issue here is that the callout doesn't show up, that's definitely a bug we can reproduce and we would track that.
Comment 9 Chet Corcos 2018-08-16 19:09:02 PDT
Interesting. You're right! Just verified here... If you copy some plain text in there then it will show the callout.

https://codepen.io/anon/pen/VBJJXX?editors=1010#anon-login

So the callout thing is still a bug, but the original bug I reported here appears half-way fixed.
Comment 10 Wenson Hsieh 2018-08-17 12:23:03 PDT
I think we should probably allow the -paste: action if the pasteboard contains custom pasteboard data with an origin matching the origin of the currently focused frame's document.
Comment 11 Wenson Hsieh 2018-08-18 01:59:32 PDT
Created attachment 347440 [details]
Patch
Comment 12 Ryosuke Niwa 2018-08-18 14:06:54 PDT
Comment on attachment 347440 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:2279
> +    auto& editorState = _page->editorState();

It looks like editorState() returns a new object each time?
In that case, I don't think it makes much sense to use auto&. Just use auto.

Also, if it were really returning a reference,
I'm afraid that the object could get destroyed by some of the code below.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteMixedContent.mm:260
> +TEST(PasteMixedContent, CopyAndPasteWithCustomPasteboardDataOnly)

I would have preferred to write a layout test for something like this but okay.
Comment 13 Wenson Hsieh 2018-08-18 14:38:31 PDT
Comment on attachment 347440 [details]
Patch

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

Thanks for the review!

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:2279
>> +    auto& editorState = _page->editorState();
> 
> It looks like editorState() returns a new object each time?
> In that case, I don't think it makes much sense to use auto&. Just use auto.
> 
> Also, if it were really returning a reference,
> I'm afraid that the object could get destroyed by some of the code below.

Good point! Changed to just `auto`.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteMixedContent.mm:260
>> +TEST(PasteMixedContent, CopyAndPasteWithCustomPasteboardDataOnly)
> 
> I would have preferred to write a layout test for something like this but okay.

Yeah, I had considered this too. I went with an API test in the end because the root cause of this bug is that WebKit API (namely, -[WKWebView canPerformAction:withSender:]) is returning the wrong result, which causes the UICalloutBar to not show "Paste" as an option. In this case, it was a little bit simpler to just test the API directly rather than route through UIScriptController for a layout test.
Comment 14 Wenson Hsieh 2018-08-18 15:19:49 PDT
Created attachment 347452 [details]
Patch for landing
Comment 15 WebKit Commit Bot 2018-08-18 15:59:28 PDT
Comment on attachment 347452 [details]
Patch for landing

Clearing flags on attachment: 347452

Committed r235011: <https://trac.webkit.org/changeset/235011>
Comment 16 Alexey Proskuryakov 2018-08-18 16:35:25 PDT
Comment on attachment 347452 [details]
Patch for landing

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

> Source/WebKit/ChangeLog:23
> +        * Shared/EditorState.h:
> +
> +        Add a originIdentifierForPasteboard field, and add support for encoding it when propagating EditorState via IPC.

Like everything in UI process, origin cached in EditorState can get out of sync with WebContent. What is the worst problem that can happen in this situation?
Comment 17 Wenson Hsieh 2018-08-18 17:29:54 PDT
(In reply to Alexey Proskuryakov from comment #16)
> Comment on attachment 347452 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=347452&action=review
> 
> > Source/WebKit/ChangeLog:23
> > +        * Shared/EditorState.h:
> > +
> > +        Add a originIdentifierForPasteboard field, and add support for encoding it when propagating EditorState via IPC.
> 
> Like everything in UI process, origin cached in EditorState can get out of
> sync with WebContent. What is the worst problem that can happen in this
> situation?

The worst that could happen if the EditorState's cached origin is out of sync is that we might show an option to "paste" when we shouldn't, but only when the pasteboard contains only custom data (the opposite is also possible, although before this fix, we'd simply never show the option to "paste" anyways).

If the user were to try and hit "Paste" in this scenario, we would not allow the page to read cross-origin custom pasteboard data, because after we call out to the web process to handle the paste, DataTransfer would check the up-to-date origin identifier against the contents of the custom pasteboard data and bail as needed.
Comment 18 Dawei Fenton (:realdawei) 2018-08-20 07:59:41 PDT
(In reply to WebKit Commit Bot from comment #15)
> Comment on attachment 347452 [details]
> Patch for landing
> 
> Clearing flags on attachment: 347452
> 
> Committed r235011: <https://trac.webkit.org/changeset/235011>

Seeing API failures on Mac after this revision

sample output:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/6244/steps/run-api-tests/logs/stdio

Failed

    TestWebKitAPI.PasteMixedContent.CopyAndPasteWithCustomPasteboardDataOnly
        
        /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteMixedContent.mm:285
        Value of: [destination stringByEvaluatingJavaScript:@"document.querySelector('input').value"]
          Actual: "bar"
        Expected: ""
Comment 19 Wenson Hsieh 2018-08-20 08:35:35 PDT
Created attachment 347498 [details]
API test fix
Comment 20 WebKit Commit Bot 2018-08-20 09:43:36 PDT
Comment on attachment 347498 [details]
API test fix

Clearing flags on attachment: 347498

Committed r235079: <https://trac.webkit.org/changeset/235079>