[webkit-reviews] review granted: [Bug 194873] [iOS] Callout menu overlaps in-page controls when editing a comment in github.com's issue tracker : [Attachment 362612] Patch

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 194873: [iOS] Callout menu overlaps in-page controls when editing a comment
in github.com's issue tracker
https://bugs.webkit.org/show_bug.cgi?id=194873

Attachment 362612: Patch

https://bugs.webkit.org/attachment.cgi?id=362612&action=review




--- Comment #6 from Tim Horton <thorton at apple.com> ---
Comment on attachment 362612
  --> https://bugs.webkit.org/attachment.cgi?id=362612
Patch

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

> Source/WebKit/Shared/WebPreferences.yaml:1562
> +SmartCalloutBarPositioningEnabled:

Do we really need a pref for this?

> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:1122
> +void
WebPageProxy::requestEvasionRectsAboveSelection(CompletionHandler<void(const
Vector<WebCore::FloatRect>&)>&& callback)

This is "above" because above is where we want to put it by default. Can we get
evasion rects below too and end up presenting the callout bar to the left or
right?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1560
> +    const Vector<FloatPoint, 5> offsetsForHitTesting {{ -30, -51 }, { 31,
-50 }, { -60, -34 }, { 60, -36 }, { 0, -20 }};

This is based on popover geometry? (it's in WebPageIOS, so I guess that's OK?)

Why are the numbers all a little fuzzy (where do the 1s and 4s and 6s come
from?)

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1563
> +	   if (auto* hitNode =
clickableNonEditableNode(centerTopInRootViewCoordinates + offset))

I peeked and I think this /does/ update layout, so you should be OK?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1581
> +    auto contextMenuElementSizeLimit = factorOfContentArea *
pageScaleFactor() * m_page->mainFrame().view()->unobscuredContentRect().size();

You already have pageScaleFactor() as scaleFactor.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1588
> +	   RefPtr<Element> element;
> +	   if (is<Element>(node))
> +	       element = downcast<Element>(node.ptr());
> +	   else
> +	       element = node->parentElement();

Weird, I would have expected this to be a thing node can do (nearest element).

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1594
> +	   if (bounds.width() > contextMenuElementSizeLimit.width() ||
bounds.height() > contextMenuElementSizeLimit.height())

Should this be area-based instead, so we can have long skinny bars?