WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
194271
Allow pages to trigger programmatic paste from script on iOS
https://bugs.webkit.org/show_bug.cgi?id=194271
Summary
Allow pages to trigger programmatic paste from script on iOS
Wenson Hsieh
Reported
2019-02-04 19:29:38 PST
i.e. using document.execCommand('paste');
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
,
EWS Watchlist
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
Show Obsolete
(12)
View All
Add attachment
proposed patch, testcase, etc.
Wenson Hsieh
Comment 1
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.
Wenson Hsieh
Comment 2
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.
Radar WebKit Bug Importer
Comment 3
2019-02-04 19:34:50 PST
<
rdar://problem/47808810
>
Wenson Hsieh
Comment 4
2019-02-05 10:57:40 PST
Comment hidden (obsolete)
Created
attachment 361202
[details]
WIP only, no tests yet
Wenson Hsieh
Comment 5
2019-02-05 12:58:51 PST
Comment hidden (obsolete)
Created
attachment 361212
[details]
WIP (Fix WPE/GTK/Win builds)
Wenson Hsieh
Comment 6
2019-02-05 17:18:30 PST
Comment hidden (obsolete)
Created
attachment 361252
[details]
WIP (with ChangeLog; tests to come)
Wenson Hsieh
Comment 7
2019-02-06 10:53:17 PST
Comment hidden (obsolete)
Created
attachment 361305
[details]
Part 1
Wenson Hsieh
Comment 8
2019-02-06 10:54:26 PST
Comment hidden (obsolete)
Created
attachment 361306
[details]
Part 2
Wenson Hsieh
Comment 9
2019-02-06 11:11:56 PST
Comment hidden (obsolete)
(In reply to Wenson Hsieh from
comment #8
)
> Created
attachment 361306
[details]
> Part 2
I passed --no-ews when uploading this, and yet it somehow applied and is going through EWS anyways. I would expect two of the new tests I added to fail without the actual source changes in Part 1. So I suppose EWS might be useful after all!
EWS Watchlist
Comment 10
2019-02-06 12:40:21 PST
Comment hidden (obsolete)
Comment on
attachment 361306
[details]
Part 2
Attachment 361306
[details]
did not pass win-ews (win): Output:
https://webkit-queues.webkit.org/results/11055495
New failing tests: editing/pasteboard/ios/programmatic-paste-confirmation.html editing/pasteboard/ios/programmatic-paste-rejection.html
EWS Watchlist
Comment 11
2019-02-06 12:40:33 PST
Comment hidden (obsolete)
Created
attachment 361316
[details]
Archive of layout-test-results from ews200 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Wenson Hsieh
Comment 12
2019-02-06 17:44:17 PST
Comment hidden (obsolete)
Created
attachment 361350
[details]
Part 1
Wenson Hsieh
Comment 13
2019-02-06 17:45:37 PST
Comment hidden (obsolete)
Created
attachment 361351
[details]
Part 2
Ryosuke Niwa
Comment 14
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, }
Wenson Hsieh
Comment 15
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.
Wenson Hsieh
Comment 16
2019-02-07 08:36:12 PST
Comment hidden (obsolete)
Created
attachment 361397
[details]
Part 1
Wenson Hsieh
Comment 17
2019-02-07 08:40:01 PST
Comment hidden (obsolete)
Created
attachment 361398
[details]
Part 1 (v2)
Ryosuke Niwa
Comment 18
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.
Ryosuke Niwa
Comment 19
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.
Wenson Hsieh
Comment 20
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.
Wenson Hsieh
Comment 21
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 😅
Wenson Hsieh
Comment 22
2019-02-08 14:45:09 PST
Comment hidden (obsolete)
Created
attachment 361537
[details]
Part 2 (tests & minor tweaks)
Wenson Hsieh
Comment 23
2019-02-08 15:46:50 PST
Comment hidden (obsolete)
Created
attachment 361547
[details]
Combined patch
Wenson Hsieh
Comment 24
2019-02-08 19:12:14 PST
Created
attachment 361582
[details]
Combined patch (v2)
Wenson Hsieh
Comment 25
2019-02-11 12:26:23 PST
Created
attachment 361702
[details]
Part 2 (tests)
Tim Horton
Comment 26
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
Wenson Hsieh
Comment 27
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:]
Wenson Hsieh
Comment 28
2019-02-12 13:59:42 PST
Created
attachment 361837
[details]
Part 1 (for landing)
WebKit Commit Bot
Comment 29
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
>
Wenson Hsieh
Comment 30
2019-02-12 14:50:49 PST
Created
attachment 361847
[details]
Part 2 (for landing)
WebKit Commit Bot
Comment 31
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
>
Alex Christensen
Comment 32
2019-02-12 17:05:54 PST
http://trac.webkit.org/r241331
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug