Dictionary hotkey does not work on vertical text
authorap@apple.com <ap@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 23 Jul 2013 19:09:39 +0000 (19:09 +0000)
committerap@apple.com <ap@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 23 Jul 2013 19:09:39 +0000 (19:09 +0000)
        https://bugs.webkit.org/show_bug.cgi?id=118993
        <rdar://problem/14478260>

        Reviewed by Enrica Casucci.

        Test: platform/mac/editing/input/firstrectforcharacterrange-vertical.html

        * editing/Editor.cpp:
        (WebCore::collapseCaretWidth): A helper function.
        (WebCore::Editor::firstRectForRange): Many changes:
        - use RenderObject::absoluteBoundingBoxRectForRange() in regular case, because
        that's more direct that getting caret rects and computing bounding rect from those.
        - handle collapsed ranges separately, because absoluteBoundingBoxRectForRange()
        doesn't provide the needed result, and because it can be done faster.
        - wherever we use carets to compute the result, account for vertical text (in a hackish
        way, as we don't have layout information at Editor level).

        * rendering/RenderBlock.cpp: (WebCore::RenderBlock::localCaretRect): Removed
        dead code.

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

LayoutTests/ChangeLog
LayoutTests/platform/mac/editing/input/caret-primary-bidi-expected.txt
LayoutTests/platform/mac/editing/input/firstrectforcharacterrange-vertical-expected.txt [new file with mode: 0644]
LayoutTests/platform/mac/editing/input/firstrectforcharacterrange-vertical.html [new file with mode: 0644]
LayoutTests/platform/wk2/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/editing/Editor.cpp
Source/WebCore/rendering/RenderBlock.cpp

index 095da53..bd0f5fd 100644 (file)
@@ -1,3 +1,18 @@
+2013-07-23  Alexey Proskuryakov  <ap@apple.com>
+
+        Dictionary hotkey does not work on vertical text
+        https://bugs.webkit.org/show_bug.cgi?id=118993
+        <rdar://problem/14478260>
+
+        Reviewed by Enrica Casucci.
+
+        * platform/mac/editing/input/caret-primary-bidi-expected.txt: Old results had
+        some collapsed positions have a rect of width 1. Now they are all consistently 0.
+
+        * platform/mac/editing/input/firstrectforcharacterrange-vertical-expected.txt: Added.
+        * platform/mac/editing/input/firstrectforcharacterrange-vertical.html: Added.
+        * platform/wk2/TestExpectations: Skipping the new test, as WKTR doesn't implement firstRectForCharacterRange.
+
 2013-07-23  Bem Jones-Bey  <bjonesbe@adobe.com>
 
         [CSS Shapes] New positioning model: Borders
index 8347f41..40722f5 100644 (file)
@@ -1,7 +1,7 @@
 0: 124,508,0,28
 1: 21,564,0,28
 2: 36,564,0,28
-3: 48,564,1,28
+3: 48,564,0,28
 4: 154,564,0,28
 5: 140,564,0,28
 6: 85,564,0,28
@@ -10,7 +10,7 @@
 9: 131,564,0,28
 10: 73,564,0,28
 11: 56,564,0,28
-12: 169,564,1,28
+12: 170,564,0,28
 13: 185,564,0,28
 14: 198,564,0,28
 15: 207,564,0,28
@@ -37,7 +37,7 @@
 36: 47,478,0,28
 37: 60,478,0,28
 38: 75,478,0,28
-39: 87,478,1,28
+39: 87,478,0,28
 40: 112,478,0,28
 41: 95,478,0,28
 42: 124,478,0,28
@@ -60,7 +60,7 @@
 59: 791,422,0,28
 60: 776,422,0,28
 61: 762,422,0,28
-62: 752,422,1,28
+62: 753,422,0,28
 63: 722,422,0,28
 64: 737,422,0,28
 65: 707,422,0,28
@@ -80,7 +80,7 @@
 79: 751,364,0,28
 80: 736,364,0,28
 81: 722,364,0,28
-82: 711,364,1,28
+82: 711,364,0,28
 83: 689,364,0,28
 84: 702,364,0,28
 85: 674,364,0,28
diff --git a/LayoutTests/platform/mac/editing/input/firstrectforcharacterrange-vertical-expected.txt b/LayoutTests/platform/mac/editing/input/firstrectforcharacterrange-vertical-expected.txt
new file mode 100644 (file)
index 0000000..5cbc3d7
--- /dev/null
@@ -0,0 +1,9 @@
+Test that firstRectForCharacterRange result for vertical text is reasonable in a very simple case. To test manually, invoke Dictionary pop-up with Ctrl+Option+D combo.
+
+PASS textInputController.firstRectForCharacterRange(0, 1) is reasonable
+PASS textInputController.firstRectForCharacterRange(1, 0) is reasonable
+PASS textInputController.firstRectForCharacterRange(1, 200) is reasonable
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/platform/mac/editing/input/firstrectforcharacterrange-vertical.html b/LayoutTests/platform/mac/editing/input/firstrectforcharacterrange-vertical.html
new file mode 100644 (file)
index 0000000..24ca9a5
--- /dev/null
@@ -0,0 +1,61 @@
+<head>
+<script src="../../../../fast/js/resources/js-test-pre.js"></script>
+</head>
+<body>
+<p>Test that firstRectForCharacterRange result for vertical text is reasonable in a very simple case. To test manually, invoke Dictionary pop-up with Ctrl+Option+D combo.</p>
+<p id="console"></p>
+<div style="-webkit-writing-mode:vertical-lr;" contenteditable id=target>Lorem ipsum dolor sit amet, consectetur adipiscing elit. Integer pretium quis odio dapibus interdum. Fusce vel consequat arcu, non suscipit leo. In faucibus neque nulla, sodales luctus nulla sodales in. Praesent lobortis enim nec mauris aliquam, et tincidunt nulla gravida. Etiam a sem turpis. Suspendisse odio erat, sodales eget nunc at, adipiscing posuere dolor. Cras sit amet leo pulvinar, condimentum augue eget, mattis lacus. Integer volutpat nulla neque, faucibus varius massa sagittis at. Praesent non mauris vel justo tristique porttitor. Cras et tempus enim. Sed nec condimentum diam. Vivamus bibendum a odio at bibendum. Phasellus iaculis facilisis ante, a sagittis magna sollicitudin non. In pretium eu lacus nec vulputate. Morbi lacinia tortor arcu, quis ultrices risus dictum eu. Suspendisse semper mattis tortor, eget pretium neque accumsan ac. Morbi imperdiet ultricies ante, sit amet dapibus ipsum mollis at. Fusce placerat tortor at est mattis ornare. Aenean tristique est id posuere.</div>
+<script>
+function singleCharacterRectIsReasonable(rect)
+{
+    var width = rect[2];
+    var height = rect[3];
+
+    return width > 5 && width < 20 && height > 5 && height < 20;
+}
+
+function emptyRangeRectIsReasonable(rect)
+{
+    var width = rect[2];
+    var height = rect[3];
+
+    return width > 5 && width < 20 && height == 0;
+}
+
+function multilineRangeRectIsReasonable(rect)
+{
+    var width = rect[2];
+    var height = rect[3];
+
+    return width > 5 && width < 20 && height > 100;
+}
+
+if (window.testRunner) {
+    testRunner.dumpAsText();
+
+    var target = document.getElementById("target");
+
+    window.getSelection().setPosition(target, 0);
+    var singleCharacterRect = textInputController.firstRectForCharacterRange(0, 1);
+    if (singleCharacterRectIsReasonable(singleCharacterRect))
+        testPassed("textInputController.firstRectForCharacterRange(0, 1) is reasonable");
+    else
+        testFailed("textInputController.firstRectForCharacterRange(0, 1) is not reasonable: " + singleCharacterRect);
+
+    var emptyRangeRect = textInputController.firstRectForCharacterRange(1, 0);
+    if (emptyRangeRectIsReasonable(emptyRangeRect))
+        testPassed("textInputController.firstRectForCharacterRange(1, 0) is reasonable");
+    else
+        testFailed("textInputController.firstRectForCharacterRange(1, 0) is not reasonable: " + emptyRangeRect);
+
+    var multilineRangeRect = textInputController.firstRectForCharacterRange(1, 200);
+    if (multilineRangeRectIsReasonable(multilineRangeRect))
+        testPassed("textInputController.firstRectForCharacterRange(1, 200) is reasonable");
+    else
+        testFailed("textInputController.firstRectForCharacterRange(1, 200) is not reasonable: " + multilineRangeRect);
+
+    target.style.display = "none";
+}
+</script>
+<script src="../../../../fast/js/resources/js-test-post.js"></script>
+</body>
index d5344cc..8dc3d5e 100644 (file)
@@ -127,6 +127,7 @@ platform/mac/editing/input/bold-node.html
 platform/mac/editing/input/caret-primary-bidi.html
 platform/mac/editing/input/firstrectforcharacterrange-plain.html
 platform/mac/editing/input/firstrectforcharacterrange-styled.html
+platform/mac/editing/input/firstrectforcharacterrange-vertical.html
 platform/mac/editing/input/hangul-enter-confirms-and-sends-keypress.html
 platform/mac/editing/input/insert-delete-smp-symbol.html
 platform/mac/editing/input/kotoeri-enter-to-confirm-and-newline.html
index 7c966cd..15a06da 100644 (file)
@@ -1,3 +1,26 @@
+2013-07-23  Alexey Proskuryakov  <ap@apple.com>
+
+        Dictionary hotkey does not work on vertical text
+        https://bugs.webkit.org/show_bug.cgi?id=118993
+        <rdar://problem/14478260>
+
+        Reviewed by Enrica Casucci.
+
+        Test: platform/mac/editing/input/firstrectforcharacterrange-vertical.html
+
+        * editing/Editor.cpp:
+        (WebCore::collapseCaretWidth): A helper function.
+        (WebCore::Editor::firstRectForRange): Many changes:
+        - use RenderObject::absoluteBoundingBoxRectForRange() in regular case, because
+        that's more direct that getting caret rects and computing bounding rect from those.
+        - handle collapsed ranges separately, because absoluteBoundingBoxRectForRange()
+        doesn't provide the needed result, and because it can be done faster.
+        - wherever we use carets to compute the result, account for vertical text (in a hackish
+        way, as we don't have layout information at Editor level).
+
+        * rendering/RenderBlock.cpp: (WebCore::RenderBlock::localCaretRect): Removed
+        dead code.
+
 2013-07-23  Bem Jones-Bey  <bjonesbe@adobe.com>
 
         [CSS Shapes] New positioning model: Borders
index 21e71cf..d0e2bf7 100644 (file)
@@ -2656,33 +2656,51 @@ String Editor::selectedText(TextIteratorBehavior behavior) const
     return plainText(m_frame->selection()->toNormalizedRange().get(), behavior).replaceWithLiteral('\0', "");
 }
 
+static inline void collapseCaretWidth(IntRect& rect)
+{
+    // FIXME: Width adjustment doesn't work for rotated text.
+    if (rect.width() == caretWidth)
+        rect.setWidth(0);
+    else if (rect.height() == caretWidth)
+        rect.setHeight(0);
+}
+
 IntRect Editor::firstRectForRange(Range* range) const
 {
-    LayoutUnit extraWidthToEndOfLine = 0;
     ASSERT(range->startContainer());
     ASSERT(range->endContainer());
 
-    IntRect startCaretRect = RenderedPosition(VisiblePosition(range->startPosition()).deepEquivalent(), DOWNSTREAM).absoluteRect(&extraWidthToEndOfLine);
-    if (startCaretRect == LayoutRect())
-        return IntRect();
+    VisiblePosition startVisiblePosition(range->startPosition(), DOWNSTREAM);
+
+    if (range->collapsed(ASSERT_NO_EXCEPTION)) {
+        // FIXME: Getting caret rect and removing caret width is a very roundabout way to get collapsed range location.
+        // In particular, width adjustment doesn't work for rotated text.
+        IntRect startCaretRect = RenderedPosition(startVisiblePosition).absoluteRect();
+        collapseCaretWidth(startCaretRect);
+        return startCaretRect;
+    }
 
-    IntRect endCaretRect = RenderedPosition(VisiblePosition(range->endPosition()).deepEquivalent(), UPSTREAM).absoluteRect();
-    if (endCaretRect == LayoutRect())
+    VisiblePosition endVisiblePosition(range->endPosition(), UPSTREAM);
+
+    if (inSameLine(startVisiblePosition, endVisiblePosition))
+        return enclosingIntRect(RenderObject::absoluteBoundingBoxRectForRange(range));
+
+    LayoutUnit extraWidthToEndOfLine = 0;
+    IntRect startCaretRect = RenderedPosition(startVisiblePosition).absoluteRect(&extraWidthToEndOfLine);
+    if (startCaretRect == IntRect())
         return IntRect();
 
-    if (startCaretRect.y() == endCaretRect.y()) {
-        // start and end are on the same line
-        return IntRect(min(startCaretRect.x(), endCaretRect.x()),
+    // When start and end aren't on the same line, we want to go from start to the end of its line.
+    bool textIsHorizontal = startCaretRect.width() == caretWidth;
+    return textIsHorizontal ?
+        IntRect(startCaretRect.x(),
             startCaretRect.y(),
-            abs(endCaretRect.x() - startCaretRect.x()),
-            max(startCaretRect.height(), endCaretRect.height()));
-    }
-
-    // start and end aren't on the same line, so go from start to the end of its line
-    return IntRect(startCaretRect.x(),
-        startCaretRect.y(),
-        startCaretRect.width() + extraWidthToEndOfLine,
-        startCaretRect.height());
+            startCaretRect.width() + extraWidthToEndOfLine,
+            startCaretRect.height()) :
+        IntRect(startCaretRect.x(),
+            startCaretRect.y(),
+            startCaretRect.width(),
+            startCaretRect.height() + extraWidthToEndOfLine);
 }
 
 bool Editor::shouldChangeSelection(const VisibleSelection& oldSelection, const VisibleSelection& newSelection, EAffinity affinity, bool stillSelecting) const
index ee6aa32..c1b3fe1 100644 (file)
@@ -7327,24 +7327,9 @@ LayoutRect RenderBlock::localCaretRect(InlineBox* inlineBox, int caretOffset, La
 
     LayoutRect caretRect = localCaretRectForEmptyElement(width(), textIndentOffset());
 
-    if (extraWidthToEndOfLine) {
-        if (isRenderBlock()) {
-            *extraWidthToEndOfLine = width() - caretRect.maxX();
-        } else {
-            // FIXME: This code looks wrong.
-            // myRight and containerRight are set up, but then clobbered.
-            // So *extraWidthToEndOfLine will always be 0 here.
-
-            LayoutUnit myRight = caretRect.maxX();
-            // FIXME: why call localToAbsoluteForContent() twice here, too?
-            FloatPoint absRightPoint = localToAbsolute(FloatPoint(myRight, 0));
-
-            LayoutUnit containerRight = containingBlock()->x() + containingBlockLogicalWidthForContent();
-            FloatPoint absContainerPoint = localToAbsolute(FloatPoint(containerRight, 0));
-
-            *extraWidthToEndOfLine = absContainerPoint.x() - absRightPoint.x();
-        }
-    }
+    // FIXME: Does this need to adjust for vertical orientation?
+    if (extraWidthToEndOfLine)
+        *extraWidthToEndOfLine = width() - caretRect.maxX();
 
     return caretRect;
 }