Skip to content

Conversation

@kcheney1
Copy link
Contributor

@kcheney1 kcheney1 commented Jun 15, 2022

ae412c0

[Trackpad with iPad] Right-clicking on text sometimes doesn't show the correct list of actions
https://bugs.webkit.org/show_bug.cgi?id=241645
rdar://91792562

Reviewed by Devin Rousso.

This patch fixes a race condition in Reveal code where we may not have updated
the selection in UITextInteractionAssistant before calling the
completion handler for the Reveal menu. This results in an empty menu
appearing often during a right-click.

To fix this, this patch converts sendEditorStateUpdate to an async call and
makes sure that call has completed before returning the context menu.

Since this is a race condition, it was a bit tricky to test. This patch
adds a function _simulateSelectionStart which sets up the WKContentView
as if a selection has begun, but it doesn't actually contact the
UITextInteractionAssistant (to avoid flaky false positives). Then the test calls
the context menu code and makes sure that the UITextInteractionAssistant has
been contacted by the time it completes.

* Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivateForTesting.h:
* Source/WebKit/UIProcess/API/Cocoa/WKWebViewTesting.mm:
(-[WKWebView _simulateSelectionStart]):
* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::editorStateChanged):
* Source/WebKit/UIProcess/WebPageProxy.h:
* Source/WebKit/UIProcess/WebPageProxy.messages.in:
* Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:
* Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:
(-[WKContentView _simulateSelectionStart]):
* Source/WebKit/WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::deleteSurrounding):
(WebKit::WebPage::didApplyStyle):
(WebKit::WebPage::didChangeContents):
(WebKit::WebPage::didUpdateComposition):
(WebKit::WebPage::didEndUserTriggeredSelectionChanges):
(WebKit::WebPage::discardedComposition):
(WebKit::WebPage::canceledComposition):
(WebKit::WebPage::sendEditorStateUpdate):
(WebKit::WebPage::flushPendingEditorStateUpdate):
* Source/WebKit/WebProcess/WebPage/WebPage.h:
* Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:
(WebKit::WebPage::prepareSelectionForContextMenuWithLocationInView):
(WebKit::WebPage::requestPositionInformation):
* Tools/TestWebKitAPI/Tests/WebKitCocoa/iOSMouseSupport.mm:
(handleUpdatedSelection):
(TEST):
* Tools/TestWebKitAPI/ios/UIKitSPI.h:

Canonical link: https://commits.webkit.org/251631@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@295626 268f45cc-cd09-0410-ab3c-d52691b4dbfc

@kcheney1 kcheney1 requested a review from JonWBedard as a code owner June 15, 2022 21:26
@kcheney1 kcheney1 self-assigned this Jun 15, 2022
@kcheney1 kcheney1 added WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). WebKit Nightly Build labels Jun 15, 2022
@webkit-early-warning-system webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label Jun 15, 2022
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: Could we put this in WKWebViewTestingIOS.mm (and WKWebViewPrivateForTestingIOS.h) instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: Do we need to convert this to a strong pointer before using it?

RefPtr strongThis = weakThis.get();
if (!strongThis) {
    completionHandler(true, { });
    return;
}
completionHandler(true, { strongThis->revealItemForCurrentSelection(); });

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: CGPointMake(0, 0) -> CGPointZero

@kcheney1 kcheney1 removed merging-blocked Applied to prevent a change from being merged WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). WebKit Nightly Build labels Jun 17, 2022
@kcheney1 kcheney1 added WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). WebKit Nightly Build merge-queue Applied to send a pull request to merge-queue labels Jun 17, 2022
@webkit-early-warning-system webkit-early-warning-system merged commit ae412c0 into WebKit:main Jun 17, 2022
@webkit-early-warning-system
Copy link
Collaborator

Committed r295626 (251631@main): https://commits.webkit.org/251631@main

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

@webkit-early-warning-system webkit-early-warning-system removed the merge-queue Applied to send a pull request to merge-queue label Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants