Attachment 29513 Details for Bug 25135 – Proposed fix

[patch] Proposed fix

reverse_truncation.patch (text/plain), 20.27 KB, created by Jeremy Moskovich on 2009-04-15 12:47:17 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Jeremy Moskovich

Created: 2009-04-15 12:47:17 PDT

Size: 20.27 KB

patch

obsolete

>commit d80974e8027f3a7816262477f60f6fa7dd033417
>Author: Jeremy Moskovich <playmobil@dhcp-172-22-71-196.mtv.corp.google.com>
>Date:   Wed Apr 15 12:44:37 2009 -0700
>
>    Fix text-overflow:ellipsis in cases where the ellipsis falls inside an element who's directionality is different from the content.
>
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index c889f99..3315049 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,16 @@
>+2009-04-15  Jeremy Moskovich  <jeremy@chromium.org>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Fix text-overflow:ellipsis in cases where the ellipsis falls
>+        inside an element who's directionality is different from the content.
>+
>+        https://bugs.webkit.org/show_bug.cgi?id=25135
>+
>+        * fast/css/text-overflow-ellipsis.html: Added test cases for mixed
>+        directionality. Separated dir=ltr/rtl test cases to make it easier
>+        to see failures.
>+
> 2009-04-15  Dan Bernstein  <mitz@apple.com>
> 
>         Reviewed by Dave Hyatt.
>diff --git a/LayoutTests/fast/css/text-overflow-ellipsis.html b/LayoutTests/fast/css/text-overflow-ellipsis.html
>index 213cac5..2de2d2a 100644
>--- a/LayoutTests/fast/css/text-overflow-ellipsis.html
>+++ b/LayoutTests/fast/css/text-overflow-ellipsis.html
>@@ -10,10 +10,6 @@
>     margin: 0 0 20px 0;
> }
> 
>-.rtl {
>-    direction: rtl;
>-}
>-
> .forcertl {
>     direction: rtl;
>     unicode-bidi: bidi-override;
>@@ -33,28 +29,56 @@ Pass if the ellipses do not overlap the text and the underlines for the links ar
> placed under the text and not the ellipses.
> <br><br>
> 
>-LTR:
>-<div class="testDiv ellipses">
>-Lorem ipsum dolor sit amet, consectetur adipiscing
>-</div>
>-
> RTL:
>-<div class="testDiv rtl ellipses">
>+<div class="testDiv ellipses" dir=rtl>
> ××©×××§×¡× ×¢×¨×× ××× × ×¨×¦× ××××¡××£ ××××¤×¡××ª ××¡××£ ×©××¨×
> </div>
> 
> RTL Override:
>-<div class="testDiv forcertl ellipses">
>+<div class="testDiv ellipses" dir=rtl>
> Lorem ipsum dolor sit amet, consectetur adipiscing
> </div>
> 
>+RTL link:
>+<div class="testDiv forcertl ellipses" dir=rtl>
>+<a href=#>Lorem ipsum dolor sit amet, consectetur adipiscing</a>
>+</div>
> 
>-LTR link:
>-<div class="testDiv ellipses">
>+RTL with ltr content:
>+<div class="testDiv ellipses" dir=rtl>
>+Lorem ipsum dolor sit amet, consectetur adipiscing
>+</div>
>+
>+RTL link with ltr content:
>+<div class="testDiv ellipses" dir=rtl>
> <a href=#>Lorem ipsum dolor sit amet, consectetur adipiscing</a>
> </div>
> 
>-RTL link:
>-<div class="testDiv forcertl ellipses">
>+LTR link:
>+<div class="testDiv ellipses" dir=ltr>
> <a href=#>Lorem ipsum dolor sit amet, consectetur adipiscing</a>
> </div>
>+
>+LTR:
>+<div class="testDiv ellipses" dir=ltr>
>+Lorem ipsum dolor sit amet, consectetur adipiscing
>+</div>
>+
>+LTR with rtl content:
>+<div class="testDiv ellipses" dir=ltr>
>+××©×××§×¡× ×¢×¨×× ××× × ×¨×¦× ××××¡××£ ××××¤×¡××ª ××¡××£ ×©××¨×
>+</div>
>+
>+LTR link with rtl content:
>+<div class="testDiv ellipses" dir=ltr>
>+<a href=#>
>+××©×××§×¡× ×¢×¨×× ××× × ×¨×¦× ××××¡××£ ××××¤×¡××ª ××¡××£ ×©××¨×
>+</a>
>+</div>
>+
>+LTR link with forced rtl content:
>+<div class="testDiv ellipses" dir=ltr>
>+<span class=forcertl dir=rtl>
>+Lorem ipsum dolor sit amet, consectetur adipiscing
>+</span>
>+</div>
>\ No newline at end of file
>diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
>index af5f39c..d39d17c 100644
>--- a/WebCore/ChangeLog
>+++ b/WebCore/ChangeLog
>@@ -1,3 +1,33 @@
>+2009-04-15  Jeremy Moskovich  <jeremy@chromium.org>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Fix text-overflow:ellipsis in cases where the ellipsis falls
>+        inside an element who's directionality is different from the content.
>+
>+        Also, clean whitespaces at EOL in InlineTextBox.cpp.
>+
>+        https://bugs.webkit.org/show_bug.cgi?id=25135
>+
>+        * rendering/InlineTextBox.cpp:
>+        (WebCore::InlineTextBox::selectionRect): Whitespace hygene.
>+        (WebCore::InlineTextBox::attachLine): Whitespace hygene.
>+        (WebCore::InlineTextBox::placeEllipsisBox): Correctly truncate textboxes.
>+        whose directionality is different than the surrounding flow.
>+        (WebCore::correctedTextColor): Whitespace hygene.
>+        (WebCore::updateGraphicsContext): Whitespace hygene.
>+        (WebCore::InlineTextBox::paint): Fixup drawing of truncated text runs.
>+        (WebCore::InlineTextBox::getTruncationStartEnd): Added.
>+        (WebCore::InlineTextBox::paintSelection): Whitespace hygene.
>+        (WebCore::InlineTextBox::paintCompositionBackground): Whitespace hygene.
>+        (WebCore::InlineTextBox::paintDecoration): Fixup drawing of truncated text runs.
>+        (WebCore::InlineTextBox::paintSpellingOrGrammarMarker): Whitespace hygene.
>+        (WebCore::InlineTextBox::paintTextMatchMarker): Whitespace hygene.
>+        (WebCore::InlineTextBox::paintDocumentMarkers): Whitespace hygene.
>+        (WebCore::InlineTextBox::paintCompositionUnderline): Whitespace hygene.
>+        (WebCore::InlineTextBox::textPos): Whitespace hygene.
>+        * rendering/InlineTextBox.h: Added WebCore::InlineTextBox::getTruncationStartEnd
>+
> 2009-04-15  Adam Langley  <agl@google.com>
> 
>         Reviewed by Darin Fisher.
>diff --git a/WebCore/rendering/InlineTextBox.cpp b/WebCore/rendering/InlineTextBox.cpp
>index 0822d7e..ac4c739 100644
>--- a/WebCore/rendering/InlineTextBox.cpp
>+++ b/WebCore/rendering/InlineTextBox.cpp
>@@ -93,7 +93,7 @@ IntRect InlineTextBox::selectionRect(int tx, int ty, int startPos, int endPos)
> {
>     int sPos = max(startPos - m_start, 0);
>     int ePos = min(endPos - m_start, (int)m_len);
>-    
>+
>     if (sPos >= ePos)
>         return IntRect();
> 
>@@ -129,7 +129,7 @@ void InlineTextBox::attachLine()
> {
>     if (!m_extracted)
>         return;
>-    
>+
>     toRenderText(renderer())->attachTextBox(this);
> }
> 
>@@ -141,7 +141,7 @@ int InlineTextBox::placeEllipsisBox(bool ltr, int blockEdge, int ellipsisWidth,
>     }
> 
>     int ellipsisX = ltr ? blockEdge - ellipsisWidth : blockEdge + ellipsisWidth;
>-    
>+
>     // Criteria for full truncation:
>     // LTR: the left edge of the ellipsis is to the left of our text run.
>     // RTL: the right edge of the ellipsis is to the right of our text run.
>@@ -157,9 +157,6 @@ int InlineTextBox::placeEllipsisBox(bool ltr, int blockEdge, int ellipsisWidth,
>     bool ltrEllipsisWithinBox = ltr && (ellipsisX < m_x + m_width);
>     bool rtlEllipsisWithinBox = !ltr && (ellipsisX > m_x);
>     if (ltrEllipsisWithinBox || rtlEllipsisWithinBox) {
>-        if ((ltr && direction() == RTL) || (!ltr && direction() == LTR))
>-            return -1; // FIXME: Support cases in which the last run's directionality differs from the context.
>-
>         foundBox = true;
> 
>         int offset = offsetForPosition(ellipsisX, false);
>@@ -172,32 +169,44 @@ int InlineTextBox::placeEllipsisBox(bool ltr, int blockEdge, int ellipsisWidth,
> 
>         // Set the truncation index on the text run.  The ellipsis needs to be placed just after the last visible character.
>         m_truncation = offset;
>-        if (ltr)
>-            return m_x + toRenderText(renderer())->width(m_start, offset, textPos(), m_firstLine);
>+        int ellipsisCharOffset = offset;
>+        bool ellipsisOnRightOfBox = ltr;
>+
>+        // If the direction of the text run is different from the context's directionality, then flip the side of the
>+        // box that the ellipsis is on.
>+        bool truncationInOppositeDirection = (ltr && direction() == RTL) || (!ltr && direction() == LTR);
>+        if (truncationInOppositeDirection) {
>+            ellipsisOnRightOfBox = !ellipsisOnRightOfBox;
>+            ellipsisCharOffset = m_truncation - 1;
>+            m_truncation += 1;
>+        }
>+
>+        if (ellipsisOnRightOfBox)
>+            return m_x + toRenderText(renderer())->width(m_start, ellipsisCharOffset, textPos(), m_firstLine);
>         else
>-            return m_x + (m_width - toRenderText(renderer())->width(m_start, offset, textPos(), m_firstLine)) - ellipsisWidth;
>+            return m_x + (m_width - toRenderText(renderer())->width(m_start, ellipsisCharOffset, textPos(), m_firstLine)) - ellipsisWidth;
>     }
>     return -1;
> }
> 
>-Color correctedTextColor(Color textColor, Color backgroundColor) 
>+Color correctedTextColor(Color textColor, Color backgroundColor)
> {
>     // Adjust the text color if it is too close to the background color,
>     // by darkening or lightening it to move it further away.
>-    
>+
>     int d = differenceSquared(textColor, backgroundColor);
>-    // semi-arbitrarily chose 65025 (255^2) value here after a few tests; 
>+    // semi-arbitrarily chose 65025 (255^2) value here after a few tests;
>     if (d > 65025) {
>         return textColor;
>     }
>-    
>+
>     int distanceFromWhite = differenceSquared(textColor, Color::white);
>     int distanceFromBlack = differenceSquared(textColor, Color::black);
> 
>     if (distanceFromWhite < distanceFromBlack) {
>         return textColor.dark();
>     }
>-    
>+
>     return textColor.light();
> }
> 
>@@ -211,7 +220,7 @@ void updateGraphicsContext(GraphicsContext* context, const Color& fillColor, con
>             mode = newMode;
>         }
>     }
>-    
>+
>     if (mode & cTextFill && fillColor != context->fillColor())
>         context->setFillColor(fillColor);
> 
>@@ -290,7 +299,7 @@ void InlineTextBox::paint(RenderObject::PaintInfo& paintInfo, int tx, int ty)
>     if (isLineBreak() || !renderer()->shouldPaintWithinRoot(paintInfo) || renderer()->style()->visibility() != VISIBLE ||
>         m_truncation == cFullTruncation || paintInfo.phase == PaintPhaseOutline)
>         return;
>-    
>+
>     ASSERT(paintInfo.phase != PaintPhaseSelfOutline && paintInfo.phase != PaintPhaseChildOutlines);
> 
>     int xPos = tx + m_x - parent()->maxHorizontalVisualOverflow();
>@@ -299,7 +308,7 @@ void InlineTextBox::paint(RenderObject::PaintInfo& paintInfo, int tx, int ty)
>         return;
> 
>     bool isPrinting = textRenderer()->document()->printing();
>-    
>+
>     // Determine whether or not we're selected.
>     bool haveSelection = !isPrinting && paintInfo.phase != PaintPhaseTextClip && selectionState() != RenderObject::SelectionNone;
>     if (!haveSelection && paintInfo.phase == PaintPhaseSelection)
>@@ -426,8 +435,9 @@ void InlineTextBox::paint(RenderObject::PaintInfo& paintInfo, int tx, int ty)
> 
>         updateGraphicsContext(context, textFillColor, textStrokeColor, textStrokeWidth);
>         if (!paintSelectedTextSeparately || ePos <= sPos) {
>-            // FIXME: Truncate right-to-left text correctly.
>-            paintTextWithShadows(context, font, textRun, 0, m_truncation == cNoTruncation ? m_len : m_truncation, textOrigin, m_x + tx, m_y + ty, width(), height(), textShadow, textStrokeWidth > 0);
>+            int truncationSPos, truncationEPos;
>+            getTruncationStartEnd(truncationSPos, truncationEPos);
>+            paintTextWithShadows(context, font, textRun, truncationSPos, truncationEPos, textOrigin, m_x + tx, m_y + ty, width(), height(), textShadow, textStrokeWidth > 0);
>         } else
>             paintTextWithShadows(context, font, textRun, ePos, sPos, textOrigin, m_x + tx, m_y + ty, width(), height(), textShadow, textStrokeWidth > 0);
> 
>@@ -465,10 +475,10 @@ void InlineTextBox::paint(RenderObject::PaintInfo& paintInfo, int tx, int ty)
> 
>                 if (underline.endOffset <= start())
>                     // underline is completely before this run.  This might be an underline that sits
>-                    // before the first run we draw, or underlines that were within runs we skipped 
>+                    // before the first run we draw, or underlines that were within runs we skipped
>                     // due to truncation.
>                     continue;
>-                
>+
>                 if (underline.startOffset <= end()) {
>                     // underline intersects this run.  Paint it.
>                     paintCompositionUnderline(context, tx, ty, underline);
>@@ -483,6 +493,22 @@ void InlineTextBox::paint(RenderObject::PaintInfo& paintInfo, int tx, int ty)
>     }
> }
> 
>+void InlineTextBox::getTruncationStartEnd(int& truncationSPos, int& truncationEPos)
>+{
>+  truncationSPos = 0;
>+  truncationEPos = m_len;
>+
>+  // No truncation -> nothing to do.
>+  if (m_truncation == cNoTruncation || m_truncation == cFullTruncation)
>+      return;
>+
>+  if (renderer()->containingBlock()->style()->direction() != direction())
>+      // e.g. English text truncated in an RTL context: Hello -> ...lo
>+      truncationSPos = m_truncation;
>+  else
>+      truncationEPos = m_truncation;
>+}
>+
> void InlineTextBox::selectionStartEnd(int& sPos, int& ePos)
> {
>     int startPos, endPos;
>@@ -524,7 +550,7 @@ void InlineTextBox::paintSelection(GraphicsContext* context, int tx, int ty, Ren
>     int y = selectionTop();
>     int h = selectionHeight();
>     context->clip(IntRect(m_x + tx, y + ty, m_width, h));
>-    context->drawHighlightForText(font, TextRun(textRenderer()->text()->characters() + m_start, m_len, textRenderer()->allowTabs(), textPos(), m_toAdd, 
>+    context->drawHighlightForText(font, TextRun(textRenderer()->text()->characters() + m_start, m_len, textRenderer()->allowTabs(), textPos(), m_toAdd,
>                                   direction() == RTL, m_dirOverride || style->visuallyOrdered()),
>                                   IntPoint(m_x + tx, y + ty), h, c, sPos, ePos);
>     context->restore();
>@@ -542,7 +568,7 @@ void InlineTextBox::paintCompositionBackground(GraphicsContext* context, int tx,
>     context->save();
> 
>     Color c = Color(225, 221, 85);
>-    
>+
>     updateGraphicsContext(context, c, c, 0); // Don't draw text at all!
> 
>     int y = selectionTop();
>@@ -583,15 +609,24 @@ void InlineTextBox::paintDecoration(GraphicsContext* context, int tx, int ty, in
> 
>     int width = m_width;
>     if (m_truncation != cNoTruncation) {
>-        width = toRenderText(renderer())->width(m_start, m_truncation, textPos(), m_firstLine);
>-        if (direction() == RTL)
>+        int truncationSPos, truncationEPos;
>+        getTruncationStartEnd(truncationSPos, truncationEPos);
>+
>+        TextDirection context_direction = renderer()->containingBlock()->style()->direction();
>+
>+        if (context_direction != direction() && context_direction == LTR)
>+            truncationSPos = min(truncationSPos + 1, truncationEPos);
>+
>+        width = toRenderText(renderer())->width(m_start + truncationSPos, truncationEPos, textPos(), m_firstLine);
>+
>+        if (context_direction == RTL)
>             tx += (m_width - width);
>     }
>-    
>+
>     // Get the text decoration colors.
>     Color underline, overline, linethrough;
>     renderer()->getTextDecorationColors(deco, underline, overline, linethrough, true);
>-    
>+
>     // Use a special function for underlines to get the positioning exactly right.
>     bool isPrinting = textRenderer()->document()->printing();
>     context->setStrokeThickness(1.0f); // FIXME: We should improve this rule and not always just assume 1.
>@@ -620,7 +655,7 @@ void InlineTextBox::paintDecoration(GraphicsContext* context, int tx, int ty, in
>     }
> 
>     bool setShadow = false;
>-    
>+
>     do {
>         if (shadow) {
>             if (!shadow->next) {
>@@ -681,7 +716,7 @@ void InlineTextBox::paintSpellingOrGrammarMarker(GraphicsContext* pt, int tx, in
>     if (!markerSpansWholeBox || grammar) {
>         int startPosition = max<int>(marker.startOffset - m_start, 0);
>         int endPosition = min<int>(marker.endOffset - m_start, m_len);
>-        
>+
>         if (m_truncation != cNoTruncation)
>             endPosition = min<int>(endPosition, m_truncation);
> 
>@@ -689,17 +724,17 @@ void InlineTextBox::paintSpellingOrGrammarMarker(GraphicsContext* pt, int tx, in
>         IntPoint startPoint(tx + m_x, ty + selectionTop());
>         TextRun run(textRenderer()->text()->characters() + m_start, m_len, textRenderer()->allowTabs(), textPos(), m_toAdd, direction() == RTL, m_dirOverride || style->visuallyOrdered());
>         int h = selectionHeight();
>-        
>+
>         IntRect markerRect = enclosingIntRect(font.selectionRectForText(run, startPoint, h, startPosition, endPosition));
>         start = markerRect.x() - startPoint.x();
>         width = markerRect.width();
>-        
>+
>         // Store rendered rects for bad grammar markers, so we can hit-test against it elsewhere in order to
>         // display a toolTip. We don't do this for misspelling markers.
>         if (grammar)
>             renderer()->document()->setRenderedRectForMarker(renderer()->node(), marker, markerRect);
>     }
>-    
>+
>     // IMPORTANT: The misspelling underline is not considered when calculating the text bounds, so we have to
>     // make sure to fit within those bounds.  This means the top pixel(s) of the underline will overlap the
>     // bottom pixel(s) of the glyphs in smaller font sizes.  The alternatives are to increase the line spacing (bad!!)
>@@ -726,16 +761,16 @@ void InlineTextBox::paintTextMatchMarker(GraphicsContext* pt, int tx, int ty, Do
>    // the same word there are no pieces sticking out.
>     int y = selectionTop();
>     int h = selectionHeight();
>-    
>+
>     int sPos = max(marker.startOffset - m_start, (unsigned)0);
>-    int ePos = min(marker.endOffset - m_start, (unsigned)m_len);    
>+    int ePos = min(marker.endOffset - m_start, (unsigned)m_len);
>     TextRun run(textRenderer()->text()->characters() + m_start, m_len, textRenderer()->allowTabs(), textPos(), m_toAdd, direction() == RTL, m_dirOverride || style->visuallyOrdered());
>     IntPoint startPoint = IntPoint(m_x + tx, y + ty);
>-    
>+
>     // Always compute and store the rect associated with this marker
>     IntRect markerRect = enclosingIntRect(font.selectionRectForText(run, startPoint, h, sPos, ePos));
>     renderer()->document()->setRenderedRectForMarker(renderer()->node(), marker, markerRect);
>-     
>+
>     // Optionally highlight the text
>     if (renderer()->document()->frame()->markedTextMatchesAreHighlighted()) {
>         Color color = marker.activeMatch ?
>@@ -761,7 +796,7 @@ void InlineTextBox::paintDocumentMarkers(GraphicsContext* pt, int tx, int ty, Re
>     // Note end() points at the last char, not one past it like endOffset and ranges do.
>     for ( ; markerIt != markers.end(); markerIt++) {
>         DocumentMarker marker = *markerIt;
>-        
>+
>         // Paint either the background markers or the foreground markers, but not both
>         switch (marker.type) {
>             case DocumentMarker::Grammar:
>@@ -769,12 +804,12 @@ void InlineTextBox::paintDocumentMarkers(GraphicsContext* pt, int tx, int ty, Re
>                 if (background)
>                     continue;
>                 break;
>-                
>+
>             case DocumentMarker::TextMatch:
>                 if (!background)
>                     continue;
>                 break;
>-            
>+
>             default:
>                 ASSERT_NOT_REACHED();
>         }
>@@ -783,11 +818,11 @@ void InlineTextBox::paintDocumentMarkers(GraphicsContext* pt, int tx, int ty, Re
>             // marker is completely before this run.  This might be a marker that sits before the
>             // first run we draw, or markers that were within runs we skipped due to truncation.
>             continue;
>-        
>+
>         if (marker.startOffset > end())
>             // marker is completely after this run, bail.  A later run will paint it.
>             break;
>-        
>+
>         // marker intersects this run.  Paint it.
>         switch (marker.type) {
>             case DocumentMarker::Spelling:
>@@ -814,7 +849,7 @@ void InlineTextBox::paintCompositionUnderline(GraphicsContext* ctx, int tx, int
> 
>     if (m_truncation == cFullTruncation)
>         return;
>-    
>+
>     int start = 0;                 // start of line to draw, relative to tx
>     int width = m_width;           // how much line to draw
>     bool useWholeWidth = true;
>@@ -874,7 +909,7 @@ int InlineTextBox::textPos() const
> {
>     if (x() == 0)
>         return 0;
>-        
>+
>     RenderBlock *blockElement = renderer()->containingBlock();
>     return direction() == RTL ? x() - blockElement->borderRight() - blockElement->paddingRight()
>                       : x() - blockElement->borderLeft() - blockElement->paddingLeft();
>diff --git a/WebCore/rendering/InlineTextBox.h b/WebCore/rendering/InlineTextBox.h
>index 6689fcd..031ca82 100644
>--- a/WebCore/rendering/InlineTextBox.h
>+++ b/WebCore/rendering/InlineTextBox.h
>@@ -72,6 +72,7 @@ public:
>     virtual IntRect selectionRect(int absx, int absy, int startPos, int endPos);
>     bool isSelected(int startPos, int endPos) const;
>     void selectionStartEnd(int& sPos, int& ePos);
>+    void getTruncationStartEnd(int& truncationSPos, int& truncationEPos);
> 
> private:
>     virtual void paint(RenderObject::PaintInfo&, int tx, int ty);

Actions: View | Formatted Diff | Diff

Attachments on bug 25135: 29513 | 29951