Bug 68939 – Simplify ReplaceSelectionCommand::positionAtStartOfInsertedContent

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

I don't think we should be calling nextCandidate in positionAtStartOfInsertedContent. We need to simplify this function in order to resolve the bug 68874.

[Ryosuke Niwa]

Created attachment 108916 [details]
Simplified the function with additional fixes and refactoring

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Created attachment 108926 [details]
new patch

[Darin Adler]

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.

[Ryosuke Niwa]

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*?

[Ryosuke Niwa]

Created attachment 108943 [details]
Addressed comments

[Ryosuke Niwa]

Thanks for reviewing these patches!

[Ryosuke Niwa]

Committed r96187: <http://trac.webkit.org/changeset/96187>