i.e. using document.execCommand('paste');
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.
(In reply to Wenson Hsieh from comment #1) > More context: WKWebView (by default) … Of course, this policy extends to Safari as well.
<rdar://problem/47808810>
Created attachment 361202 [details] WIP only, no tests yet
Created attachment 361212 [details] WIP (Fix WPE/GTK/Win builds)
Created attachment 361252 [details] WIP (with ChangeLog; tests to come)
Created attachment 361305 [details] Part 1
Created attachment 361306 [details] Part 2
(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!
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
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
Created attachment 361350 [details] Part 1
Created attachment 361351 [details] Part 2
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 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.
Created attachment 361397 [details] Part 1
Created attachment 361398 [details] Part 1 (v2)
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 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 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.
> auto gestureToken = UserGestureIndicator::currentUserGesture(); > if (!gestureToken->processingUserGesture()) > return false; ...of course, I also meant to check !gestureToken here as well 😅
Created attachment 361537 [details] Part 2 (tests & minor tweaks)
Created attachment 361547 [details] Combined patch
Created attachment 361582 [details] Combined patch (v2)
Created attachment 361702 [details] Part 2 (tests)
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 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:]
Created attachment 361837 [details] Part 1 (for landing)
Comment on attachment 361837 [details] Part 1 (for landing) Clearing flags on attachment: 361837 Committed r241320: <https://trac.webkit.org/changeset/241320>
Created attachment 361847 [details] Part 2 (for landing)
Comment on attachment 361847 [details] Part 2 (for landing) Clearing flags on attachment: 361847 Committed r241322: <https://trac.webkit.org/changeset/241322>
http://trac.webkit.org/r241331