Bug 120006 – <br> does not get deleted when inlined after some non-textual content.

• Component: HTML Editing
• Status: RESOLVED FIXED

[Arpita Bahuguna]

Created attachment 209076 [details]
Test page

Fetch the attached testpage and place the cursor at the start of the second line, right before "text2".
Now press backspace (delete backward) so that the two lines merge.

Notice that the lines do not merge and the back-delete does not have any effect.

[Arpita Bahuguna]

Created attachment 209078 [details]
Patch

[Build Bot]

Comment on attachment 209078 [details]
Patch

Attachment 209078 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1478937

New failing tests:
editing/deleting/delete-inline-br.html

[Build Bot]

Created attachment 209101 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Andy Estes]

Comment on attachment 209078 [details]
Patch

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

> Source/WebCore/editing/DeleteSelectionCommand.cpp:323
> -    if (upstreamStartIsBR && downstreamStartIsBR && !(isStartOfBlock(positionBeforeNode(nodeAfterUpstreamStart)) && isEndOfBlock(positionAfterNode(nodeAfterUpstreamStart)))) {
> +    if (upstreamStartIsBR && downstreamStartIsBR && !(isStartOfBlock(positionBeforeNode(nodeAfterUpstreamStart)) && isEndOfBlock(positionAfterNode(nodeAfterDownstreamStart))) && (!nodeAfterUpstreamEnd || !(nodeAfterUpstreamEnd->previousSibling() == nodeAfterUpstreamStart))) {

Just a drive-by comment: I think !(nodeAfterUpstreamEnd->previousSibling() == nodeAfterUpstreamStart) should be nodeAfterUpstreamEnd->previousSibling() != nodeAfterUpstreamStart instead.

[Ryosuke Niwa]

Comment on attachment 209078 [details]
Patch

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

> Source/WebCore/ChangeLog:24
> +        Added a check to verify that the inline <br> is not on an empty line if
> +        the end node's previous sibling is the <br> element.

I don't understand this comment.  If end node is a br and its previous sibling is a br, then it IS an empty line.

>> Source/WebCore/editing/DeleteSelectionCommand.cpp:323
>> +    if (upstreamStartIsBR && downstreamStartIsBR && !(isStartOfBlock(positionBeforeNode(nodeAfterUpstreamStart)) && isEndOfBlock(positionAfterNode(nodeAfterDownstreamStart))) && (!nodeAfterUpstreamEnd || !(nodeAfterUpstreamEnd->previousSibling() == nodeAfterUpstreamStart))) {
> 
> Just a drive-by comment: I think !(nodeAfterUpstreamEnd->previousSibling() == nodeAfterUpstreamStart) should be nodeAfterUpstreamEnd->previousSibling() != nodeAfterUpstreamStart instead.

I don't think this fix is correct. What we want to detect here is if the entire line consists of br or not.

[Ryosuke Niwa]

Comment on attachment 209078 [details]
Patch

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

> LayoutTests/editing/deleting/delete-inline-br.html:15
> +function editingTest() {
> +    for (i = 0; i < 7; i++) {
> +        moveSelectionForwardByCharacterCommand();
> +    }
> +    deleteCommand();
> +}
> +</script>

This should be a dump-as-markup test.  There is no need for this to be a pixel test.  r- because of this.

> LayoutTests/platform/efl/TestExpectations:1782
> +# Needs to be rebaselined after landing patch for 120006
> +# https://bugs.webkit.org/show_bug.cgi?id=120006
> +webkit.org/b/120006 editing/deleting/delete-br-004.html [ Failure ]
> +webkit.org/b/120006 editing/deleting/delete-br-005.html [ Failure ]
> +webkit.org/b/120006 editing/deleting/delete-br-006.html [ Failure ]

Please don't add these test expectations. Instead, land this patch and rebaseline them after the fact.

[Arpita Bahuguna]

Created attachment 209340 [details]
Patch

[Arpita Bahuguna]

Comment on attachment 209078 [details]
Patch

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

>> Source/WebCore/ChangeLog:24
>> +        the end node's previous sibling is the <br> element.
> 
> I don't understand this comment.  If end node is a br and its previous sibling is a br, then it IS an empty line.

The end node is not a <br> is this case. Have now added an explicit check for the same.

>>> Source/WebCore/editing/DeleteSelectionCommand.cpp:323
>>> +    if (upstreamStartIsBR && downstreamStartIsBR && !(isStartOfBlock(positionBeforeNode(nodeAfterUpstreamStart)) && isEndOfBlock(positionAfterNode(nodeAfterDownstreamStart))) && (!nodeAfterUpstreamEnd || !(nodeAfterUpstreamEnd->previousSibling() == nodeAfterUpstreamStart))) {
>> 
>> Just a drive-by comment: I think !(nodeAfterUpstreamEnd->previousSibling() == nodeAfterUpstreamStart) should be nodeAfterUpstreamEnd->previousSibling() != nodeAfterUpstreamStart instead.
> 
> I don't think this fix is correct. What we want to detect here is if the entire line consists of br or not.

Yes, so if the end node's previous sibling is the <br> element and we know that the <br> is inline and the end node is not another <br> element (check added now) then we can assume that the <br> is not on an empty line. (?)

A similar check is done in the code before this where:
 bool isBROnLineByItself = upstreamStartIsBR && downstreamStartIsBR && ((nodeAfterDownstreamStart == nodeAfterUpstreamEnd) || (nodeAfterUpstreamEnd && nodeAfterUpstreamEnd->hasTagName(brTag) && nodeAfterUpstreamStart->nextSibling() == nodeAfterUpstreamEnd));
This particularly checks for the condition if we have a <br> following another <br> (<br><br>) and has the check nodeAfterUpstreamStart->nextSibling() == nodeAfterUpstreamEnd.

Even for a case like:
<div contenteditable="true">
<span>text1<input type="text"/><br></span>text2
</div>
In this case both upstreamStart and downstreamStart will point to the <br> element and upstreamEnd will be NULL. So it will set m_startsAtEmptyLine (since my condition checks for !nodeAfterUpstreamEnd).

>> LayoutTests/editing/deleting/delete-inline-br.html:15
>> +</script>
> 
> This should be a dump-as-markup test.  There is no need for this to be a pixel test.  r- because of this.

Have changed the layout testcase.

>> LayoutTests/platform/efl/TestExpectations:1782
>> +webkit.org/b/120006 editing/deleting/delete-br-006.html [ Failure ]
> 
> Please don't add these test expectations. Instead, land this patch and rebaseline them after the fact.

Removed these from the next patch. Shall rebaseline the same after landing the patch.

[Arpita Bahuguna]

(In reply to comment #6)
> (From update of attachment 209078 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209078&action=review
> 
Thanks for the review Ryosuke. Have uploaded another patch as per the review comments.

> > LayoutTests/editing/deleting/delete-inline-br.html:15
> > +function editingTest() {
> > +    for (i = 0; i < 7; i++) {
> > +        moveSelectionForwardByCharacterCommand();
> > +    }
> > +    deleteCommand();
> > +}
> > +</script>
> 
> This should be a dump-as-markup test.  There is no need for this to be a pixel test.  r- because of this.
> 
> > LayoutTests/platform/efl/TestExpectations:1782
> > +# Needs to be rebaselined after landing patch for 120006
> > +# https://bugs.webkit.org/show_bug.cgi?id=120006
> > +webkit.org/b/120006 editing/deleting/delete-br-004.html [ Failure ]
> > +webkit.org/b/120006 editing/deleting/delete-br-005.html [ Failure ]
> > +webkit.org/b/120006 editing/deleting/delete-br-006.html [ Failure ]
> 
> Please don't add these test expectations. Instead, land this patch and rebaseline them after the fact.

[Arpita Bahuguna]

Comment on attachment 209340 [details]
Patch

Expected failures:
editing/deleting/delete-br-004.html
editing/deleting/delete-br-005.html
editing/deleting/delete-br-006.html

[Arpita Bahuguna]

(In reply to comment #4)
> (From update of attachment 209078 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209078&action=review
> 
Thanks for the comment Andy. Have made the suggested change.

> > Source/WebCore/editing/DeleteSelectionCommand.cpp:323
> > -    if (upstreamStartIsBR && downstreamStartIsBR && !(isStartOfBlock(positionBeforeNode(nodeAfterUpstreamStart)) && isEndOfBlock(positionAfterNode(nodeAfterUpstreamStart)))) {
> > +    if (upstreamStartIsBR && downstreamStartIsBR && !(isStartOfBlock(positionBeforeNode(nodeAfterUpstreamStart)) && isEndOfBlock(positionAfterNode(nodeAfterDownstreamStart))) && (!nodeAfterUpstreamEnd || !(nodeAfterUpstreamEnd->previousSibling() == nodeAfterUpstreamStart))) {
> 
> Just a drive-by comment: I think !(nodeAfterUpstreamEnd->previousSibling() == nodeAfterUpstreamStart) should be nodeAfterUpstreamEnd->previousSibling() != nodeAfterUpstreamStart instead.

[Build Bot]

Comment on attachment 209340 [details]
Patch

Attachment 209340 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1426065

New failing tests:
editing/deleting/delete-br-005.html
editing/deleting/delete-br-006.html
editing/deleting/delete-br-004.html

[Build Bot]

Created attachment 209351 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Arpita Bahuguna]

Created attachment 209357 [details]
Patch

[Arpita Bahuguna]

Comment on attachment 209357 [details]
Patch

Patch with rebaselines for the qt port.

[Build Bot]

Comment on attachment 209357 [details]
Patch

Attachment 209357 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1545133

New failing tests:
editing/deleting/delete-br-005.html
editing/deleting/delete-br-006.html
editing/deleting/delete-br-004.html

[Build Bot]

Created attachment 209364 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Build Bot]

Comment on attachment 209357 [details]
Patch

Attachment 209357 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1380576

New failing tests:
editing/deleting/delete-br-005.html
editing/deleting/delete-br-006.html
editing/deleting/delete-br-004.html

[Build Bot]

Created attachment 209370 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Build Bot]

Comment on attachment 209357 [details]
Patch

Attachment 209357 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1528339

New failing tests:
editing/deleting/delete-br-005.html
editing/deleting/delete-br-006.html
editing/deleting/delete-br-004.html

[Build Bot]

Created attachment 209393 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Arpita Bahuguna]

Patch Landed: http://trac.webkit.org/changeset/154479