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
Daniel Bates
2020-04-23 15:38:28 PDT
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. |