Bug 98188 - ReplaceSelectionCommand should merge text nodes
Summary: ReplaceSelectionCommand should merge text nodes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 97266 99663
  Show dependency treegraph
 
Reported: 2012-10-02 12:17 PDT by Ryosuke Niwa
Modified: 2015-11-06 08:16 PST (History)
12 users (show)

See Also:


Attachments
work in progress (71.92 KB, patch)
2012-10-02 16:15 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (110.02 KB, patch)
2012-10-03 15:52 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed per Levi's comments (110.29 KB, patch)
2012-10-03 16:55 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2012-10-02 12:17:58 PDT
In order to resolve the bug 97266, we need to improve ReplaceSelectionCommand to merge consecutive text nodes.
Comment 1 Ryosuke Niwa 2012-10-02 16:15:36 PDT
Created attachment 166770 [details]
work in progress

There are still 12 test failures :(
Comment 2 Ryosuke Niwa 2012-10-02 16:18:12 PDT
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.
Comment 3 Ryosuke Niwa 2012-10-03 15:52:30 PDT
Created attachment 166978 [details]
Patch
Comment 4 Levi Weintraub 2012-10-03 16:32:24 PDT
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.
Comment 5 Ryosuke Niwa 2012-10-03 16:36:18 PDT
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.
Comment 6 Ryosuke Niwa 2012-10-03 16:43:26 PDT
(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...
Comment 7 Levi Weintraub 2012-10-03 16:47:10 PDT
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
Comment 8 Ryosuke Niwa 2012-10-03 16:55:13 PDT
Created attachment 166999 [details]
Fixed per Levi's comments
Comment 9 Build Bot 2012-10-03 17:31:52 PDT
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
Comment 10 WebKit Review Bot 2012-10-04 08:46:42 PDT
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
Comment 11 Levi Weintraub 2012-10-04 09:19:08 PDT
Comment on attachment 166999 [details]
Fixed per Levi's comments

I trust you'll keep the bots green :)
Comment 12 Ryosuke Niwa 2012-10-04 11:36:25 PDT
Committed r130411: <http://trac.webkit.org/changeset/130411>
Comment 13 Ryosuke Niwa 2012-10-04 11:56:50 PDT
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.
Comment 14 Levi Weintraub 2012-10-04 15:00:30 PDT
Comment on attachment 166999 [details]
Fixed per Levi's comments

Clearin' dem flags.
Comment 15 Ryosuke Niwa 2012-10-04 17:41:06 PDT
Landed a build fix in http://trac.webkit.org/changeset/130429.
Comment 16 Gilbie Rivas 2015-11-06 04:28:02 PST
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>.
Comment 17 Darin Adler 2015-11-06 08:16:57 PST
Wow, an advertising bot thinks this is a blog.