Delete content of a single cell table should not delete the whole table
authorjfernandez@igalia.com <jfernandez@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 17 Jul 2018 08:48:49 +0000 (08:48 +0000)
committerjfernandez@igalia.com <jfernandez@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 17 Jul 2018 08:48:49 +0000 (08:48 +0000)
https://bugs.webkit.org/show_bug.cgi?id=173117

Reviewed by Ryosuke Niwa.

Source/WebCore:

We should not extend selection looking for special elements if the
delete operation has been triggered by a caret based selection.

This change is based on a recent [1] resolution of the Editing TF,
which acknowledges that behavior of single-cell tables must be the
same that multi-cell tables and even if the last character is
deleted, we should not delete the whole table structure.

A different case would be when the user actively selects the whole
content of a table; in this case, as we do in multi-cell tables,
the structure of single-cell tables should be deleted together
with the content.

[1] https://github.com/w3c/editing/issues/163

Tests: editing/deleting/backspace-delete-last-char-in-table.html
       editing/deleting/forward-delete-last-char-in-table.html
       editing/deleting/select-and-delete-last-char-in-table.html

* editing/TypingCommand.cpp:
(WebCore::TypingCommand::deleteKeyPressed):
(WebCore::TypingCommand::forwardDeleteKeyPressed):

LayoutTests:

Tests to verify that single-cell tables are not deleted when their
last character is deleted, unless it was previously selected by
the user.

Changes two expected files to adapt them to the new logic.

* LayoutTests/editing/deleting/deleting-relative-positioned-special-element-expected.txt: The paragraph is not deleted, even if it's empty. The paragraphs above are not merged, which was the goal of the test.
* editing/deleting/delete-last-char-in-table-expected.txt: The table is not removed, even if it's empty. The formatted elements are deleted, which was the goal of the test.
* editing/deleting/backspace-delete-last-char-in-table-expected.txt: Added.
* editing/deleting/backspace-delete-last-char-in-table.html: Added.
* editing/deleting/forward-delete-last-char-in-table-expected.txt: Added.
* editing/deleting/forward-delete-last-char-in-table.html: Added.
* editing/deleting/select-and-delete-last-char-in-table-expected.txt: Added.
* editing/deleting/select-and-delete-last-char-in-table.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@233885 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/editing/deleting/backspace-delete-last-char-in-table-expected.txt [new file with mode: 0644]
LayoutTests/editing/deleting/backspace-delete-last-char-in-table.html [new file with mode: 0644]
LayoutTests/editing/deleting/delete-last-char-in-table-expected.txt
LayoutTests/editing/deleting/deleting-relative-positioned-special-element-expected.txt
LayoutTests/editing/deleting/forward-delete-last-char-in-table-expected.txt [new file with mode: 0644]
LayoutTests/editing/deleting/forward-delete-last-char-in-table.html [new file with mode: 0644]
LayoutTests/editing/deleting/select-and-delete-last-char-in-table-expected.txt [new file with mode: 0644]
LayoutTests/editing/deleting/select-and-delete-last-char-in-table.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/editing/TypingCommand.cpp

index dad2eb7..e0d1240 100644 (file)
@@ -1,3 +1,25 @@
+2018-07-17  Javier Fernandez  <jfernandez@igalia.com>
+
+        Delete content of a single cell table should not delete the whole table
+        https://bugs.webkit.org/show_bug.cgi?id=173117
+
+        Reviewed by Ryosuke Niwa.
+
+        Tests to verify that single-cell tables are not deleted when their
+        last character is deleted, unless it was previously selected by
+        the user.
+
+        Changes two expected files to adapt them to the new logic.
+
+        * LayoutTests/editing/deleting/deleting-relative-positioned-special-element-expected.txt: The paragraph is not deleted, even if it's empty. The paragraphs above are not merged, which was the goal of the test.
+        * editing/deleting/delete-last-char-in-table-expected.txt: The table is not removed, even if it's empty. The formatted elements are deleted, which was the goal of the test.
+        * editing/deleting/backspace-delete-last-char-in-table-expected.txt: Added.
+        * editing/deleting/backspace-delete-last-char-in-table.html: Added.
+        * editing/deleting/forward-delete-last-char-in-table-expected.txt: Added.
+        * editing/deleting/forward-delete-last-char-in-table.html: Added.
+        * editing/deleting/select-and-delete-last-char-in-table-expected.txt: Added.
+        * editing/deleting/select-and-delete-last-char-in-table.html: Added.
+
 2018-07-16  Simon Fraser  <simon.fraser@apple.com>
 
         Roll out r233873 and r233875 since they caused 8 new layout test crashes.
diff --git a/LayoutTests/editing/deleting/backspace-delete-last-char-in-table-expected.txt b/LayoutTests/editing/deleting/backspace-delete-last-char-in-table-expected.txt
new file mode 100644 (file)
index 0000000..5d69d00
--- /dev/null
@@ -0,0 +1,24 @@
+In this test the last character of a single-cell table is deleted, just using the backspace key. Only the table cell's content should be deleted, and not the table itself.
+
+BeforeDeletion:
+| "First"
+| <table>
+|   border="1"
+|   <tbody>
+|     <tr>
+|       <td>
+|         id="test"
+|         "1<#selection-caret>"
+| "Second"
+
+AfterDeletion:
+| "First"
+| <table>
+|   border="1"
+|   <tbody>
+|     <tr>
+|       <td>
+|         id="test"
+|         <#selection-caret>
+|         <br>
+| "Second"
diff --git a/LayoutTests/editing/deleting/backspace-delete-last-char-in-table.html b/LayoutTests/editing/deleting/backspace-delete-last-char-in-table.html
new file mode 100644 (file)
index 0000000..e1da944
--- /dev/null
@@ -0,0 +1,14 @@
+<!doctype html>
+<script src=../editing.js ></script>
+<script src="../../resources/dump-as-markup.js"></script>
+<div contenteditable id="root">First<table border="1"><tr><td id="test">1</td></tr></table>Second</div>
+<script>
+Markup.description("In this test the last character of a single-cell table is deleted, just using the backspace key. Only the table cell's content should be deleted, and not the table itself.")
+
+const element = document.getElementById("test");
+getSelection().collapse(element, 1);
+Markup.dump('root', 'BeforeDeletion');
+
+deleteCommand();
+Markup.dump('root', 'AfterDeletion');
+</script>
index e78d010..96847f2 100644 (file)
@@ -2,4 +2,4 @@ See bug 57148. When deleteing the last character in a table deletes the table, n
 
 
 PASS
-execDeleteCommand: <br>
+execDeleteCommand: <table style="border-collapse:collapse"><tbody><tr><td id="cursor"><br></td></tr></tbody></table>
index b67b334..3bbd47c 100644 (file)
@@ -14,8 +14,10 @@ BeforeDeletion:
 AfterDeletion:
 | <p>
 |   "1"
-| <#selection-caret>
-| <br>
+| <p>
+|   id="paragraphToDelete"
+|   <#selection-caret>
+|   <br>
 | <p>
 |   "3"
 | <p>
diff --git a/LayoutTests/editing/deleting/forward-delete-last-char-in-table-expected.txt b/LayoutTests/editing/deleting/forward-delete-last-char-in-table-expected.txt
new file mode 100644 (file)
index 0000000..430fa2a
--- /dev/null
@@ -0,0 +1,23 @@
+In this test the last character of a single-cell table is deleted, just using the forwardDelete key. Only the table cell's content should be deleted, and not the table itself.
+
+BeforeDeletion:
+| "First"
+| <table>
+|   border="1"
+|   <tbody>
+|     <tr>
+|       <td>
+|         id="test"
+|         "1<#selection-caret>"
+| "Second"
+
+AfterDeletion:
+| "First"
+| <table>
+|   border="1"
+|   <tbody>
+|     <tr>
+|       <td>
+|         id="test"
+|         "1<#selection-caret>"
+| "Second"
diff --git a/LayoutTests/editing/deleting/forward-delete-last-char-in-table.html b/LayoutTests/editing/deleting/forward-delete-last-char-in-table.html
new file mode 100644 (file)
index 0000000..aa114c8
--- /dev/null
@@ -0,0 +1,14 @@
+<!doctype html>
+<script src=../editing.js ></script>
+<script src="../../resources/dump-as-markup.js"></script>
+<div contenteditable id="root">First<table border="1"><tr><td id="test">1</td></tr></table>Second</div>
+<script>
+Markup.description("In this test the last character of a single-cell table is deleted, just using the forwardDelete key. Only the table cell's content should be deleted, and not the table itself.")
+
+const element = document.getElementById("test");
+getSelection().collapse(element, 1);
+Markup.dump('root', 'BeforeDeletion');
+
+forwardDeleteCommand();
+Markup.dump('root', 'AfterDeletion');
+</script>
diff --git a/LayoutTests/editing/deleting/select-and-delete-last-char-in-table-expected.txt b/LayoutTests/editing/deleting/select-and-delete-last-char-in-table-expected.txt
new file mode 100644 (file)
index 0000000..8d40053
--- /dev/null
@@ -0,0 +1,17 @@
+In this test the last character of a single-cell table is selected and then deleted using the backspace key. The whole table is deleted.
+
+BeforeDeletion:
+| "First"
+| <table>
+|   border="1"
+|   <tbody>
+|     <tr>
+|       <td>
+|         id="test"
+|         "<#selection-anchor>1<#selection-focus>"
+| "Second"
+
+AfterDeletion:
+| "First<#selection-caret>"
+| <br>
+| "Second"
diff --git a/LayoutTests/editing/deleting/select-and-delete-last-char-in-table.html b/LayoutTests/editing/deleting/select-and-delete-last-char-in-table.html
new file mode 100644 (file)
index 0000000..16fee35
--- /dev/null
@@ -0,0 +1,15 @@
+<!doctype html>
+<script src=../editing.js ></script>
+<script src="../../resources/dump-as-markup.js"></script>
+<div contenteditable id="root">First<table border="1"><tr><td id="test">1</td></tr></table>Second</div>
+<script>
+Markup.description("In this test the last character of a single-cell table is selected and then deleted using the backspace key. The whole table is deleted.")
+
+const element = document.getElementById("test");
+getSelection().collapse(element, 0);
+extendSelectionForwardByCharacterCommand();
+Markup.dump('root', 'BeforeDeletion');
+
+deleteCommand();
+Markup.dump('root', 'AfterDeletion');
+</script>
index af8f7ab..31d84d7 100644 (file)
@@ -1,3 +1,33 @@
+2018-07-17  Javier Fernandez  <jfernandez@igalia.com>
+
+        Delete content of a single cell table should not delete the whole table
+        https://bugs.webkit.org/show_bug.cgi?id=173117
+
+        Reviewed by Ryosuke Niwa.
+
+        We should not extend selection looking for special elements if the
+        delete operation has been triggered by a caret based selection.
+
+        This change is based on a recent [1] resolution of the Editing TF,
+        which acknowledges that behavior of single-cell tables must be the
+        same that multi-cell tables and even if the last character is
+        deleted, we should not delete the whole table structure.
+
+        A different case would be when the user actively selects the whole
+        content of a table; in this case, as we do in multi-cell tables,
+        the structure of single-cell tables should be deleted together
+        with the content.
+
+        [1] https://github.com/w3c/editing/issues/163
+
+        Tests: editing/deleting/backspace-delete-last-char-in-table.html
+               editing/deleting/forward-delete-last-char-in-table.html
+               editing/deleting/select-and-delete-last-char-in-table.html
+
+        * editing/TypingCommand.cpp:
+        (WebCore::TypingCommand::deleteKeyPressed):
+        (WebCore::TypingCommand::forwardDeleteKeyPressed):
+
 2018-07-16  Megan Gardner  <megan_gardner@apple.com>
 
         Correctly adjust scroll offsets when a page is zoomed
index fd79066..b94df5a 100644 (file)
@@ -649,6 +649,7 @@ void TypingCommand::deleteKeyPressed(TextGranularity granularity, bool shouldAdd
 
     VisibleSelection selectionToDelete;
     VisibleSelection selectionAfterUndo;
+    bool expandForSpecialElements = !endingSelection().isCaret();
 
     switch (endingSelection().selectionType()) {
     case VisibleSelection::RangeSelection:
@@ -760,7 +761,7 @@ void TypingCommand::deleteKeyPressed(TextGranularity granularity, bool shouldAdd
     // more text than you insert.  In that case all of the text that was around originally should be selected.
     if (m_openedByBackwardDelete)
         setStartingSelection(selectionAfterUndo);
-    CompositeEditCommand::deleteSelection(selectionToDelete, m_smartDelete);
+    CompositeEditCommand::deleteSelection(selectionToDelete, m_smartDelete, /* mergeBlocksAfterDelete*/ true, /* replace*/ false, expandForSpecialElements, /*sanitizeMarkup*/ true);
     setSmartDelete(false);
     typingAddedToOpenCommand(DeleteKey);
 }
@@ -774,6 +775,7 @@ void TypingCommand::forwardDeleteKeyPressed(TextGranularity granularity, bool sh
 
     VisibleSelection selectionToDelete;
     VisibleSelection selectionAfterUndo;
+    bool expandForSpecialElements = !endingSelection().isCaret();
 
     switch (endingSelection().selectionType()) {
     case VisibleSelection::RangeSelection:
@@ -862,7 +864,7 @@ void TypingCommand::forwardDeleteKeyPressed(TextGranularity granularity, bool sh
         frame.editor().addRangeToKillRing(*selectionToDelete.toNormalizedRange().get(), Editor::KillRingInsertionMode::AppendText);
     // make undo select what was deleted
     setStartingSelection(selectionAfterUndo);
-    CompositeEditCommand::deleteSelection(selectionToDelete, m_smartDelete);
+    CompositeEditCommand::deleteSelection(selectionToDelete, m_smartDelete, /* mergeBlocksAfterDelete*/ true, /* replace*/ false, expandForSpecialElements, /*sanitizeMarkup*/ true);
     setSmartDelete(false);
     typingAddedToOpenCommand(ForwardDeleteKey);
 }