Bug 224204 – [iOS] contextmenu hints can be clipped by the WKWebView

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Devin Rousso]

if an app has a `WKWebView` that is smaller than the entire screen, contextmenu previews can be clipped if they try to display towards the edge of the `WKWebView`

[Devin Rousso]

<rdar://problem/75504620>

[Devin Rousso]

Created attachment 425208 [details]
Patch

[Devin Rousso]

Created attachment 425211 [details]
Patch

[Tim Horton]

Comment on attachment 425211 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425211&action=review

Does not seem outrageous (though it sure would be nice if the UIKit SPI worked).

> Source/WebKit/ChangeLog:3
> +        [iOS] contextmenu previews can be clipped by the WKWebView

please use the word "hint" everywhere you use "preview"... "preview" is the VC that comes up adjacent to the context menu actions with a web view in it previewing the link destination

> Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteScrollingCoordinatorProxyIOS.mm:57
>  UIScrollView *RemoteScrollingCoordinatorProxy::scrollViewForScrollingNodeID(ScrollingNodeID nodeID) const

I wonder what other things call this that will now suddenly work in iframes? Need smfr's input.

> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeFrameScrollingNodeRemoteIOS.h:41
> +    UIScrollView* scrollView() const;

Star's on the wrong side for all of your UIScrollViews

[Devin Rousso]

Created attachment 425215 [details]
Patch

[Wenson Hsieh]

Comment on attachment 425211 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425211&action=review

>> Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteScrollingCoordinatorProxyIOS.mm:57
>>  UIScrollView *RemoteScrollingCoordinatorProxy::scrollViewForScrollingNodeID(ScrollingNodeID nodeID) const
> 
> I wonder what other things call this that will now suddenly work in iframes? Need smfr's input.

From glancing through the code, it seems this is only used in this one place (i.e. to find the right scroll view to use as the position tracking view in `-overridePositionTrackingViewForTargetedPreviewIfNecessary:containerScrollingNodeID:`).

[Wenson Hsieh]

Comment on attachment 425215 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425215&action=review

r=mews

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:7567
> +        [self.textEffectsWindow addSubview:_targetedPreviewViewsContainerView.get()];

This /might/ be fine as-is...but since we're now inserting `_targetedPreviewViewsContainerView` in a window that we don't own, it would probably be a good idea to ensure that we remove it if the web view is removed from the view hierarchy (and more generally, when we don't require it anymore).

[Devin Rousso]

Comment on attachment 425215 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425215&action=review

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:7567
>> +        [self.textEffectsWindow addSubview:_targetedPreviewViewsContainerView.get()];
> 
> This /might/ be fine as-is...but since we're now inserting `_targetedPreviewViewsContainerView` in a window that we don't own, it would probably be a good idea to ensure that we remove it if the web view is removed from the view hierarchy (and more generally, when we don't require it anymore).

Good call.  Would this involve adding something to `-dealloc` or somewhere else?

[Wenson Hsieh]

Comment on attachment 425215 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425215&action=review

>>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:7567
>>> +        [self.textEffectsWindow addSubview:_targetedPreviewViewsContainerView.get()];
>> 
>> This /might/ be fine as-is...but since we're now inserting `_targetedPreviewViewsContainerView` in a window that we don't own, it would probably be a good idea to ensure that we remove it if the web view is removed from the view hierarchy (and more generally, when we don't require it anymore).
> 
> Good call.  Would this involve adding something to `-dealloc` or somewhere else?

I was thinking along the lines of `-didMoveToWindow`, with a nil `self.window` (i.e. the content view is being unparented).

[Devin Rousso]

Created attachment 425323 [details]
Patch

remove `_targetedPreviewViewsContainerView` when removed from a window and make `_scrollViewForTargetedPreview` into a weak pointer

[EWS]

Committed r275562: <https://commits.webkit.org/r275562>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425323 [details].

[WebKit Commit Bot]

Re-opened since this is blocked by bug 225002

[Devin Rousso]

Created attachment 428572 [details]
Patch

[Devin Rousso]

<rdar://problem/77089174>

[Devin Rousso]

Created attachment 428575 [details]
Patch

[Wenson Hsieh]

Comment on attachment 428575 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428575&action=review

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1133
> +- (void)cleanUpRelatedViews

Nit - "Related views" sounds a bit vague. What about something like `-cleanUpPreviewContainers`?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:7668
> +    if (!_dropPreviewContainerView)
> +        return;

Nit - I think you can remove this early return.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:7687
> +    if (!_dragPreviewContainerView)
> +        return;

(Ditto)

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:7717
> +    if (!_contextMenuHintContainerView)
> +        return;

(Ditto)

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ContextMenus.mm:375
> +TEST(ContextMenu, DISABLED_HintPreviewContainer)

We should really investigate these at some point :/

[Devin Rousso]

Comment on attachment 428575 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428575&action=review

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1133
>> +- (void)cleanUpRelatedViews
> 
> Nit - "Related views" sounds a bit vague. What about something like `-cleanUpPreviewContainers`?

I think `cleanUpInteractionPreviewContainers` is a bit more specific so I'm going to go with that.

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:7668
>> +        return;
> 
> Nit - I think you can remove this early return.

I have some logic I'm currently planning on adding in a followup to this that I don't want to do if this isn't set.

[Devin Rousso]

Created attachment 428593 [details]
Patch

[EWS]

Committed r277501 (237733@main): <https://commits.webkit.org/237733@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428593 [details].