Bug 188826

Summary: Change Selection modification to not snap the grabber when selecting above or below the selection anchor
Product: WebKit Reporter: Megan Gardner <megan_gardner>
Component: New BugsAssignee: Megan Gardner <megan_gardner>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, bdakin, commit-queue, rniwa, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing none

Description Megan Gardner 2018-08-21 16:29:06 PDT 
Change Selection modification to not snap the grabber when selecting above or below the selection anchor
Comment 1 Megan Gardner 2018-08-21 16:37:06 PDT 
Created attachment 347724 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2018-08-21 16:38:20 PDT 
<rdar://problem/43583165>
Comment 3 Tim Horton 2018-08-21 17:15:25 PDT 
Comment on attachment 347724 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1224
> +    IntPoint adjustedPoint(pointInRootViewCoordinates.x(), pointInRootViewCoordinates.y());

Why not just `IntPoint adjustedPoint = pointInRootViewCoordinates`?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1228
> +        int startY = caret.y() + caret.height() / 2;

We have center(), maybe use that?
Comment 4 Wenson Hsieh 2018-08-21 17:19:54 PDT 
Comment on attachment 347724 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1242
> +    HitTestResult hitTest = frame.eventHandler().hitTestResultAtPoint(adjustedPoint, HitTestRequest::ReadOnly | HitTestRequest::Active | HitTestRequest::AllowChildFrameContent);

adjustedPoint is in root view coordinates, no?
Comment 5 Tim Horton 2018-08-21 17:22:02 PDT 
Comment on attachment 347724 [details]
Patch

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

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1242
>> +    HitTestResult hitTest = frame.eventHandler().hitTestResultAtPoint(adjustedPoint, HitTestRequest::ReadOnly | HitTestRequest::Active | HitTestRequest::AllowChildFrameContent);
> 
> adjustedPoint is in root view coordinates, no?

Oops.
Comment 6 Megan Gardner 2018-08-21 17:37:34 PDT 
Created attachment 347741 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2018-08-21 18:41:56 PDT 
Comment on attachment 347741 [details]
Patch for landing

Clearing flags on attachment: 347741

Committed r235153: <https://trac.webkit.org/changeset/235153>
Comment 8 WebKit Commit Bot 2018-08-21 18:41:58 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 9 Alexey Proskuryakov 2018-08-23 09:55:50 PDT 
Ryan is fixing layout tests in bug 188888. In the future, please wait for EWS before landing patches.
Comment 10 Wenson Hsieh 2018-09-08 15:22:07 PDT 
Comment on attachment 347741 [details]
Patch for landing

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

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1230
> +            adjustedPoint.setY(startY);

It looks like the caret bounds are in content coordinates, but adjusted point is in root view coordinates?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1235
> +            adjustedPoint.setY(endY);

(Here too)