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.
Created attachment 209078 [details] Patch
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
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
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.
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.
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.
Created attachment 209340 [details] Patch
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.
(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.
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
(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.
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
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
Created attachment 209357 [details] Patch
Comment on attachment 209357 [details] Patch Patch with rebaselines for the qt port.
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
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
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
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
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
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
Patch Landed: http://trac.webkit.org/changeset/154479