REGRESSION (r212693): getClientRects(), getBoundingClientRect() for range that spans...
authordbates@webkit.org <dbates@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 24 Dec 2019 00:32:04 +0000 (00:32 +0000)
committerdbates@webkit.org <dbates@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 24 Dec 2019 00:32:04 +0000 (00:32 +0000)
https://bugs.webkit.org/show_bug.cgi?id=205527
<rdar://problem/58128278>

Reviewed by Zalan Bujtas.

Source/WebCore:

Include empty rect when range start position coincides with the end of a simple line layout run.
This makes it match the behavior of line box layout, Firefox's behavior, as well as my understanding
of Extensions to the Range Interface: <https://drafts.csswg.org/cssom-view/#extensions-to-the-range-interface>
(Editor's Draft, 10 October 2019).

At the time of writing, there are two code paths for laying out lines: simple line layout and
line box layout. Simple line layout is not enabled when there is a selection at the time of
writing. As a result, we use line box layout to answer getClientRects(), getBoundingClientRect()
queries.

Test: fast/dom/Range/mac/getClientRects-and-getBoundingClientRect-before-and-after-selection.html

* rendering/SimpleLineLayoutResolver.cpp:
(WebCore::SimpleLineLayout::RunResolver::rangeForRendererWithOffsets const): Do not skip over a run
if its end position coincides with the range's start offset. This ensures that we emit an empty rect
for this part of the box selection, which matches what we do using the analagous line box layout
code path.

LayoutTests:

For now, add a Mac-specific test. This test is specific to Mac because it depends on text metrics for the
Times font. I specifically did not use Ahem so that this test could also be used as the test for
<https://bugs.webkit.org/show_bug.cgi?id=205563>. Currently the test includes expected failure results
since that bug is not fixed.

* TestExpectations: Skip
* fast/dom/Range/mac/getClientRects-and-getBoundingClientRect-before-and-after-selection-expected.txt: Added.
* fast/dom/Range/mac/getClientRects-and-getBoundingClientRect-before-and-after-selection.html: Added.
* platform/mac/TestExpectations: Unskip the test on Mac.

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

LayoutTests/ChangeLog
LayoutTests/TestExpectations
LayoutTests/fast/dom/Range/mac/getClientRects-and-getBoundingClientRect-before-and-after-selection-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/Range/mac/getClientRects-and-getBoundingClientRect-before-and-after-selection.html [new file with mode: 0644]
LayoutTests/platform/mac/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/rendering/SimpleLineLayoutResolver.cpp

index f0b2e2a..0fa35ca 100644 (file)
@@ -1,3 +1,21 @@
+2019-12-23  Daniel Bates  <dabates@apple.com>
+
+        REGRESSION (r212693): getClientRects(), getBoundingClientRect() for range that spans multi-lines differs depending on whether text is selected
+        https://bugs.webkit.org/show_bug.cgi?id=205527
+        <rdar://problem/58128278>
+
+        Reviewed by Zalan Bujtas.
+
+        For now, add a Mac-specific test. This test is specific to Mac because it depends on text metrics for the
+        Times font. I specifically did not use Ahem so that this test could also be used as the test for
+        <https://bugs.webkit.org/show_bug.cgi?id=205563>. Currently the test includes expected failure results
+        since that bug is not fixed.
+
+        * TestExpectations: Skip 
+        * fast/dom/Range/mac/getClientRects-and-getBoundingClientRect-before-and-after-selection-expected.txt: Added.
+        * fast/dom/Range/mac/getClientRects-and-getBoundingClientRect-before-and-after-selection.html: Added.
+        * platform/mac/TestExpectations: Unskip the test on Mac.
+
 2019-12-23  Alexey Proskuryakov  <ap@apple.com>
 
         Remove TestExpectations for scrollingcoordinator/ios/fixed-scrolling-with-keyboard.html
index b109308..a962c0d 100644 (file)
@@ -63,6 +63,7 @@ pointerevents/ios [ Skip ]
 editing/pasteboard/dom-paste [ Skip ]
 editing/pasteboard/mac [ Skip ]
 fast/media/ios [ Skip ]
+fast/dom/Range/mac [ Skip ]
 
 # Requires async overflow scrolling
 compositing/shared-backing/overflow-scroll [ Skip ]
diff --git a/LayoutTests/fast/dom/Range/mac/getClientRects-and-getBoundingClientRect-before-and-after-selection-expected.txt b/LayoutTests/fast/dom/Range/mac/getClientRects-and-getBoundingClientRect-before-and-after-selection-expected.txt
new file mode 100644 (file)
index 0000000..badba0e
--- /dev/null
@@ -0,0 +1,22 @@
+Tests that getClientRects(), getBoundingClientRect() return the same result regardless of whether text is selected or not.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+
+Before selection:
+PASS range.getClientRects().length is 2
+FAIL JSON.stringify(range.getClientRects()[0]) should be {"x":163,"y":8,"width":0,"height":18,"top":8,"right":163,"bottom":26,"left":163}. Was {"x":163.953125,"y":8,"width":0,"height":18,"top":8,"right":163.953125,"bottom":26,"left":163.953125}.
+FAIL JSON.stringify(range.getClientRects()[1]) should be {"x":8,"y":26,"width":29,"height":18,"top":26,"right":37,"bottom":44,"left":8}. Was {"x":8,"y":26,"width":28.453125,"height":18,"top":26,"right":36.453125,"bottom":44,"left":8}.
+FAIL JSON.stringify(range.getBoundingClientRect()) should be {"x":8,"y":8,"width":155,"height":36,"top":8,"right":163,"bottom":44,"left":8}. Was {"x":8,"y":8,"width":155.953125,"height":36,"top":8,"right":163.953125,"bottom":44,"left":8}.
+
+After selection:
+PASS range.getClientRects().length is 2
+PASS JSON.stringify(range.getClientRects()[0]) is "{\"x\":163,\"y\":8,\"width\":0,\"height\":18,\"top\":8,\"right\":163,\"bottom\":26,\"left\":163}"
+PASS JSON.stringify(range.getClientRects()[1]) is "{\"x\":8,\"y\":26,\"width\":29,\"height\":18,\"top\":26,\"right\":37,\"bottom\":44,\"left\":8}"
+PASS JSON.stringify(range.getBoundingClientRect()) is "{\"x\":8,\"y\":8,\"width\":155,\"height\":36,\"top\":8,\"right\":163,\"bottom\":44,\"left\":8}"
+PASS successfullyParsed is true
+Some tests failed.
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/dom/Range/mac/getClientRects-and-getBoundingClientRect-before-and-after-selection.html b/LayoutTests/fast/dom/Range/mac/getClientRects-and-getBoundingClientRect-before-and-after-selection.html
new file mode 100644 (file)
index 0000000..4012f04
--- /dev/null
@@ -0,0 +1,50 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../../../../resources/js-test.js"></script>
+<style>
+#test {
+    /* We do not use Ahem because its perfectly square glyph would mask rounding bugs in layout. */
+    /* FIXME: Find a way to write this test so as to insulate it from font changes, but not mask
+    rounding bugs in layout. */;
+    font-family: Times;
+    font-size: 16px;
+}
+</style>
+</head>
+<body>
+<div id="test" style="width: 180px;">This is a sentence that is long enough to span 2 lines.</div>
+<pre id="console"></pre>
+<script>
+description("Tests that getClientRects(), getBoundingClientRect() return the same result regardless of whether text is selected or not.");
+
+let test = document.getElementById("test");
+let range = document.createRange();
+range.setStart(test.firstChild, "This is a sentence that is".length);
+range.setEnd(test.firstChild, "This is a sentence that is long".length);
+
+function testBoundingRect()
+{
+    shouldBeEqualToString('JSON.stringify(range.getBoundingClientRect())', '{"x":8,"y":8,"width":155,"height":36,"top":8,"right":163,"bottom":44,"left":8}');
+}
+
+function testClientRects()
+{
+    shouldBe('range.getClientRects().length', '2');
+    shouldBeEqualToString('JSON.stringify(range.getClientRects()[0])', '{"x":163,"y":8,"width":0,"height":18,"top":8,"right":163,"bottom":26,"left":163}');
+    shouldBeEqualToString('JSON.stringify(range.getClientRects()[1])', '{"x":8,"y":26,"width":29,"height":18,"top":26,"right":37,"bottom":44,"left":8}');
+}
+
+debug("<br>Before selection:");
+testClientRects();
+testBoundingRect();
+
+debug("<br>After selection:");
+window.getSelection().selectAllChildren(test);
+testClientRects();
+testBoundingRect();
+
+document.body.removeChild(test);
+</script>
+</body>
+</html>
index b00b9de..9027a9d 100644 (file)
@@ -12,6 +12,7 @@ editing/mac [ Pass ]
 fast/scrolling/latching [ Pass ]
 media/mac [ Pass ]
 editing/pasteboard/mac [ Pass ]
+fast/dom/Range/mac [ Pass ]
 
 fast/forms/search/search-padding-cancel-results-buttons.html [ Pass ]
 fast/forms/search/search-results-hidden-crash.html [ Pass ]
index 135c2b7..2552c7d 100644 (file)
@@ -1,3 +1,29 @@
+2019-12-23  Daniel Bates  <dabates@apple.com>
+
+        REGRESSION (r212693): getClientRects(), getBoundingClientRect() for range that spans multi-lines differs depending on whether text is selected
+        https://bugs.webkit.org/show_bug.cgi?id=205527
+        <rdar://problem/58128278>
+
+        Reviewed by Zalan Bujtas.
+
+        Include empty rect when range start position coincides with the end of a simple line layout run.
+        This makes it match the behavior of line box layout, Firefox's behavior, as well as my understanding
+        of Extensions to the Range Interface: <https://drafts.csswg.org/cssom-view/#extensions-to-the-range-interface>
+        (Editor's Draft, 10 October 2019).
+
+        At the time of writing, there are two code paths for laying out lines: simple line layout and
+        line box layout. Simple line layout is not enabled when there is a selection at the time of
+        writing. As a result, we use line box layout to answer getClientRects(), getBoundingClientRect()
+        queries.
+
+        Test: fast/dom/Range/mac/getClientRects-and-getBoundingClientRect-before-and-after-selection.html
+
+        * rendering/SimpleLineLayoutResolver.cpp:
+        (WebCore::SimpleLineLayout::RunResolver::rangeForRendererWithOffsets const): Do not skip over a run
+        if its end position coincides with the range's start offset. This ensures that we emit an empty rect
+        for this part of the box selection, which matches what we do using the analagous line box layout
+        code path.
+
 2019-12-23  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         [GTK][WPE] Special combination characters doesn't respect the keystroke order when high CPU load
index 14c61c7..d3a6c18 100644 (file)
@@ -297,8 +297,10 @@ WTF::IteratorRange<RunResolver::Iterator> RunResolver::rangeForRendererWithOffse
         return { end(), end() };
     auto it = range.begin();
     auto localEnd = (*it).start() + endOffset;
-    // Advance to the first run with the start offset inside. Only the first node in a range can have a startOffset.
-    while (it != range.end() && (*it).end() <= startOffset)
+    // Advance to the first run before the start offset. Only the first node in a range can have a startOffset.
+    // Note that the start offset may coincide with the end of a run. The run is still considered so that we
+    // can return an empty rect, which conforms to the behavior of Element.getClientRects().
+    while (it != range.end() && (*it).end() < startOffset)
         ++it;
     if (it == range.end())
         return { end(), end() };