Bug 210939 – Caret may be placed in the wrong spot for text input context that is a form control

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Daniel Bates]

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

[Daniel Bates]

<rdar://problem/61943089>

[Daniel Bates]

Created attachment 397635 [details]
Patch

[Daniel Bates]

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.

[Daniel Bates]

Test failure is fixed by bug #211056

[Darin Adler]

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.

[Daniel Bates]

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.

[Daniel Bates]

Created attachment 397687 [details]
To Land

[Daniel Bates]

Comment on attachment 397687 [details]
To Land

Clearing flags on attachment: 397687

Committed r260759: <https://trac.webkit.org/changeset/260759>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.