Bug 194271 - Allow pages to trigger programmatic paste from script on iOS
Summary: Allow pages to trigger programmatic paste from script on iOS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-04 19:29 PST by Wenson Hsieh
Modified: 2019-02-12 17:05 PST (History)
10 users (show)

See Also:


Attachments
WIP only, no tests yet (35.48 KB, patch)
2019-02-05 10:57 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
WIP (Fix WPE/GTK/Win builds) (41.09 KB, patch)
2019-02-05 12:58 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
WIP (with ChangeLog; tests to come) (46.96 KB, patch)
2019-02-05 17:18 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Part 1 (48.12 KB, patch)
2019-02-06 10:53 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Part 2 (32.78 KB, patch)
2019-02-06 10:54 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews200 for win-future (12.85 MB, application/zip)
2019-02-06 12:40 PST, Build Bot
no flags Details
Part 1 (48.48 KB, patch)
2019-02-06 17:44 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Part 2 (33.45 KB, patch)
2019-02-06 17:45 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Part 1 (50.20 KB, patch)
2019-02-07 08:36 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Part 1 (v2) (49.30 KB, patch)
2019-02-07 08:40 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Part 2 (tests & minor tweaks) (57.98 KB, patch)
2019-02-08 14:45 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Combined patch (105.20 KB, patch)
2019-02-08 15:46 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Combined patch (v2) (104.68 KB, patch)
2019-02-08 19:12 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Part 2 (tests) (57.56 KB, patch)
2019-02-11 12:26 PST, Wenson Hsieh
thorton: review+
Details | Formatted Diff | Diff
Part 1 (for landing) (49.90 KB, patch)
2019-02-12 13:59 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Part 2 (for landing) (56.86 KB, patch)
2019-02-12 14:50 PST, 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 Wenson Hsieh 2019-02-04 19:29:38 PST
i.e. using document.execCommand('paste');
Comment 1 Wenson Hsieh 2019-02-04 19:32:25 PST
More context: WKWebView (by default) currently doesn't allow pages to trigger programmatic paste as a user privacy measure, such that untrusted web content can't sniff the contents of the user's pasteboard.
Comment 2 Wenson Hsieh 2019-02-04 19:32:56 PST
(In reply to Wenson Hsieh from comment #1)
> More context: WKWebView (by default) …

Of course, this policy extends to Safari as well.
Comment 3 Radar WebKit Bug Importer 2019-02-04 19:34:50 PST
<rdar://problem/47808810>
Comment 4 Wenson Hsieh 2019-02-05 10:57:40 PST Comment hidden (obsolete)
Comment 5 Wenson Hsieh 2019-02-05 12:58:51 PST Comment hidden (obsolete)
Comment 6 Wenson Hsieh 2019-02-05 17:18:30 PST Comment hidden (obsolete)
Comment 7 Wenson Hsieh 2019-02-06 10:53:17 PST Comment hidden (obsolete)
Comment 8 Wenson Hsieh 2019-02-06 10:54:26 PST Comment hidden (obsolete)
Comment 9 Wenson Hsieh 2019-02-06 11:11:56 PST Comment hidden (obsolete)
Comment 10 Build Bot 2019-02-06 12:40:21 PST Comment hidden (obsolete)
Comment 11 Build Bot 2019-02-06 12:40:33 PST Comment hidden (obsolete)
Comment 12 Wenson Hsieh 2019-02-06 17:44:17 PST Comment hidden (obsolete)
Comment 13 Wenson Hsieh 2019-02-06 17:45:37 PST Comment hidden (obsolete)
Comment 14 Ryosuke Niwa 2019-02-06 20:36:37 PST
Comment on attachment 361350 [details]
Part 1

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

> Source/WebCore/dom/Document.cpp:538
> +    , m_programmaticClipboardAccessResetTimer(*this, &Document::programmaticClipboardAccessTimerFired)

Let's not event a new terminology like "programmaticClipboardAccess".
We already have javaScriptCanAccessClipboard / DOMPasteAllowed / clipboardAccessPolicy.

Maybe DOMPasteAccessPolicy?

> Source/WebCore/dom/Document.cpp:5512
> +    if (!m_programmaticClipboardAccessResetTimer.isActive())
> +        m_programmaticClipboardAccessResetTimer.startOneShot(0_s);

This doesn't seem right.
This code would allow any timer or micro tasks scheduled prior to this code to trigger a paste.

> Source/WebCore/editing/EditorCommand.cpp:1245
> +    bool defaultValue = (settings.javaScriptCanAccessClipboard() && settings.DOMPasteAllowed()) || RuntimeEnabledFeatures::sharedFeatures().programmaticClipboardAccessRequestEnabled();

Why isn't this feature simply a setting? It doesn't seem like it needs to be a RuntimeEnabledFeature.

> Source/WebCore/page/Frame.cpp:677
> +    if (m_doc->hasGrantedProgrammaticClipboardAccess())
> +        return true;
> +
> +    if (m_doc->hasDeniedProgrammaticClipboardAccess())
> +        return false;

Instead of having two member functions like this,
why don't we have a function that returns an enum like
DOMPastePolicy {
    Granted,
    Denied,
    Pending,
}
Comment 15 Wenson Hsieh 2019-02-07 07:35:57 PST
Comment on attachment 361350 [details]
Part 1

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

>> Source/WebCore/dom/Document.cpp:538
>> +    , m_programmaticClipboardAccessResetTimer(*this, &Document::programmaticClipboardAccessTimerFired)
> 
> Let's not event a new terminology like "programmaticClipboardAccess".
> We already have javaScriptCanAccessClipboard / DOMPasteAllowed / clipboardAccessPolicy.
> 
> Maybe DOMPasteAccessPolicy?

Sure, DOMPasteAccessPolicy sounds like a good type name to me.

For the verb form of this (i.e. the methods that I've currently named requestProgrammaticClipboardAccess()), what do you think about requestDOMPasteAccess() instead? Or requestAccessToDOMPaste()?

>> Source/WebCore/dom/Document.cpp:5512
>> +        m_programmaticClipboardAccessResetTimer.startOneShot(0_s);
> 
> This doesn't seem right.
> This code would allow any timer or micro tasks scheduled prior to this code to trigger a paste.

Good point! As we discussed, I'll move this to the user gesture token, such that we won't need a timer.

>> Source/WebCore/editing/EditorCommand.cpp:1245
>> +    bool defaultValue = (settings.javaScriptCanAccessClipboard() && settings.DOMPasteAllowed()) || RuntimeEnabledFeatures::sharedFeatures().programmaticClipboardAccessRequestEnabled();
> 
> Why isn't this feature simply a setting? It doesn't seem like it needs to be a RuntimeEnabledFeature.

Ok! Changed to a setting.

>> Source/WebCore/page/Frame.cpp:677
>> +        return false;
> 
> Instead of having two member functions like this,
> why don't we have a function that returns an enum like
> DOMPastePolicy {
>     Granted,
>     Denied,
>     Pending,
> }

Sure! I was debating between exposing two getters and keeping the enum type private, or exposing a single getter with the enum type. I'll change it to do the latter.
Comment 16 Wenson Hsieh 2019-02-07 08:36:12 PST Comment hidden (obsolete)
Comment 17 Wenson Hsieh 2019-02-07 08:40:01 PST Comment hidden (obsolete)
Comment 18 Ryosuke Niwa 2019-02-07 19:14:31 PST
Comment on attachment 361398 [details]
Part 1 (v2)

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

> Source/WebCore/page/Frame.cpp:679
> +    case DOMPasteAccessPolicy::Pending: {

It's odd that this state is called pending because there was no pending request.
How about something like DOMPasteAccessPolicy::NotRequestedYet or DOMPasteAccessPolicy::None?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4866
> +    const CGFloat maximumElementWidth = 300;
> +    const CGFloat maximumElementHeight = 120;
> +    if (elementRect.isEmpty() || elementRect.width() > maximumElementWidth || elementRect.height() > maximumElementHeight) {
> +        const CGFloat interactionLocationMargin = 10;

These values seems somewhat arbitrary. Why are these values picked?
We should probably explain it in a comment or in the change log.
Comment 19 Ryosuke Niwa 2019-02-07 19:16:07 PST
Comment on attachment 361398 [details]
Part 1 (v2)

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

> Source/WebCore/page/Frame.cpp:673
> +    auto gestureToken = UserGestureIndicator::currentUserGesture();

Maybe you should just check the nullity of this instead of checking UserGestureIndicator::processingUserGesture() earlier.
Comment 20 Wenson Hsieh 2019-02-07 19:23:48 PST
Comment on attachment 361398 [details]
Part 1 (v2)

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

>> Source/WebCore/page/Frame.cpp:673
>> +    auto gestureToken = UserGestureIndicator::currentUserGesture();
> 
> Maybe you should just check the nullity of this instead of checking UserGestureIndicator::processingUserGesture() earlier.

So UserGestureIndicator::processingUserGesture actually checks a tiny bit more than a null check:

    bool UserGestureIndicator::processingUserGesture()
    {
        if (!isMainThread())
            return false;

        return currentToken() ? currentToken()->processingUserGesture() : false;
    }

I think we can assume we're always on the main thread here (since Much Worse Things™️ would be happening if we were on a background thread while evaluating script that calls execCommand on document). However, there's still the check for processingUserGesture() on the token. I can change this to something like:

    auto gestureToken = UserGestureIndicator::currentUserGesture();
    if (!gestureToken->processingUserGesture())
        return false;

...instead.

>> Source/WebCore/page/Frame.cpp:679
>> +    case DOMPasteAccessPolicy::Pending: {
> 
> It's odd that this state is called pending because there was no pending request.
> How about something like DOMPasteAccessPolicy::NotRequestedYet or DOMPasteAccessPolicy::None?

Sounds good — I quite like DOMPasteAccessPolicy::NotRequestedYet.

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4866
>> +        const CGFloat interactionLocationMargin = 10;
> 
> These values seems somewhat arbitrary. Why are these values picked?
> We should probably explain it in a comment or in the change log.

It's true that these are arbitrary values — I picked these after trying this new behavior out on a few relevant websites. The idea is that if the element that the user has interacted with is small enough, the callout looks quite natural if we just use the element rect as the menu's target rect; but if the element is too large, it makes more sense to fall back to using the touch location (with a small amount of margin, so that the callout action isn't obscured by the user's touch).

I'll elaborate on this in the ChangeLog.
Comment 21 Wenson Hsieh 2019-02-07 19:41:49 PST
>     auto gestureToken = UserGestureIndicator::currentUserGesture();
>     if (!gestureToken->processingUserGesture())
>         return false;

...of course, I also meant to check !gestureToken here as well 😅
Comment 22 Wenson Hsieh 2019-02-08 14:45:09 PST Comment hidden (obsolete)
Comment 23 Wenson Hsieh 2019-02-08 15:46:50 PST Comment hidden (obsolete)
Comment 24 Wenson Hsieh 2019-02-08 19:12:14 PST
Created attachment 361582 [details]
Combined patch (v2)
Comment 25 Wenson Hsieh 2019-02-11 12:26:23 PST
Created attachment 361702 [details]
Part 2 (tests)
Comment 26 Tim Horton 2019-02-12 13:26:38 PST
Comment on attachment 361702 [details]
Part 2 (tests)

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

> Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm:859
> +            if (CFStringCompare((__bridge CFStringRef)buttonTitle, action.get(), 0) != kCFCompareEqualTo)

Why no ObjC
Comment 27 Wenson Hsieh 2019-02-12 13:37:22 PST
Comment on attachment 361702 [details]
Part 2 (tests)

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

>> Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm:859
>> +            if (CFStringCompare((__bridge CFStringRef)buttonTitle, action.get(), 0) != kCFCompareEqualTo)
> 
> Why no ObjC

👍 changed to -[NSString isEqualToString:]
Comment 28 Wenson Hsieh 2019-02-12 13:59:42 PST
Created attachment 361837 [details]
Part 1 (for landing)
Comment 29 WebKit Commit Bot 2019-02-12 14:37:54 PST
Comment on attachment 361837 [details]
Part 1 (for landing)

Clearing flags on attachment: 361837

Committed r241320: <https://trac.webkit.org/changeset/241320>
Comment 30 Wenson Hsieh 2019-02-12 14:50:49 PST
Created attachment 361847 [details]
Part 2 (for landing)
Comment 31 WebKit Commit Bot 2019-02-12 15:18:36 PST
Comment on attachment 361847 [details]
Part 2 (for landing)

Clearing flags on attachment: 361847

Committed r241322: <https://trac.webkit.org/changeset/241322>
Comment 32 Alex Christensen 2019-02-12 17:05:54 PST
http://trac.webkit.org/r241331