Bug 224204 - [iOS] contextmenu hints can be clipped by the WKWebView
Summary: [iOS] contextmenu hints can be clipped by the WKWebView
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-04-05 14:25 PDT by Devin Rousso
Modified: 2021-04-06 15:29 PDT (History)
5 users (show)

See Also:


Attachments
Patch (16.42 KB, patch)
2021-04-05 14:33 PDT, Devin Rousso
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (16.97 KB, patch)
2021-04-05 14:55 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (16.96 KB, patch)
2021-04-05 15:39 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (18.92 KB, patch)
2021-04-06 13:59 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2021-04-05 14:25:12 PDT
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`
Comment 1 Devin Rousso 2021-04-05 14:25:27 PDT
<rdar://problem/75504620>
Comment 2 Devin Rousso 2021-04-05 14:33:40 PDT
Created attachment 425208 [details]
Patch
Comment 3 Devin Rousso 2021-04-05 14:55:21 PDT
Created attachment 425211 [details]
Patch
Comment 4 Tim Horton 2021-04-05 15:33:21 PDT
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
Comment 5 Devin Rousso 2021-04-05 15:39:57 PDT
Created attachment 425215 [details]
Patch
Comment 6 Wenson Hsieh 2021-04-05 19:33:46 PDT
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:`).
Comment 7 Wenson Hsieh 2021-04-05 19:34:17 PDT
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).
Comment 8 Devin Rousso 2021-04-05 20:06:28 PDT
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?
Comment 9 Wenson Hsieh 2021-04-05 20:09:10 PDT
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).
Comment 10 Devin Rousso 2021-04-06 13:59:59 PDT
Created attachment 425323 [details]
Patch

remove `_targetedPreviewViewsContainerView` when removed from a window and make `_scrollViewForTargetedPreview` into a weak pointer
Comment 11 EWS 2021-04-06 15:29:21 PDT
Committed r275562: <https://commits.webkit.org/r275562>

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