Bug 186900

Created attachment 343283 [details] Patch

Created attachment 343286 [details] Patch

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.

Created attachment 343294 [details] Patch

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

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

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

Created attachment 343564 [details] Patch

(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 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. 🤷‍♂️

Created attachment 343570 [details] Patch

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

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

Created attachment 343643 [details] Patch

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?

Created attachment 343665 [details] Patch

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.

<rdar://problem/41548811>

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

Created attachment 343770 [details] Patch for landing

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 on attachment 343770 [details] Patch for landing Clearing flags on attachment: 343770 Committed r233296: <https://trac.webkit.org/changeset/233296>

All reviewed patches have been landed. Closing bug.