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
WIP (Fix WPE/GTK/Win builds) (41.09 KB, patch)
2019-02-05 12:58 PST, Wenson Hsieh
no flags
WIP (with ChangeLog; tests to come) (46.96 KB, patch)
2019-02-05 17:18 PST, Wenson Hsieh
no flags
Part 1 (48.12 KB, patch)
2019-02-06 10:53 PST, Wenson Hsieh
no flags
Part 2 (32.78 KB, patch)
2019-02-06 10:54 PST, Wenson Hsieh
no flags
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
Part 1 (48.48 KB, patch)
2019-02-06 17:44 PST, Wenson Hsieh
no flags
Part 2 (33.45 KB, patch)
2019-02-06 17:45 PST, Wenson Hsieh
no flags
Part 1 (50.20 KB, patch)
2019-02-07 08:36 PST, Wenson Hsieh
no flags
Part 1 (v2) (49.30 KB, patch)
2019-02-07 08:40 PST, Wenson Hsieh
no flags
Part 2 (tests & minor tweaks) (57.98 KB, patch)
2019-02-08 14:45 PST, Wenson Hsieh
no flags
Combined patch (105.20 KB, patch)
2019-02-08 15:46 PST, Wenson Hsieh
no flags
Combined patch (v2) (104.68 KB, patch)
2019-02-08 19:12 PST, Wenson Hsieh
no flags
Part 2 (tests) (57.56 KB, patch)
2019-02-11 12:26 PST, Wenson Hsieh
thorton: review+
Part 1 (for landing) (49.90 KB, patch)
2019-02-12 13:59 PST, Wenson Hsieh
no flags
Part 2 (for landing) (56.86 KB, patch)
2019-02-12 14:50 PST, Wenson Hsieh
no flags
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
Wenson Hsieh
Comment 4 2019-02-05 10:57:40 PST Comment hidden (obsolete)
Wenson Hsieh
Comment 5 2019-02-05 12:58:51 PST Comment hidden (obsolete)
Wenson Hsieh
Comment 6 2019-02-05 17:18:30 PST Comment hidden (obsolete)
Wenson Hsieh
Comment 7 2019-02-06 10:53:17 PST Comment hidden (obsolete)
Wenson Hsieh
Comment 8 2019-02-06 10:54:26 PST Comment hidden (obsolete)
Wenson Hsieh
Comment 9 2019-02-06 11:11:56 PST Comment hidden (obsolete)
EWS Watchlist
Comment 10 2019-02-06 12:40:21 PST Comment hidden (obsolete)
EWS Watchlist
Comment 11 2019-02-06 12:40:33 PST Comment hidden (obsolete)
Wenson Hsieh
Comment 12 2019-02-06 17:44:17 PST Comment hidden (obsolete)
Wenson Hsieh
Comment 13 2019-02-06 17:45:37 PST Comment hidden (obsolete)
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)
Wenson Hsieh
Comment 17 2019-02-07 08:40:01 PST Comment hidden (obsolete)
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)
Wenson Hsieh
Comment 23 2019-02-08 15:46:50 PST Comment hidden (obsolete)
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
Note You need to log in before you can comment on or make changes to this bug.