Bug 98188 – ReplaceSelectionCommand should merge text nodes

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

In order to resolve the bug 97266, we need to improve ReplaceSelectionCommand to merge consecutive text nodes.

[Ryosuke Niwa]

Created attachment 166770 [details]
work in progress

There are still 12 test failures :(

[Ryosuke Niwa]

Comment on attachment 166770 [details]
work in progress

View in context: https://bugs.webkit.org/attachment.cgi?id=166770&action=review

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1185
> +    bool notStart = !isStartOfParagraph(startOfInsertedContent);
> +    VisiblePosition p = startOfInsertedContent.previous();
> +    UChar32 c = p.characterAfter();
> +    if (c == noBreakSpace)
> +        c = ' ';
> +    bool needsLeadingSpace = notStart && !isCharacterSmartReplaceExempt(c, true);

isCharacterSmartReplaceExempt returns false on nbsp :( But we shouldn't be adding a space between words when there's already a non-breaking space.

[Ryosuke Niwa]

Created attachment 166978 [details]
Patch

[Levi Weintraub]

Comment on attachment 166978 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166978&action=review

This is a great change. Consider a rename of the second parameter in mergeTextNodesAroundPosition?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1160
> +    UChar32 characterAfterEndOfContent = endOfInsertedContent.characterAfter();
> +    bool needsTrailingSpace = !isEndOfParagraph(endOfInsertedContent)
> +        && !isCharacterSmartReplaceExempt(characterAfterEndOfContent == noBreakSpace ? ' ' : characterAfterEndOfContent, false);

I'm not a big fan of the solution here being to lie to isCharacterSmartReplaceExempt, as I don't immediately find this self-documenting.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1184
> +    bool notStart = !isStartOfParagraph(startOfInsertedContent);

I don't find this clearer than having the call in needsLeadingSpace and we don't reuse this value.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1186
> +    bool needsLeadingSpace = notStart && !isCharacterSmartReplaceExempt(characterAtStart == noBreakSpace ? ' ' : characterAtStart, true);

Same comment  as before.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1234
> +void ReplaceSelectionCommand::mergeTextNodesAroundPosition(Position& position, Position& anotherPositionToAdjust)

Why "anotherPositionToAdjust"? This makes it sound to me like we're going to adjust the text nodes around that position as well. We're only directly modifying text nodes around position. The second position is provided only to be updated with the changes, correct?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1283
> +            updatePositionForNodeRemoval(anotherPositionToAdjust, next);

I'm guessing we could coalesce the two updatePositionForNodeRemovals, but it's probably not worth the complexity.

[Ryosuke Niwa]

Comment on attachment 166978 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166978&action=review

>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1160
>> +        && !isCharacterSmartReplaceExempt(characterAfterEndOfContent == noBreakSpace ? ' ' : characterAfterEndOfContent, false);
> 
> I'm not a big fan of the solution here being to lie to isCharacterSmartReplaceExempt, as I don't immediately find this self-documenting.

Any suggestion?

>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1184
>> +    bool notStart = !isStartOfParagraph(startOfInsertedContent);
> 
> I don't find this clearer than having the call in needsLeadingSpace and we don't reuse this value.

Oops, this is a left over from my debugging code. Will fix.

>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1234
>> +void ReplaceSelectionCommand::mergeTextNodesAroundPosition(Position& position, Position& anotherPositionToAdjust)
> 
> Why "anotherPositionToAdjust"? This makes it sound to me like we're going to adjust the text nodes around that position as well. We're only directly modifying text nodes around position. The second position is provided only to be updated with the changes, correct?

Why suggestion?

>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1283
>> +            updatePositionForNodeRemoval(anotherPositionToAdjust, next);
> 
> I'm guessing we could coalesce the two updatePositionForNodeRemovals, but it's probably not worth the complexity.

We can't.

[Ryosuke Niwa]

(In reply to comment #4)
> (From update of attachment 166978 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166978&action=review
> 
> This is a great change. Consider a rename of the second parameter in mergeTextNodesAroundPosition?
> 
> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1160
> > +    UChar32 characterAfterEndOfContent = endOfInsertedContent.characterAfter();
> > +    bool needsTrailingSpace = !isEndOfParagraph(endOfInsertedContent)
> > +        && !isCharacterSmartReplaceExempt(characterAfterEndOfContent == noBreakSpace ? ' ' : characterAfterEndOfContent, false);
> 
> I'm not a big fan of the solution here being to lie to isCharacterSmartReplaceExempt, as I don't immediately find this self-documenting.

To give you some context, this function is implemented by each port and relies on ICU, etc...

[Levi Weintraub]

Comment on attachment 166978 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166978&action=review

>>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1160
>>> +        && !isCharacterSmartReplaceExempt(characterAfterEndOfContent == noBreakSpace ? ' ' : characterAfterEndOfContent, false);
>> 
>> I'm not a big fan of the solution here being to lie to isCharacterSmartReplaceExempt, as I don't immediately find this self-documenting.
> 
> Any suggestion?

Something like needsSpaceForSmartReplace? isCharacterSmartReplaceExemptConsideringNonBreakingSpace()? Something with even more characters? ;)

>>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1234
>>> +void ReplaceSelectionCommand::mergeTextNodesAroundPosition(Position& position, Position& anotherPositionToAdjust)
>> 
>> Why "anotherPositionToAdjust"? This makes it sound to me like we're going to adjust the text nodes around that position as well. We're only directly modifying text nodes around position. The second position is provided only to be updated with the changes, correct?
> 
> Why suggestion?

Why not just "positionToUpdate"? "Another" suggests it's not different from the first position.

>>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1283
>>> +            updatePositionForNodeRemoval(anotherPositionToAdjust, next);
>> 
>> I'm guessing we could coalesce the two updatePositionForNodeRemovals, but it's probably not worth the complexity.
> 
> We can't.

Okay :p

[Ryosuke Niwa]

Created attachment 166999 [details]
Fixed per Levi's comments

[Build Bot]

Comment on attachment 166999 [details]
Fixed per Levi's comments

Attachment 166999 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14136791

New failing tests:
fast/lists/drag-into-marker.html
fast/events/ondragenter.html

[WebKit Review Bot]

Comment on attachment 166999 [details]
Fixed per Levi's comments

Attachment 166999 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14182013

New failing tests:
editing/pasteboard/smart-drag-drop.html
editing/pasteboard/8145-3.html
editing/pasteboard/4944770-2.html
fast/forms/range/slider-delete-while-dragging-thumb.html
editing/deleting/delete-block-merge-contents-020.html
editing/execCommand/paste-2.html
editing/pasteboard/paste-line-endings-009.html
editing/pasteboard/paste-xml.xhtml
editing/execCommand/find-after-replace.html
editing/pasteboard/paste-line-endings-007.html
editing/deleting/delete-block-merge-contents-022.html
editing/pasteboard/unrendered-br.html
editing/pasteboard/5006779.html
editing/pasteboard/paste-text-009.html
editing/pasteboard/paste-line-endings-008.html
editing/pasteboard/smart-paste-007.html
editing/deleting/delete-block-merge-contents-018.html
editing/pasteboard/5028447.html
editing/pasteboard/drag-drop-modifies-page.html
editing/execCommand/paste-1.html
editing/pasteboard/merge-end-borders.html
editing/pasteboard/subframe-dragndrop-1.html
editing/pasteboard/paste-line-endings-010.html
fast/forms/range/slider-mouse-events.html
http/tests/xmlhttprequest/web-apps/001.html
editing/pasteboard/paste-match-style-001.html
editing/pasteboard/paste-text-008.html
editing/deleting/delete-block-merge-contents-021.html
editing/deleting/merge-no-br.html
editing/deleting/delete-block-merge-contents-019.html

[Levi Weintraub]

Comment on attachment 166999 [details]
Fixed per Levi's comments

I trust you'll keep the bots green :)

[Ryosuke Niwa]

Committed r130411: <http://trac.webkit.org/changeset/130411>

[Ryosuke Niwa]

Please don't roll out this patch even if 50+ tests started to fail. It's expected and we just need to rebaseline them after confirming that there is no regression.

[Levi Weintraub]

Comment on attachment 166999 [details]
Fixed per Levi's comments

Clearin' dem flags.

[Ryosuke Niwa]

Landed a build fix in http://trac.webkit.org/changeset/130429.

[Gilbie Rivas]

Thank you for perhaps the most level headed thing I have read today. That helped me a lot. I would like to share with you a great service to merge some files online. BTW, if anyone needs to merge PDF/PNG files online, I found a service here <a href="http://www.altomerge.com/" >AltoMerge</a>.

[Darin Adler]

Wow, an advertising bot thinks this is a blog.