Bug 186900 – Fix IBeam issues with iPad apps on Mac

• Component: New Bugs
• Status: RESOLVED FIXED

[Megan Gardner]

Fix IBeam issues with iPad apps on Mac

[Megan Gardner]

Created attachment 343283 [details]
Patch

[Megan Gardner]

Created attachment 343286 [details]
Patch

[Tim Horton]

Comment on attachment 343286 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3324
> +    if (![self ensurePositionInformationIsUpToDate:request])

Just Say No to implementing new synchronous IPC.

[Megan Gardner]

Created attachment 343294 [details]
Patch

[Tim Horton]

Comment on attachment 343294 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3324
> +    [self requestAsynchronousPositionInformationUpdate:request];

Don’t you want to request inside hover, so that there’s /some/ chance it’s up to date here?

[Wenson Hsieh]

Comment on attachment 343294 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3327
> +        return [WKTextPosition textPositionWithRect:_positionInformation.bounds];

This is going to return a really, really big caret...

[Wenson Hsieh]

Probably/definitely not for the short term, but we'll eventually need UIKit to provide some sort of SPI or API to either:

  • Allow us to control the cursor type, or
  • Ask us for a cursor type at a location (preferably with an async callback)

so that we can actually respect the "cursor" CSS property.

[EWS Watchlist]

Comment on attachment 343294 [details]
Patch

Attachment 343294 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8304027

New failing tests:
http/tests/preload/onload_event.html
http/tests/security/canvas-remote-read-remote-video-localhost.html

[EWS Watchlist]

Created attachment 343434 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Megan Gardner]

Created attachment 343564 [details]
Patch

[Tim Horton]

(In reply to Tim Horton from comment #5)
> Comment on attachment 343294 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343294&action=review
> 
> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3324
> > +    [self requestAsynchronousPositionInformationUpdate:request];
> 
> Don’t you want to request inside hover, so that there’s /some/ chance it’s
> up to date here?

Still haven’t done this.

[Tim Horton]

Comment on attachment 343564 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2178
> +            // make a caret rect

Get rid of this.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2180
> +            VisiblePosition visiblePos(hitNode->renderer()->positionForPoint(request.point, nullptr));

We’ve already stashed renderer in a local.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2181
> +            info.caretRect = visiblePos.absoluteCaretBounds(&isInsideFixedPosition);

If we’re committed to only doing the UI process side of this in iPad-apps-on-Mac mode, do we need to do this (making a VisiblePosition is nontrivial!) work on other platforms?

But then again, it would be weird to have caretRect there and have it always be invalid.

🤷‍♂️

[Megan Gardner]

Created attachment 343570 [details]
Patch

[Darin Adler]

Comment on attachment 343570 [details]
Patch

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

I would say review+, but I don’t understand the use of a rectangle to transmit a WKTextPosition. Someone else better versed with the iOS code is welcome to review+.

> Source/WebKit/ChangeLog:20
> +        UIKit is using this function to determine if we should show an iBeam or not.
> +        So we need to implement it, at least for this platform. 

I’m a little confused. I am not sure why a rectangle is the best way to transmit a WKTextPosition across process boundaries.

> Source/WebKit/Shared/ios/InteractionInformationAtPosition.h:65
> +    WebCore::IntRect caretRect; // currenlty only valid on iPad apps on Mac

typo: currenlty

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2179
> +            bool isInsideFixedPosition = false;

Doesn’t seem important to initialize something that is an out argument.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2180
> +            VisiblePosition visiblePos(renderer->positionForPoint(request.point, nullptr));

Normally WebKit coding style says to use words, not abbreviations like "pos". I think I would have called this local variable just "position" or maybe "caretPosition".

[EWS Watchlist]

Comment on attachment 343570 [details]
Patch

Attachment 343570 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8347152

New failing tests:
http/tests/security/canvas-remote-read-remote-video-redirect.html

[EWS Watchlist]

Created attachment 343607 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Wenson Hsieh]

Comment on attachment 343570 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3325
> +    if (_positionInformation.isSelectable)

I would still check here for whether the cached position information is valid for the request (or maybe just check to make sure the points are close enough).

[Megan Gardner]

Created attachment 343643 [details]
Patch

[Tim Horton]

Comment on attachment 343643 [details]
Patch

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

> Source/WebKit/ChangeLog:20
> +        As there is no way to premptively reqest information on hover, we need to use 

s/reqest/request/

> Source/WebKit/ChangeLog:29
> +        UIKit is using this function to determine if we should show an iBeam or not.

Capital I. Because I-beams look like I... they don’t have disconnected dots :)

> Source/WebKit/Shared/ios/InteractionInformationRequest.cpp:71
> +bool InteractionInformationRequest::isApproximateForRequest(const InteractionInformationRequest& other)

isApproximatelyValidForRequest?

You also need to keep the includeSnapshot/IncludeLinkIndicator checks, if someone calls this wanting a snapshot and we don’t have one you should say no even if the points are close.

> Source/WebKit/Shared/ios/InteractionInformationRequest.cpp:73
> +    if (std::abs(other.point.x()-point.x()) > 10)

Maybe something more like

“return (other.point - point).diagonalLengthSquared() <= 4” or so?

[Megan Gardner]

Created attachment 343665 [details]
Patch

[Wenson Hsieh]

Comment on attachment 343665 [details]
Patch

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

> Source/WebKit/Shared/ios/InteractionInformationRequest.cpp:71
> +bool InteractionInformationRequest::isApproximateForRequest(const InteractionInformationRequest& other)

Nit - as Tim mentioned earlier, I think isApproximatelyValidForRequest is a more appropriate name here.

[Radar WebKit Bug Importer]

<rdar://problem/41548811>

[Tim Horton]

Comment on attachment 343665 [details]
Patch

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

> Source/WebKit/Shared/ios/InteractionInformationAtPosition.h:65
> +    WebCore::IntRect caretRect; // currently only valid on iPad apps on Mac

Maybe this gets #If’d out instead of the comment?

>> Source/WebKit/Shared/ios/InteractionInformationRequest.cpp:71
>> +bool InteractionInformationRequest::isApproximateForRequest(const InteractionInformationRequest& other)
> 
> Nit - as Tim mentioned earlier, I think isApproximatelyValidForRequest is a more appropriate name here.

Yes please

[Megan Gardner]

Created attachment 343770 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 343770 [details]:

inspector/unit-tests/editing-support.html bug 187128 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 343770 [details]
Patch for landing

Clearing flags on attachment: 343770

Committed r233296: <https://trac.webkit.org/changeset/233296>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.