Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop overriding -[UITextInputPrivate supportsImagePaste] on WKContentView #20712

Merged
merged 1 commit into from
Nov 19, 2023

Conversation

whsieh
Copy link
Member

@whsieh whsieh commented Nov 19, 2023

c152bf5

Stop overriding `-[UITextInputPrivate supportsImagePaste]` on `WKContentView`
https://bugs.webkit.org/show_bug.cgi?id=265094
rdar://118430889

Reviewed by Tim Horton.

Stop implementing the SPI method `-supportsImagePaste`, which is used only to prevent UIKit from
bumping the `changeCount` from underneath us when showing the keyboard for a focused element by
short-circuiting the logic in `-[UIKeyboardImpl uncachedDelegateSupportsImagePaste]` (i.e. before
attempting to pin a 1x1 PNG image to the pasteboard, which is intended to check whether or not the
Emoji sticker section in the keyboard should be shown). If implemented on the responder, UIKit only
consults this SPI; since we currently make this return `YES` whenever the focused element is
"selectable" (i.e., contains editable/selectable text) and this delegate method is only consulted
when the keyboard is shown, this basically has the effect of always returning `YES` in the case of
WebKit.

To move off of this SPI, we can instead set a `UIPasteConfiguration` on the content view, which
achieves the same effect by allowing us to specify a list of uniform type identifiers that we can
support, for the purposes of paste handling. Note that the presence of a UTI in this list doesn't
necessarily mean that we'll unconditionally allow `@selector(paste:)` for that UTI, since UIKit
still consults our more granular logic in `-canPerformAction:withSender:` before showing any paste
UI. This only allows UIKit to short-circuit certain codepaths (such as the one above) when checking
whether or not to allow pastes, in the case where the paste configuration indicates that a type is
*not* supported. As such, it's safe to include more than we'd strictly allow in our
`UIPasteConfiguration`.

Test: fast/forms/ios/focusing-text-field-does-not-increase-change-count.html

* LayoutTests/fast/forms/ios/focusing-text-field-does-not-increase-change-count-expected.txt: Added.
* LayoutTests/fast/forms/ios/focusing-text-field-does-not-increase-change-count.html: Added.

Add a new layout test to exercise this change, by ensuring that the pasteboard's `changeCount` isn't
erroneously bumped when focusing any text field. This replaces the API test below, which is no
longer relevant since we no longer implement `-supportsImagePaste`.

* LayoutTests/resources/ui-helper.js:

Add a script controller hook to ask for the current pasteboard change count, and implement this hook
on all Cocoa platforms.

(window.UIHelper.async pasteboardChangeCount):
* Source/WTF/wtf/PlatformHave.h:

Add a compile-time flag to guard the availability of `UIPasteConfiguration`.

* Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:

Refactor supported pasteboard types into a few helper functions, (some of) which are now used in
multiple places below.

(-[WKContentView setUpInteraction]):
(-[WKContentView cleanUpInteraction]):
(-[WKContentView supportedPasteboardTypesForCurrentSelection]):
(-[WKContentView canPerformActionForWebView:withSender:]):

Use the new helper functions above, and also refactor an existing "attachment element enabled" check
to consult `WebPreferences` instead of the `WKWebViewConfiguration` (the latter of which triggers an
unnecessary copy of the whole web view configuration object on the `WKWebView`).

(-[WKContentView _elementDidFocus:userIsInteracting:blurPreviousNode:activityStateChanges:userObject:]):
(-[WKContentView supportsImagePaste]): Deleted.

Remove this SPI method implementation.

* Tools/DumpRenderTree/ios/UIScriptControllerIOS.h:
* Tools/DumpRenderTree/ios/UIScriptControllerIOS.mm:
(WTR::UIScriptControllerIOS::pasteboardChangeCount const):
* Tools/DumpRenderTree/mac/UIScriptControllerMac.h:
* Tools/DumpRenderTree/mac/UIScriptControllerMac.mm:
(WTR::UIScriptControllerMac::pasteboardChangeCount const):
* Tools/TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl:
* Tools/TestRunnerShared/UIScriptContext/UIScriptController.h:
(WTR::UIScriptController::pasteboardChangeCount const):
* Tools/TestRunnerShared/spi/UIKitSPIForTesting.h:
* Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:

Delete an existing API test, which has now been converted into a new layout test (see above).

* Tools/WebKitTestRunner/ios/UIScriptControllerIOS.h:
* Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm:
(WTR::UIScriptControllerIOS::pasteboardChangeCount const):
* Tools/WebKitTestRunner/mac/UIScriptControllerMac.h:
* Tools/WebKitTestRunner/mac/UIScriptControllerMac.mm:
(WTR::UIScriptControllerMac::pasteboardChangeCount const):

Canonical link: https://commits.webkit.org/270947@main

f0e6539

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ§ͺ api-wpe
βœ… πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1 βœ… πŸ›  gtk
βœ… πŸ›  πŸ§ͺ jsc βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ§ͺ gtk-wk2
βœ… πŸ›  πŸ§ͺ jsc-arm64 βœ… πŸ›  tv βœ… πŸ§ͺ mac-AS-debug-wk2 βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-wk2-stress βœ… πŸ›  jsc-armv7
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch βœ… πŸ§ͺ jsc-armv7-tests
βœ… πŸ›  watch-sim

@whsieh whsieh self-assigned this Nov 19, 2023
@whsieh whsieh added the HTML Editing For bugs in HTML editing support (including designMode and contentEditable). label Nov 19, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 19, 2023
@whsieh whsieh removed the merging-blocked Applied to prevent a change from being merged label Nov 19, 2023
@whsieh whsieh added the merge-queue Applied to send a pull request to merge-queue label Nov 19, 2023
@whsieh
Copy link
Member Author

whsieh commented Nov 19, 2023

Thanks for the review!

…entView`

https://bugs.webkit.org/show_bug.cgi?id=265094
rdar://118430889

Reviewed by Tim Horton.

Stop implementing the SPI method `-supportsImagePaste`, which is used only to prevent UIKit from
bumping the `changeCount` from underneath us when showing the keyboard for a focused element by
short-circuiting the logic in `-[UIKeyboardImpl uncachedDelegateSupportsImagePaste]` (i.e. before
attempting to pin a 1x1 PNG image to the pasteboard, which is intended to check whether or not the
Emoji sticker section in the keyboard should be shown). If implemented on the responder, UIKit only
consults this SPI; since we currently make this return `YES` whenever the focused element is
"selectable" (i.e., contains editable/selectable text) and this delegate method is only consulted
when the keyboard is shown, this basically has the effect of always returning `YES` in the case of
WebKit.

To move off of this SPI, we can instead set a `UIPasteConfiguration` on the content view, which
achieves the same effect by allowing us to specify a list of uniform type identifiers that we can
support, for the purposes of paste handling. Note that the presence of a UTI in this list doesn't
necessarily mean that we'll unconditionally allow `@selector(paste:)` for that UTI, since UIKit
still consults our more granular logic in `-canPerformAction:withSender:` before showing any paste
UI. This only allows UIKit to short-circuit certain codepaths (such as the one above) when checking
whether or not to allow pastes, in the case where the paste configuration indicates that a type is
*not* supported. As such, it's safe to include more than we'd strictly allow in our
`UIPasteConfiguration`.

Test: fast/forms/ios/focusing-text-field-does-not-increase-change-count.html

* LayoutTests/fast/forms/ios/focusing-text-field-does-not-increase-change-count-expected.txt: Added.
* LayoutTests/fast/forms/ios/focusing-text-field-does-not-increase-change-count.html: Added.

Add a new layout test to exercise this change, by ensuring that the pasteboard's `changeCount` isn't
erroneously bumped when focusing any text field. This replaces the API test below, which is no
longer relevant since we no longer implement `-supportsImagePaste`.

* LayoutTests/resources/ui-helper.js:

Add a script controller hook to ask for the current pasteboard change count, and implement this hook
on all Cocoa platforms.

(window.UIHelper.async pasteboardChangeCount):
* Source/WTF/wtf/PlatformHave.h:

Add a compile-time flag to guard the availability of `UIPasteConfiguration`.

* Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:

Refactor supported pasteboard types into a few helper functions, (some of) which are now used in
multiple places below.

(-[WKContentView setUpInteraction]):
(-[WKContentView cleanUpInteraction]):
(-[WKContentView supportedPasteboardTypesForCurrentSelection]):
(-[WKContentView canPerformActionForWebView:withSender:]):

Use the new helper functions above, and also refactor an existing "attachment element enabled" check
to consult `WebPreferences` instead of the `WKWebViewConfiguration` (the latter of which triggers an
unnecessary copy of the whole web view configuration object on the `WKWebView`).

(-[WKContentView _elementDidFocus:userIsInteracting:blurPreviousNode:activityStateChanges:userObject:]):
(-[WKContentView supportsImagePaste]): Deleted.

Remove this SPI method implementation.

* Tools/DumpRenderTree/ios/UIScriptControllerIOS.h:
* Tools/DumpRenderTree/ios/UIScriptControllerIOS.mm:
(WTR::UIScriptControllerIOS::pasteboardChangeCount const):
* Tools/DumpRenderTree/mac/UIScriptControllerMac.h:
* Tools/DumpRenderTree/mac/UIScriptControllerMac.mm:
(WTR::UIScriptControllerMac::pasteboardChangeCount const):
* Tools/TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl:
* Tools/TestRunnerShared/UIScriptContext/UIScriptController.h:
(WTR::UIScriptController::pasteboardChangeCount const):
* Tools/TestRunnerShared/spi/UIKitSPIForTesting.h:
* Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:

Delete an existing API test, which has now been converted into a new layout test (see above).

* Tools/WebKitTestRunner/ios/UIScriptControllerIOS.h:
* Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm:
(WTR::UIScriptControllerIOS::pasteboardChangeCount const):
* Tools/WebKitTestRunner/mac/UIScriptControllerMac.h:
* Tools/WebKitTestRunner/mac/UIScriptControllerMac.mm:
(WTR::UIScriptControllerMac::pasteboardChangeCount const):

Canonical link: https://commits.webkit.org/270947@main
@webkit-commit-queue
Copy link
Collaborator

Committed 270947@main (c152bf5): https://commits.webkit.org/270947@main

Reviewed commits have been landed. Closing PR #20712 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit c152bf5 into WebKit:main Nov 19, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Nov 19, 2023
@whsieh whsieh deleted the eng/265094 branch November 19, 2023 04:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HTML Editing For bugs in HTML editing support (including designMode and contentEditable).
Projects
None yet
6 participants