-_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.
<rdar://problem/61943089>
Created attachment 397635 [details] Patch
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.
Test failure is fixed by bug #211056
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 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.
Created attachment 397687 [details] To Land
Comment on attachment 397687 [details] To Land Clearing flags on attachment: 397687 Committed r260759: <https://trac.webkit.org/changeset/260759>
All reviewed patches have been landed. Closing bug.