[webkit-changes] [WebKit/WebKit] d34e00: REGRESSION (254359 at main): Yellow highlight for loo...

[Wenson Hsieh]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: d34e00e6207ce86589cf77dd2f91a1d905fddd3f
      https://github.com/WebKit/WebKit/commit/d34e00e6207ce86589cf77dd2f91a1d905fddd3f
  Author: Wenson Hsieh <wenson_hsieh at apple.com>
  Date:   2022-10-17 (Mon, 17 Oct 2022)

  Changed paths:
    M Source/WebCore/PlatformMac.cmake
    M Source/WebCore/SourcesCocoa.txt
    M Source/WebCore/WebCore.xcodeproj/project.pbxproj
    M Source/WebCore/editing/cocoa/DictionaryLookup.mm
    M Source/WebCore/editing/mac/DictionaryLookup.h
    A Source/WebCore/platform/mac/RevealUtilities.h
    A Source/WebCore/platform/mac/RevealUtilities.mm
    M Source/WebKit/Platform/mac/MenuUtilities.mm
    M Tools/TestWebKitAPI/Configurations/TestWebKitAPI.xcconfig
    M Tools/TestWebKitAPI/Tests/mac/ImmediateActionTests.mm

  Log Message:
  -----------
  REGRESSION (254359 at main): Yellow highlight for lookup doesn't show text
https://bugs.webkit.org/show_bug.cgi?id=246586
rdar://100828328

Reviewed by Aditya Keerthi.

After 254359 at main, we no longer have a retain cycle between the `WebRevealHighlight` and the
`_clearTextIndicator` callback block data. However, this means that nothing currently holds on to
`WebRevealHighlight` after we return from `showPopupOrCreateAnimationController`, which causes the
text indicator snapshot and data to not show up when looking up a word (e.g. via force click).

To fix this without reintroducing the retain cycle, we instead tie the lifetime of the
`WebRevealHighlight` to the `RVPresentingContext` itself, which only has a weak pointer back to the
reveal highlight (via `-highlightDelegate`). This ensures that the highlight information will never
prematurely disappear from underneath the presenting context.

Note that this technique is already used in `MenuUtilities.mm` inside `menuForTelephoneNumber`,
which handles context menu presentation when force clicking on a data detected phone number. To
avoid repeating this code in both places, we move it into a new helper function in `RevealUtilities`
and call it from both places.

See below for more details.

Test: ImmediateActionTests.ImmediateActionOverText

* Source/WebCore/PlatformMac.cmake:
* Source/WebCore/SourcesCocoa.txt:
* Source/WebCore/WebCore.xcodeproj/project.pbxproj:
* Source/WebCore/editing/cocoa/DictionaryLookup.mm:
(-[WebRevealHighlight initWithHighlightRect:useDefaultHighlight:attributedString:clearTextIndicatorCallback:]):

Drive-by refactoring: remove `setClearTextIndicator:`, and just pass it in when creating the reveal
highlight. Currently, this method is only called right after initialization anyways.

(WebCore::showPopupOrCreateAnimationController):

Use the new `createRVPresentingContextWithRetainedDelegate` method to create the presenting context.

(-[WebRevealHighlight initWithHighlightRect:useDefaultHighlight:attributedString:]): Deleted.
(-[WebRevealHighlight setClearTextIndicator:]): Deleted.
* Source/WebCore/editing/mac/DictionaryLookup.h:
* Source/WebCore/platform/mac/RevealUtilities.h: Added.
* Source/WebCore/platform/mac/RevealUtilities.mm: Added.

Add a new file that contains a helper method (`createRVPresentingContextWithRetainedDelegate`) that
allocates a new `RVPresentingContext` with the given parameters, and ties the lifetime of the
delegate to the context.

(WebCore::createRVPresentingContextWithRetainedDelegate):
* Source/WebKit/Platform/mac/MenuUtilities.mm:
(WebKit::menuForTelephoneNumber):

Use the new helper method here, too.

* Tools/TestWebKitAPI/Configurations/TestWebKitAPI.xcconfig:

Additionally link against Reveal in TestWebKitAPI on macOS, so that it's simpler to intercept and
replace the call to `-initWithPointerLocationInView:inView:highlightDelegate:`.

* Tools/TestWebKitAPI/Tests/mac/ImmediateActionTests.mm:
(-[NSObject swizzled_initWithPointerLocationInView:inView:highlightDelegate:]):
(TestWebKitAPI::swizzlePresentingContextInitialization):

Augment a test to verify that the `RVPresentingContext`'s highlight delegate doesn't vanish right
after triggering Look Up via a force click.

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