Changeset 153060 in webkit

Timestamp:

Jul 23, 2013 12:09:39 PM (11 years ago)

Author:

ap@apple.com

Message:

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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/platform/mac/editing/input/caret-primary-bidi-expected.txt (modified) (5 diffs)
• LayoutTests/platform/mac/editing/input/firstrectforcharacterrange-vertical-expected.txt (added)
• LayoutTests/platform/mac/editing/input/firstrectforcharacterrange-vertical.html (added)
• LayoutTests/platform/wk2/TestExpectations (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/editing/Editor.cpp (modified) (1 diff)
• Source/WebCore/rendering/RenderBlock.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/LayoutTests/platform/mac/editing/input/caret-primary-bidi-expected.txt

@@ -2 +2 @@
 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
@@ -11 +11 @@
 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
@@ -38 +38 @@
 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
@@ -61 +61 @@
 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
@@ -81 +81 @@
 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

trunk/LayoutTests/platform/wk2/TestExpectations

@@ -128 +128 @@
 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

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/editing/Editor.cpp

@@ -2657 +2657 @@
 }
 
+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())
+    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;
+    }
+
+    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();
 
-    IntRect endCaretRect = RenderedPosition(VisiblePosition(range->endPosition()).deepEquivalent(), UPSTREAM).absoluteRect();
-    if (endCaretRect == LayoutRect())
-        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);
 }
 

trunk/Source/WebCore/rendering/RenderBlock.cpp

@@ -7328 +7328 @@
     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;