Bug 210939

Summary: Caret may be placed in the wrong spot for text input context that is a form control
Product: WebKit Reporter: Daniel Bates <dbates>
Component: WebKit Misc.Assignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, ews-watchlist, mifenton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=211056
https://bugs.webkit.org/show_bug.cgi?id=213975
Attachments:
Description Flags
Patch
none
To Land none

Description Daniel Bates 2020-04-23 15:38:28 PDT
-_focusTextInputContext:placeCaretAt:completionHandler: may place the caret in the wrong spot when the text input context represents a form control element (e.g. <input>) and the point is outside the bounds of the form control's inner element.
Comment 1 Daniel Bates 2020-04-23 15:38:42 PDT
<rdar://problem/61943089>
Comment 2 Daniel Bates 2020-04-26 16:45:38 PDT
Created attachment 397635 [details]
Patch
Comment 3 Daniel Bates 2020-04-26 16:57:38 PDT
Comment on attachment 397635 [details]
Patch

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

> Source/WebCore/editing/Editing.cpp:585
> +    auto visiblePosition = frame->visiblePositionForPoint(constrainedPoint);

I don't need to do this, which does a hit test. I think I can do something like:

[[
bool isFixed = renderer->style().position() == PositionType::Fixed;
FloatPoint localPoint = renderer->absoluteToLocal(constrainedPoint, UseTransforms | (isFixed ? IsFixed : 0));
auto visiblePosition = renderer->positionForPoint(flooredLayoutPoint(localPoint), nullptr);
]]

To the reviewer, I hope you don't mind that I do this in a follow up likely in the same fix for the FIXME I added in focusTextInputContextAndPlaceCaret() below. I prefer to do it this way to minimize risk: I want to fix this bug with the smallest change necessary given the trickiness with hit testing. This change builds up my confidence to then more radically change the algorithm.
Comment 4 Daniel Bates 2020-04-26 17:48:14 PDT
Test failure is fixed by bug #211056
Comment 5 Darin Adler 2020-04-26 18:02:16 PDT
Comment on attachment 397635 [details]
Patch

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

> Source/WebCore/editing/Editing.cpp:568
> +    RenderObject* renderer = element.renderer();

Getting the renderer before update layout is always a risky proposition. This might be an element that was display none until now and was just changed.

> Source/WebCore/editing/Editing.cpp:570
> +    if (!renderer)
> +        return { };

This isn’t needed. The second null check below is sufficient.
Comment 6 Daniel Bates 2020-04-26 18:37:22 PDT
Comment on attachment 397635 [details]
Patch

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

Thanks for the review, Darin!

>> Source/WebCore/editing/Editing.cpp:568
>> +    RenderObject* renderer = element.renderer();
> 
> Getting the renderer before update layout is always a risky proposition. This might be an element that was display none until now and was just changed.

Yep, I made an assumption based on my current caller, which DOES  a layout. I will update layout here too to future proof.

>> Source/WebCore/editing/Editing.cpp:570
>> +        return { };
> 
> This isn’t needed. The second null check below is sufficient.

Yep. Will remove.
Comment 7 Daniel Bates 2020-04-27 09:07:11 PDT
Created attachment 397687 [details]
To Land
Comment 8 Daniel Bates 2020-04-27 09:08:25 PDT
Comment on attachment 397687 [details]
To Land

Clearing flags on attachment: 397687

Committed r260759: <https://trac.webkit.org/changeset/260759>
Comment 9 Daniel Bates 2020-04-27 09:08:27 PDT
All reviewed patches have been landed.  Closing bug.