I don't think we should be calling nextCandidate in positionAtStartOfInsertedContent. We need to simplify this function in order to resolve the bug 68874.
Created attachment 108916 [details] Simplified the function with additional fixes and refactoring
Comment on attachment 108916 [details] Simplified the function with additional fixes and refactoring View in context: https://bugs.webkit.org/attachment.cgi?id=108916&action=review > Source/WebCore/rendering/RenderText.cpp:1606 > -unsigned RenderText::caretMaxRenderedOffset() const > +unsigned RenderText::renderedTextLength() const Come to think of it, caretMaxOffset should call this function. Will do in a minute.
(In reply to comment #2) > (From update of attachment 108916 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=108916&action=review > > > Source/WebCore/rendering/RenderText.cpp:1606 > > -unsigned RenderText::caretMaxRenderedOffset() const > > +unsigned RenderText::renderedTextLength() const > > Come to think of it, caretMaxOffset should call this function. Will do in a minute. Ugh... not really. That won't work.
Created attachment 108926 [details] new patch
Comment on attachment 108926 [details] new patch View in context: https://bugs.webkit.org/attachment.cgi?id=108926&action=review > Source/WebCore/ChangeLog:12 > + This change revealed a bug in removeUnrenderedTextNodesAtEnds that text nodes without any visible > + text at ends are not removed when it has a render object. Fixed the bug by checking the length of > + the rendered text. Since you fixed a bug we need to add a test case to show that bug in action, or call out which test cases shows us the bug. > Source/WebCore/editing/ReplaceSelectionCommand.cpp:559 > + return node->renderer() && node->renderer()->isText() && toRenderText(node->renderer())->renderedTextLength() > 0; I think that text nodes are guaranteed to have RenderText renderers or no renderer at all, so I don’t think the isText check is needed. If it is needed, we should check in a test case that exercises that code path.
Comment on attachment 108926 [details] new patch View in context: https://bugs.webkit.org/attachment.cgi?id=108926&action=review >> Source/WebCore/ChangeLog:12 >> + the rendered text. > > Since you fixed a bug we need to add a test case to show that bug in action, or call out which test cases shows us the bug. It's editing/pasteboard/pasting-word-in-div-extra-line.html. >> Source/WebCore/editing/ReplaceSelectionCommand.cpp:559 >> + return node->renderer() && node->renderer()->isText() && toRenderText(node->renderer())->renderedTextLength() > 0; > > I think that text nodes are guaranteed to have RenderText renderers or no renderer at all, so I don’t think the isText check is needed. If it is needed, we should check in a test case that exercises that code path. On the other hand, I didn't want to make this function hard to use. I don't want someone to start calling this function without checking that node is a text node. I guess I can change the argument type to Text*?
Created attachment 108943 [details] Addressed comments
Thanks for reviewing these patches!
Committed r96187: <http://trac.webkit.org/changeset/96187>