Elements in a table are incorrectly selected in JavaScript.

https://bugs.webkit.org/show_bug.cgi?id=220607 Reviewed by Wenson Hsieh. Source/WebCore: After the fix for https://bugs.webkit.org/show_bug.cgi?id=130844 in https://trac.webkit.org/changeset/260207/webkit a regression was found for table cell selections through JavaScript. The previous fix was for an issue in the difference between how PositionIterator and Position calculated if it was canonical (::isCandidate) in the cases where the renderer was a BR. Instead of only converging isCandidate for BR, this function in PositionIterator was completely subsumed by just calling the implementation for Position. While there are many similarities, there are important differences, especially in the table code, therefore PositionIterator::isCandidate is being reinstated, but with the convergence in the BR case only. Also, moved the check added in http://webkit.org/b/211864 so it will cover cases that were missed due to this code being wrong while that fix was added. Test: editing/selection/editable-table-cell-selection.html * dom/PositionIterator.cpp: (WebCore::PositionIterator::isCandidate const): * editing/InsertParagraphSeparatorCommand.cpp: (WebCore::InsertParagraphSeparatorCommand::doApply): LayoutTests: * editing/execCommand/null_calc_primitive_value_for_css_property.html: Added ending tag that was missing and does not affect the issue the test was verifying. * editing/inserting/insert-list-in-table-cell-07-expected.txt: Restored expected text file to original output. * editing/selection/editable-table-cell-selection-expected.txt: Added. * editing/selection/editable-table-cell-selection.html: Added. Canonical link: https://commits.webkit.org/233161@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@271635 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Jan 20, 2021 · a1fd940 · a1fd940
1 parent 8d2c409
commit a1fd940
Show file tree

Hide file tree

Showing 8 changed files with 209 additions and 9 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,17 @@
+2021-01-19  Megan Gardner  <megan_gardner@apple.com>
+
+        Elements in a table are incorrectly selected in JavaScript.
+        https://bugs.webkit.org/show_bug.cgi?id=220607
+
+        Reviewed by Wenson Hsieh.
+
+        * editing/execCommand/null_calc_primitive_value_for_css_property.html:
+        Added ending tag that was missing and does not affect the issue the test was verifying.
+        * editing/inserting/insert-list-in-table-cell-07-expected.txt:
+        Restored expected text file to original output.
+        * editing/selection/editable-table-cell-selection-expected.txt: Added.
+        * editing/selection/editable-table-cell-selection.html: Added.
+
 2021-01-19  Sergio Villar Senin  <svillar@igalia.com>
 
         REGRESSION(r266695) Range control with custom track width sized incorrectly

diff --git a/LayoutTests/editing/execCommand/null_calc_primitive_value_for_css_property.html b/LayoutTests/editing/execCommand/null_calc_primitive_value_for_css_property.html
@@ -16,7 +16,7 @@
 </script>
 
 <body onload=cssPrimitiveValTest()>
-<ins id="x">
+<ins id="x"></ins>
 
 </body>
 </html>
diff --git a/LayoutTests/editing/inserting/insert-list-in-table-cell-07-expected.txt b/LayoutTests/editing/inserting/insert-list-in-table-cell-07-expected.txt
@@ -1,7 +1,6 @@
 Exec insertOrderedList twice in all the cells of a table removes the previously inserted list items:
 
 Before:
-| <#selection-focus>
 | <table>
 |   border="1"
 |   <tbody>
@@ -17,6 +16,7 @@ Before:
 |       <td>
 |         "fsfg"
 |   <tbody>
+| <#selection-focus>
 
 After:
 | <table>
@@ -25,13 +25,16 @@ After:
 |     id="element"
 |     <tr>
 |       <td>
-|         "<#selection-caret>fsdf"
+|         "<#selection-anchor>fsdf"
 |         <br>
 |       <td>
 |         "fsdf"
+|         <br>
 |     <tr>
 |       <td>
 |         "gghfg"
+|         <br>
 |       <td>
-|         "fsfg"
+|         "fsfg<#selection-focus>"
+|         <br>
 |   <tbody>
diff --git a/LayoutTests/editing/selection/editable-table-cell-selection-expected.txt b/LayoutTests/editing/selection/editable-table-cell-selection-expected.txt
@@ -0,0 +1,11 @@
+A	B	C	D
+1A	1B	1C	1D
+2A	2B	2C	2D
+3A	3B	3C	3D
+4A	4B	4C	4D
+5A	5B	5C	5D
+PASS: Correctly Selects row in thead
+PASS: Correctly Selects last cell in thead
+PASS: Correctly Selects row in tbody
+PASS: Correctly Selects last cell in tbody
+
diff --git a/LayoutTests/editing/selection/editable-table-cell-selection.html b/LayoutTests/editing/selection/editable-table-cell-selection.html
@@ -0,0 +1,122 @@
+<!DOCTYPE html>
+
+<html>
+<head>
+    <script>
+        if (window.testRunner) {
+            testRunner.dumpAsText();
+            testRunner.waitUntilDone();
+        }
+
+        async function runTest()
+        {
+            var output = '';
+
+            function select(element) {
+                const range = document.createRange();
+
+                range.setStartBefore(element);
+                range.setEndAfter(element);
+
+                const selection = window.getSelection();
+
+                selection.removeAllRanges();
+                selection.addRange(range);
+            }
+
+            select(document.querySelector('thead tr'));
+            if (document.getSelection().toString() == "A\tB\tC\tD\n")
+                output += 'PASS: Correctly Selects row in thead';
+            else
+                output += 'FAIL: Incorrect Selection: ' + document.getSelection().toString();
+            output += '<br>';
+
+            const lastHeadCell = [ ...document.querySelectorAll('thead th') ].pop();
+            select(lastHeadCell);
+            if (document.getSelection().toString() == "D\n")
+                output += 'PASS: Correctly Selects last cell in thead';
+            else
+                output += 'FAIL: Incorrect Selection: ' + document.getSelection().toString();
+            output += '<br>';
+
+            select(document.querySelector('tbody tr'));
+            if (document.getSelection().toString() == "1A\t1B\t1C\t1D\n")
+                output += 'PASS: Correctly Selects row in tbody';
+            else
+                output += 'FAIL: Incorrect Selection: ' + document.getSelection().toString();
+            output += '<br>';
+
+            const lastBodyCell = [ ...document.querySelectorAll('tbody td') ].pop();
+            select(lastBodyCell);
+            if (document.getSelection().toString() == "5D\n")
+                output += 'PASS: Correctly Selects last cell in tbody';
+            else
+                output += 'FAIL: Incorrect Selection: ' + document.getSelection().toString();
+            output += '<br>';
+
+            document.getElementById('target').innerHTML = output;
+            testRunner.notifyDone();
+        }
+
+        window.addEventListener('load', runTest, false);
+    </script>
+        <style>
+        #target {
+            height: 50px;
+            width: 300px;
+            background-color: silver;
+            font-family: monospace;
+            font-size: 18px;
+        }
+    </style>
+    <meta name="viewport" content="width=device-width, initial-scale=1">
+</head>
+<body>
+    <div contenteditable>
+    <table border="1">
+        <thead>
+            <tr>
+                <th scope="col">A</th>
+                <th scope="col">B</th>
+                <th scope="col">C</th>
+                <th scope="col">D</th>
+            </tr>
+        </thead>
+        <tbody>
+            <tr>
+                <th scope="row">1A</th>
+                <td>1B</td>
+                <td>1C</td>
+                <td>1D</td>
+            </tr>
+            <tr>
+                <th scope="row">2A</th>
+                <td>2B</td>
+                <td>2C</td>
+                <td>2D</td>
+            </tr>
+            <tr>
+                <th scope="row">3A</th>
+                <td>3B</td>
+                <td>3C</td>
+                <td>3D</td>
+            </tr>
+            <tr>
+                <th scope="row">4A</th>
+                <td>4B</td>
+                <td>4C</td>
+                <td>4D</td>
+            </tr>
+            <tr>
+                <th scope="row">5A</th>
+                <td>5B</td>
+                <td>5C</td>
+                <td>5D</td>
+            </tr>
+        </tbody>
+    </table>
+</div>
+    <div id="target">
+    </div>
+</body>
+</html>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,25 @@
+2021-01-19  Megan Gardner  <megan_gardner@apple.com>
+
+        Elements in a table are incorrectly selected in JavaScript.
+        https://bugs.webkit.org/show_bug.cgi?id=220607
+
+        Reviewed by Wenson Hsieh.
+
+        After the fix for https://bugs.webkit.org/show_bug.cgi?id=130844 in https://trac.webkit.org/changeset/260207/webkit a regression was found for 
+        table cell selections through JavaScript. The previous fix was for an issue in the difference between how PositionIterator and Position calculated
+        if it was canonical (::isCandidate) in the cases where the renderer was a BR. Instead of only converging isCandidate for BR, this function in PositionIterator
+        was completely subsumed by just calling the implementation for Position. While there are many similarities, there are important differences, especially in the 
+        table code, therefore PositionIterator::isCandidate is being reinstated, but with the convergence in the BR case only.
+        Also, moved the check added in http://webkit.org/b/211864 so it will cover cases that were missed due to this code being wrong while that fix was added.
+
+        Test: editing/selection/editable-table-cell-selection.html
+
+        * dom/PositionIterator.cpp:
+        (WebCore::PositionIterator::isCandidate const):
+        * editing/InsertParagraphSeparatorCommand.cpp:
+        (WebCore::InsertParagraphSeparatorCommand::doApply):
+
+
 2021-01-19  Patrick Angle  <pangle@apple.com>
 
         Web Inspector: Font Details sidebar - Fractional variation axis ranges/default values are rounded.

diff --git a/Source/WebCore/dom/PositionIterator.cpp b/Source/WebCore/dom/PositionIterator.cpp
@@ -145,7 +145,35 @@ bool PositionIterator::atEndOfNode() const
 
 bool PositionIterator::isCandidate() const
 {
-    return m_anchorNode ? Position(*this).isCandidate() : false;
+    if (!m_anchorNode)
+        return false;
+
+    RenderObject* renderer = m_anchorNode->renderer();
+    if (!renderer)
+        return false;
+
+    if (renderer->style().visibility() != Visibility::Visible)
+        return false;
+
+    if (renderer->isBR())
+        return Position(*this).isCandidate();
+
+    if (is<RenderText>(*renderer))
+        return !Position::nodeIsUserSelectNone(m_anchorNode) && downcast<RenderText>(*renderer).containsCaretOffset(m_offsetInAnchor);
+
+    if (isRenderedTable(m_anchorNode) || editingIgnoresContent(*m_anchorNode))
+        return (atStartOfNode() || atEndOfNode()) && !Position::nodeIsUserSelectNone(m_anchorNode->parentNode());
+
+    if (!is<HTMLHtmlElement>(*m_anchorNode) && is<RenderBlockFlow>(*renderer)) {
+        RenderBlockFlow& block = downcast<RenderBlockFlow>(*renderer);
+        if (block.logicalHeight() || is<HTMLBodyElement>(*m_anchorNode)) {
+            if (!Position::hasRenderedNonAnonymousDescendantsWithHeight(block))
+                return atStartOfNode() && !Position::nodeIsUserSelectNone(m_anchorNode);
+            return m_anchorNode->hasEditableStyle() && !Position::nodeIsUserSelectNone(m_anchorNode) && Position(*this).atEditingBoundary();
+        }
+    }
+
+    return false;
 }
 
 } // namespace WebCore
diff --git a/Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp b/Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp
@@ -200,6 +200,10 @@ void InsertParagraphSeparatorCommand::doApply()
     bool isLastInBlock = isEndOfBlock(visiblePos);
     bool nestNewBlock = false;
 
+    // FIXME: <http://webkit.org/b/211864> If insertionPosition is not editable, we should compute a position that is.
+    if (!isEditablePosition(insertionPosition))
+        return;
+
     // Create block to be inserted.
     RefPtr<Element> blockToInsert;
     if (startBlock->isRootEditableElement()) {
@@ -298,10 +302,6 @@ void InsertParagraphSeparatorCommand::doApply()
     // it if visiblePos is at the start of a paragraph so that the 
     // content will move down a line.
     if (isStartOfParagraph(visiblePos)) {
-        // FIXME: <http://webkit.org/b/211864> If insertionPosition is not editable, we should compute a position that is.
-        if (!isEditablePosition(insertionPosition))
-            return;
-
         auto br = HTMLBRElement::create(document());
         auto* brPtr = br.ptr();
         insertNodeAt(WTFMove(br), insertionPosition);