Summary: | [iOS] contextmenu hints can be clipped by the WKWebView | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||||||||||
Component: | New Bugs | Assignee: | Devin Rousso <hi> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | commit-queue, hi, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=226055 | ||||||||||||||||||
Bug Depends on: | 225002 | ||||||||||||||||||
Bug Blocks: | |||||||||||||||||||
Attachments: |
|
Description
Devin Rousso
2021-04-05 14:25:12 PDT
Created attachment 425208 [details]
Patch
Created attachment 425211 [details]
Patch
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 Created attachment 425215 [details]
Patch
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 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 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 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). Created attachment 425323 [details]
Patch
remove `_targetedPreviewViewsContainerView` when removed from a window and make `_scrollViewForTargetedPreview` into a weak pointer
Committed r275562: <https://commits.webkit.org/r275562> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425323 [details]. Re-opened since this is blocked by bug 225002 Created attachment 428572 [details]
Patch
Created attachment 428575 [details]
Patch
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 :/ 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. Created attachment 428593 [details]
Patch
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]. |