rdar://74845918
Created attachment 449063 [details] Test case
Created attachment 449069 [details] For EWS
Comment on attachment 449069 [details] For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=449069&action=review > Source/WebCore/editing/FrameSelection.cpp:543 > - clear(); > + if (RefPtr node = m_position.deepEquivalent().anchorNode()) > + invalidateCaretRect(node.get(), true); > + > + m_position = { }; > + clearCaretRect(); I worry that nothing is left behind here other than the regression test to explain the subtle reason why this must not call clear(). Is there any way we can make this subtle point more obvious? Function naming? Some discipline across this entire class that more explicitly addresses repaint issues? Some kind of comment?
Comment on attachment 449069 [details] For EWS View in context: https://bugs.webkit.org/attachment.cgi?id=449069&action=review Thanks for the review! >> Source/WebCore/editing/FrameSelection.cpp:543 >> + clearCaretRect(); > > I worry that nothing is left behind here other than the regression test to explain the subtle reason why this must not call clear(). > > Is there any way we can make this subtle point more obvious? Function naming? Some discipline across this entire class that more explicitly addresses repaint issues? Some kind of comment? That's a good point — perhaps I could factor this piece of logic out into helper method named something like, "clearCaretPositionWithoutUpdatingStyle()"?
(In reply to Wenson Hsieh from comment #4) > perhaps I could factor this piece of logic out into > helper method named something like, > "clearCaretPositionWithoutUpdatingStyle()"? I love that. My favorite type of comment is a good function name.
Still might add a *brief* comment at the call site stating *why* it’s important to not update style in this case (and, by implication, why it’s not important in other cases).
(In reply to Darin Adler from comment #6) > Still might add a *brief* comment at the call site stating *why* it’s > important to not update style in this case (and, by implication, why it’s > not important in other cases). Sounds good! Will pull it into the helper method, and add a brief comment explaining that it's unsafe to update style here due to being in the middle of node removal.
Created attachment 449089 [details] For landing
Committed r287992 (246019@main): <https://commits.webkit.org/246019@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 449089 [details].