Handle special case of merging lists in mergeParagraphs()

Handle special case of merging lists in mergeParagraphs() https://bugs.webkit.org/show_bug.cgi?id=248709 Reviewed by Ryosuke Niwa. This patch to align Webkit behavior with Gecko / Firefox and Blink / Chromium by merging these two patches from Blink: > https://src.chromium.org/viewvc/blink?view=revision&revision=165218 > https://src.chromium.org/viewvc/blink?view=revision&revision=172941 Consider a html content with two consecutive lists. When cursor is placed at the end of first list and forward delete is executed, we should merge the second list with the first one. Currently, the content of the first <li> of second list is appended to the content of last <li> of first list and two lists are still retained. This is incorrect. Uploaded patch fixes this erroneous behavior. This issue is reproduced only when list is the only visible element present. In this case, visible positions after & before the list's parent node would be NULL and hence are treated as equal. Because of this, isVisiblyAdjacent() and therefore canMergeLists() would return true. This results in invoking mergeIdenticalElements where things go wrong as we try to delete the second list and add its content to first list, while we actually have only one list and should be merging the list items of the same list. In canMergeLists(), calling isVisiblyAdjacent(positionInParentAfterNode(*firstList), positionInParentBeforeNode(*secondList) makes sense only if firstList & secondList are different lists. Hence, before invoking isVisiblyAdjacent() we need to check if the lists are identical in which case return false from canMergeLists(). * Source/WebCore/editing/DeleteSelectionCommand.cpp: (DeleteSelectionCommand:mergePargraph): Add logic to merge list aligned with other browsers * LayoutTests/editing/deleting/merge-lists.html: Add Test Case * LayoutTests/editing/deleting/merge-lists-expected.txt: Add Test Case Expectation * LayoutTests/editing/deleting/merge-list-items-in-same-list.html: Add Test Case * LayoutTests/editing/deleting/merge-list-items-in-same-list-expected.txt: Add Test Case Expectation Canonical link: https://commits.webkit.org/257650@main
WebKit · Dec 9, 2022 · 31be36f · 31be36f
1 parent d98e3ab
commit 31be36f
Show file tree

Hide file tree

Showing 5 changed files with 73 additions and 1 deletion.
diff --git a/LayoutTests/editing/deleting/merge-list-items-in-same-list-expected.txt b/LayoutTests/editing/deleting/merge-list-items-in-same-list-expected.txt
@@ -0,0 +1,9 @@
+| "\n"
+| <ul>
+|   contenteditable=""
+|   <li>
+|     "one"
+|   <li>
+|     "<#selection-caret>three"
+|     <br>
+| "\n"
diff --git a/LayoutTests/editing/deleting/merge-list-items-in-same-list.html b/LayoutTests/editing/deleting/merge-list-items-in-same-list.html
@@ -0,0 +1,16 @@
+<!DOCTYPE html>
+<html>
+<body>
+<div>
+<ul contenteditable><li>one</li><li></li><li id="li">three</li></ul>
+</div>
+<script src="../../resources/dump-as-markup.js"></script>
+<script>
+var li = document.getElementById('li');
+var selection = window.getSelection();
+selection.collapse(li, 0);
+document.execCommand('Delete');
+Markup.dump(document.querySelector('div'));
+</script>
+</body>
+</html>
diff --git a/LayoutTests/editing/deleting/merge-lists-expected.txt b/LayoutTests/editing/deleting/merge-lists-expected.txt
@@ -0,0 +1,17 @@
+Placing cursor at the end of first list and executing forward delete should merge the second list with the first one.
+| "\n"
+| <ol>
+|   <li>
+|     "one"
+|   <li>
+|     "two"
+|   <li>
+|     id="li"
+|     "three<#selection-caret>"
+|   <li>
+|     "four"
+|   <li>
+|     "five"
+|   <li>
+|     "six"
+| "\n"
diff --git a/LayoutTests/editing/deleting/merge-lists.html b/LayoutTests/editing/deleting/merge-lists.html
@@ -0,0 +1,19 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p id="description">Placing cursor at the end of first list and executing forward delete should merge the second list with the first one.</p>
+<div contenteditable>
+<ol><li>one</li><li>two</li><li id="li">three</li></ol>
+<ol><li>four</li><li>five</li><li>six</li></ol>
+</div>
+<script src="../../resources/dump-as-markup.js"></script>
+<script>
+var li = document.getElementById("li");
+var selection = window.getSelection();
+selection.collapse(li, li.childNodes.length);
+document.execCommand("forwardDelete");
+Markup.description(document.getElementById('description').textContent);
+Markup.dump(document.querySelector('div'));
+</script>
+</body>
+</html>
diff --git a/Source/WebCore/editing/DeleteSelectionCommand.cpp b/Source/WebCore/editing/DeleteSelectionCommand.cpp
@@ -1,5 +1,6 @@
 /*
- * Copyright (C) 2005 Apple Inc.  All rights reserved.
+ * Copyright (C) 2005-2022 Apple Inc.  All rights reserved.
+ * Copyright (C) 2014 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -815,6 +816,16 @@ void DeleteSelectionCommand::mergeParagraphs()
     if (mergeDestination == endOfParagraphToMove)
         return;
 
+    // If the merge destination and source to be moved are both list items of different lists, merge them into single list.
+    auto* listItemInFirstParagraph = enclosingNodeOfType(m_upstreamStart, isListItem);
+    auto* listItemInSecondParagraph = enclosingNodeOfType(m_downstreamEnd, isListItem);
+    if (listItemInFirstParagraph && listItemInSecondParagraph && listItemInFirstParagraph->parentElement() != listItemInSecondParagraph->parentElement()
+    && canMergeLists(listItemInFirstParagraph->parentElement(), listItemInSecondParagraph->parentElement())) {
+        mergeIdenticalElements(*listItemInFirstParagraph->parentElement(), *listItemInSecondParagraph->parentElement());
+        m_endingPosition = mergeDestination.deepEquivalent();
+        return;
+    }
+
     // The rule for merging into an empty block is: only do so if its farther to the right.
     // FIXME: Consider RTL.
     if (!m_startsAtEmptyLine && isStartOfParagraph(mergeDestination) && startOfParagraphToMove.absoluteCaretBounds().x() > mergeDestination.absoluteCaretBounds().x()) {