Bug 87428 – Indenting a paragraph that begins with a link 3 times breaks the paragraph into two paragraphs

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

[Shezan Baig]

Created attachment 143892 [details]
simple test case

The expected result is that the paragraph is indented three times, and remains as a single paragraph.
Actual result: the text after the link is moved to a second paragraph.

[Shezan Baig]

Created attachment 144070 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 144070 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Indenting a paragraph that begins with an inline element places the nextSibling incorrectly

I don't understand what "inline element places the nextSibling incorrectly" means.
Please rename the bug title to something more descriptive as well as the bug title in the change log.

> Source/WebCore/ChangeLog:9
> +        When traverseNextSibling moves up to a new parent, we need to move
> +        lastNode up the same number of times that traverseNextSibling moved up.

Why do we "need to move lastNode up the same number of times that traverseNextSibling moved up"?

> Source/WebCore/editing/CompositeEditCommand.cpp:1008
> +        for (Node* s = start.deprecatedNode(), *n = s->traverseNextSibling(outerNode.get()); n; n = n->traverseNextSibling(outerNode.get())) {

Please don't use one-letter variable like s and n. I'd call them startNode and node respectively.
Also, I'd prefer declaring startNode outside of the for loop.

> Source/WebCore/editing/CompositeEditCommand.cpp:1013
> +            // If traverseNextSibling required moving up to a new parent, then
> +            // n->parent() will be different from s->parent(). We need to move
> +            // lastNode (our insertion point) up accordingly until n->parent()
> +            // is the same as s->parent() (i.e, until n and s are direct
> +            // siblings).

This is a "what" comment, meaning that it just describes what the code does.
We need a "why" comment instead, which explains why we need to do this.

[Shezan Baig]

Comment on attachment 144070 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        Indenting a paragraph that begins with an inline element places the nextSibling incorrectly
> 
> I don't understand what "inline element places the nextSibling incorrectly" means.
> Please rename the bug title to something more descriptive as well as the bug title in the change log.

I've renamed the bug title to describe the actual symptom of the bug.

>> Source/WebCore/ChangeLog:9
>> +        lastNode up the same number of times that traverseNextSibling moved up.
> 
> Why do we "need to move lastNode up the same number of times that traverseNextSibling moved up"?

This is so that the relative depth between the next sibling and the start node is maintained in the clone.  I'll make the next patch say this better.

>> Source/WebCore/editing/CompositeEditCommand.cpp:1008
>> +        for (Node* s = start.deprecatedNode(), *n = s->traverseNextSibling(outerNode.get()); n; n = n->traverseNextSibling(outerNode.get())) {
> 
> Please don't use one-letter variable like s and n. I'd call them startNode and node respectively.
> Also, I'd prefer declaring startNode outside of the for loop.

Yeah, I wasn't sure if I should be renaming the existing variables in the same patch as the bugfix, but I'll do this in the upcoming patch.

>> Source/WebCore/editing/CompositeEditCommand.cpp:1013
>> +            // siblings).
> 
> This is a "what" comment, meaning that it just describes what the code does.
> We need a "why" comment instead, which explains why we need to do this.

I've reworded this comment in the next patch.

[Shezan Baig]

Created attachment 145317 [details]
Patch (with change from comment 2)

[Ryosuke Niwa]

Comment on attachment 145317 [details]
Patch (with change from comment 2)

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

Please consider addressing the comments below before landing it.

> Source/WebCore/ChangeLog:11
> +        Fix the way lastNode (our insertion point) is updated whenever
> +        traverseNextSibling moves up to a new parent, so that the relative
> +        depth between the next sibling and the original start node is
> +        maintained in the clone.

Thanks for the revision. This description is much more helpful. However, it would be ideal if you could also explain why the divergence in the depth leads to breaking a single paragraph into two paragraphs.

> Source/WebCore/editing/CompositeEditCommand.cpp:1019
> +            // If traverseNextSibling required moving up to a new parent, this
> +            // means that node is not a true sibling of startNode, i.e.
> +            // node->parent() is not the same as startNode->parent(). We need
> +            // to adjust lastNode (our insertion point) so that the relative
> +            // depth between clonedNode (below) and the clone of
> +            // start->deprecatedNode() will be the same as the relative depth
> +            // between node and start->deprecatedNode(). We do this by moving
> +            // lastNode up the same number of times that traverseNextSibling
> +            // moved up, i.e. until node->parent() is the same as
> +            // startNode->parent().

This comment is very verbose. A single line comment like "Move lastNode up in the tree as much as node was moved up in the tree by traverseNextSibling" would do.

[Ryosuke Niwa]

Comment on attachment 145317 [details]
Patch (with change from comment 2)

Oh, I just realize you're not a committer yet. I guess I'll cq- then.

[Shezan Baig]

Comment on attachment 145317 [details]
Patch (with change from comment 2)

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

>> Source/WebCore/ChangeLog:11
>> +        maintained in the clone.
> 
> Thanks for the revision. This description is much more helpful. However, it would be ideal if you could also explain why the divergence in the depth leads to breaking a single paragraph into two paragraphs.

Thanks, I'm adding this in the new patch.

>> Source/WebCore/editing/CompositeEditCommand.cpp:1019
>> +            // startNode->parent().
> 
> This comment is very verbose. A single line comment like "Move lastNode up in the tree as much as node was moved up in the tree by traverseNextSibling" would do.

That's much better, thanks :)  However, I also think its important to say why lastNode needs to move up, so it will be just a little more than one line :)

[Shezan Baig]

Created attachment 145345 [details]
Patch (with change from comment 5)

[Ryosuke Niwa]

Comment on attachment 145345 [details]
Patch (with change from comment 5)

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

> Source/WebCore/ChangeLog:13
> +        maintained in the clone. The divergence in depth broke the paragraph
> +        into two paragraphs because the next sibling was inserted outside the
> +        blockquote that was created for the indentation.

Perfect! In general, change log is a good place to describe what caused a bug and how you fixed it.
It'll allow reviewers to understand the problem and assess the correctness of your fix with ease.
It also helps someone else (or ourselves) needing understand your code in the future :).

[WebKit Review Bot]

Comment on attachment 145345 [details]
Patch (with change from comment 5)

Clearing flags on attachment: 145345

Committed r119270: <http://trac.webkit.org/changeset/119270>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.