WKHoverPlatter should scale up the platter content to make it easier to see
Created attachment 451222 [details] Patch
Created attachment 451223 [details] Patch
Comment on attachment 451223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451223&action=review r=me with tvOS/watchOS builds fixed! > Source/WebKit/UIProcess/WebPageProxy.cpp:11159 > +void WebPageProxy::interactableRegionsInRect(WebCore::FloatRect rect, CompletionHandler<void(Vector<WebCore::FloatRect>)>&& completionHandler) Maybe give this a name that better describes the coordinate space? Given the usage, it looks like it's in root view space. (Also, you can remove the WebCore:: here) > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3107 > + if (WKHoverPlatterDomain.rootSettings.enabled) > + return [_hoverPlatter adjustedPointForPoint:location]; Nit - maybe it would it be cleaner if we folded these `WKHoverPlatterDomain.rootSettings.enabled` checks into -adjustedPointForPoint:? > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3117 > + if (WKHoverPlatterDomain.rootSettings.enabled) > + return [_hoverPlatter adjustedPointForPoint:location]; (Ditto) > Source/WebKit/UIProcess/ios/WKHoverPlatter.mm:122 > + if (!boundingRect.contains(point) && point != WebCore::FloatPoint { -1 , -1 }) Nit - extra space before the , (It's also not super obvious where the (-1, -1) is coming from..) > Source/WebKit/UIProcess/ios/WKHoverPlatter.mm:300 > + delta.scale(1.f / WKHoverPlatterDomain.rootSettings.platterScale); `WKHoverPlatterDomain.rootSettings.platterScale` is always guaranteed to be nonzero, right? > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6268 > +void WebPage::interactableRegionsInRect(WebCore::FloatRect rect, CompletionHandler<void(Vector<WebCore::FloatRect>)>&& completionHandler) Nit - I think you can omit the WebCore:: here > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6275 > + auto result = HitTestResult { WebCore::LayoutRect(rect) }; (here too) > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6289 > + HitTestResult::NodeSet linkNodes; Nit - maybe `interactableNodes` or `hitTestedNodes`? (linkNodes makes it sounds a bit like it's only about links) > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6301 > + rects.append(renderer->absoluteBoundingBoxRect()); (Just to double check — we don't need to convert to root view coordinates here because all the hit-tested nodes are in the main frame?)
(In reply to Wenson Hsieh from comment #3) > Comment on attachment 451223 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=451223&action=review > > r=me with tvOS/watchOS builds fixed! > > > Source/WebKit/UIProcess/WebPageProxy.cpp:11159 > > +void WebPageProxy::interactableRegionsInRect(WebCore::FloatRect rect, CompletionHandler<void(Vector<WebCore::FloatRect>)>&& completionHandler) > > Maybe give this a name that better describes the coordinate space? Given the > usage, it looks like it's in root view space. Totally! > (Also, you can remove the WebCore:: here) > > > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3107 > > + if (WKHoverPlatterDomain.rootSettings.enabled) > > + return [_hoverPlatter adjustedPointForPoint:location]; > > Nit - maybe it would it be cleaner if we folded these > `WKHoverPlatterDomain.rootSettings.enabled` checks into > -adjustedPointForPoint:? It would, but _hoverPlatter is nil in that case, so it would break all interaction. > > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3117 > > + if (WKHoverPlatterDomain.rootSettings.enabled) > > + return [_hoverPlatter adjustedPointForPoint:location]; > > (Ditto) > > > Source/WebKit/UIProcess/ios/WKHoverPlatter.mm:122 > > + if (!boundingRect.contains(point) && point != WebCore::FloatPoint { -1 , -1 }) > > Nit - extra space before the , > > (It's also not super obvious where the (-1, -1) is coming from..) We don't have it named anywhere that I know, but it's the view-level "mouseout" position hardcoded in a few other places. Still, it is a good point... let me see what I can do. > > Source/WebKit/UIProcess/ios/WKHoverPlatter.mm:300 > > + delta.scale(1.f / WKHoverPlatterDomain.rootSettings.platterScale); > > `WKHoverPlatterDomain.rootSettings.platterScale` is always guaranteed to be > nonzero, right? I will change the minimum in WKHoverPlatterParameters to 1 (but, regardless, the divide-by-zeroes would be on you if you set the debug setting to 0 :P) > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6268 > > +void WebPage::interactableRegionsInRect(WebCore::FloatRect rect, CompletionHandler<void(Vector<WebCore::FloatRect>)>&& completionHandler) > > Nit - I think you can omit the WebCore:: here > > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6275 > > + auto result = HitTestResult { WebCore::LayoutRect(rect) }; > > (here too) > > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6289 > > + HitTestResult::NodeSet linkNodes; > > Nit - maybe `interactableNodes` or `hitTestedNodes`? (linkNodes makes it > sounds a bit like it's only about links) Right! I renamed the method from being about "link"s but missed this one. > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:6301 > > + rects.append(renderer->absoluteBoundingBoxRect()); > > (Just to double check — we don't need to convert to root view coordinates > here because all the hit-tested nodes are in the main frame?) Will double check the coordinate space, very possible this is wrong.
Created attachment 451340 [details] Patch
Created attachment 451342 [details] Patch
Committed r289463 (247006@main): <https://commits.webkit.org/247006@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 451342 [details].
<rdar://problem/88678328>