Bug 186900 - Fix IBeam issues with iPad apps on Mac
Summary: Fix IBeam issues with iPad apps on Mac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Megan Gardner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-06-21 15:56 PDT by Megan Gardner
Modified: 2018-06-27 18:29 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.34 KB, patch)
2018-06-21 16:14 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (1.66 KB, patch)
2018-06-21 16:19 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (1.64 KB, patch)
2018-06-21 17:10 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.81 MB, application/zip)
2018-06-23 06:46 PDT, Build Bot
no flags Details
Patch (4.55 KB, patch)
2018-06-25 17:20 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (4.65 KB, patch)
2018-06-25 18:19 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.78 MB, application/zip)
2018-06-26 09:07 PDT, Build Bot
no flags Details
Patch (13.29 KB, patch)
2018-06-26 15:05 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (7.43 KB, patch)
2018-06-26 17:44 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (7.55 KB, patch)
2018-06-27 16:53 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Megan Gardner 2018-06-21 15:56:37 PDT
Fix IBeam issues with iPad apps on Mac
Comment 1 Megan Gardner 2018-06-21 16:14:10 PDT
Created attachment 343283 [details]
Patch
Comment 2 Megan Gardner 2018-06-21 16:19:20 PDT
Created attachment 343286 [details]
Patch
Comment 3 Tim Horton 2018-06-21 16:28:24 PDT
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.
Comment 4 Megan Gardner 2018-06-21 17:10:23 PDT
Created attachment 343294 [details]
Patch
Comment 5 Tim Horton 2018-06-21 17:17:10 PDT
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?
Comment 6 Wenson Hsieh 2018-06-21 17:26:00 PDT
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...
Comment 7 Wenson Hsieh 2018-06-22 09:14:06 PDT
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.
Comment 8 Build Bot 2018-06-23 06:46:34 PDT
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
Comment 9 Build Bot 2018-06-23 06:46:45 PDT
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
Comment 10 Megan Gardner 2018-06-25 17:20:02 PDT
Created attachment 343564 [details]
Patch
Comment 11 Tim Horton 2018-06-25 17:22:06 PDT
(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.
Comment 12 Tim Horton 2018-06-25 17:42:07 PDT
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.

🤷‍♂️
Comment 13 Megan Gardner 2018-06-25 18:19:38 PDT
Created attachment 343570 [details]
Patch
Comment 14 Darin Adler 2018-06-25 18:36:01 PDT
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".
Comment 15 Build Bot 2018-06-26 09:07:03 PDT
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
Comment 16 Build Bot 2018-06-26 09:07:15 PDT
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
Comment 17 Wenson Hsieh 2018-06-26 09:18:18 PDT
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).
Comment 18 Megan Gardner 2018-06-26 15:05:09 PDT
Created attachment 343643 [details]
Patch
Comment 19 Tim Horton 2018-06-26 16:12:52 PDT
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?
Comment 20 Megan Gardner 2018-06-26 17:44:39 PDT
Created attachment 343665 [details]
Patch
Comment 21 Wenson Hsieh 2018-06-27 15:13:44 PDT
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.
Comment 22 Radar WebKit Bug Importer 2018-06-27 15:52:34 PDT
<rdar://problem/41548811>
Comment 23 Tim Horton 2018-06-27 15:56:55 PDT
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
Comment 24 Megan Gardner 2018-06-27 16:53:28 PDT
Created attachment 343770 [details]
Patch for landing
Comment 25 WebKit Commit Bot 2018-06-27 18:29:05 PDT
The commit-queue encountered the following flaky tests while processing attachment 343770 [details]:

inspector/unit-tests/editing-support.html bug 187128 (author: nvasilyev@apple.com)
The commit-queue is continuing to process your patch.
Comment 26 WebKit Commit Bot 2018-06-27 18:29:50 PDT
Comment on attachment 343770 [details]
Patch for landing

Clearing flags on attachment: 343770

Committed r233296: <https://trac.webkit.org/changeset/233296>
Comment 27 WebKit Commit Bot 2018-06-27 18:29:52 PDT
All reviewed patches have been landed.  Closing bug.