Bug 194873 – [iOS] Callout menu overlaps in-page controls when editing a comment in github.com's issue tracker

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Wenson Hsieh]

1. Go to a repository on https://github.com.
2. Open a new issue and insert a paragraph of text.
3. Select some text near the end of the first line.

Observed: black callout bar appears, obscuring header/bold/italic/quotation/code snippet/link buttons.
Expected: it would be great if I could still access these controls...

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #0)
> 1. Go to a repository on https://github.com.

Step 0: Use an iPad

[Wenson Hsieh]

<rdar://problem/46701974>

[Wenson Hsieh]

Created attachment 362590 [details]
Work in progress

[Wenson Hsieh]

Created attachment 362592 [details]
Work in progress

[Wenson Hsieh]

Created attachment 362612 [details]
Patch

[Tim Horton]

Comment on attachment 362612 [details]
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?

[Wenson Hsieh]

Comment on attachment 362612 [details]
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?

It's not necessary, but it might help for demoing and testing!

Perhaps that's not a good reason though, so I'll remove 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?

Yes, as of now, we only hit-test "above" because that's where the bar goes by default. We could additionally do more hit-testing below to force the bar to present on the left or right sides, but I refrained from doing that here because (1) I didn't find any websites that actually did this, and (2) hit-testing is fairly expensive, so without knowing specific websites that would benefit from getting evasion rects below the selection, I'd prefer to limit the amount of hit-testing that needs to happen here :/

>> 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?)

Yes, it's based on callout bar geometry, and is therefore specific to iOS. I originally hit-tested a grid-aligned lattice of points above the selection, but then chose to perturb some of the locations a tiny bit (±1, ±1) to ensure that we don't fail detection if a row of points happen to land on a divider or something.

...but now that I think about this some more, it shouldn't be necessary because nodeRespondingToClickEvents is already fuzzy. So I'll un-fuzz these offsets.

>> 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?

Sure! Also, the call to VisibleSelection::toNormalizedRange() above should have already updated layout.

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

Good catch!

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1588
>> +            element = node->parentElement();
> 
> Weird, I would have expected this to be a thing node can do (nearest element).

This comment made me realize that I shouldn't even need to go from Node -> Element; getting the bounds of the renderer and then converting to root view coordinates should suffice. I'll change the logic here to do this.

>> 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?

Good call, I'll make this area-based instead.

[Wenson Hsieh]

Created attachment 362769 [details]
Patch for EWS

[WebKit Commit Bot]

Comment on attachment 362769 [details]
Patch for EWS

Clearing flags on attachment: 362769

Committed r241971: <https://trac.webkit.org/changeset/241971>