Changeset 76768 in webkit

Timestamp:

Jan 27, 2011 12:23:31 AM (13 years ago)

Author:

mitz@apple.com

Message:

REGRESSION (r76743): Uneven spacing in right-to-left justified text
​https://bugs.webkit.org/show_bug.cgi?id=53225

Reviewed by Sam Weinig.

Fixes failure in fast/text/atsui-spacing-features.html

There was an inconsistency between rendering code and font code in the interpretation of
'after expansion' and 'trailing expansion'. Changed all code to interpret these in terms of
visual order rather than logical.

• platform/graphics/Font.cpp:

(WebCore::Font::expansionOpportunityCount): Added a text direction parameter and changed to
iterate in visual order accordingly.

• platform/graphics/Font.h:
• platform/graphics/WidthIterator.cpp:

(WebCore::WidthIterator::WidthIterator): Pass the run direction to expansionOpportunityCount().
(WebCore::WidthIterator::advance): For right-to-left runs, evaluate the trailing expansion
condition with respect to the first character, which is the trailing character in visual order.

• platform/graphics/mac/ComplexTextController.cpp:

(WebCore::ComplexTextController::ComplexTextController): Pass the run direction to
expansionOpportunityCount().

• rendering/RenderBlockLineLayout.cpp:

(WebCore::RenderBlock::computeInlineDirectionPositionsForLine): Ditto.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• platform/graphics/Font.cpp (modified) (1 diff)
• platform/graphics/Font.h (modified) (2 diffs)
• platform/graphics/WidthIterator.cpp (modified) (2 diffs)
• platform/graphics/mac/ComplexTextController.cpp (modified) (1 diff)
• rendering/RenderBlockLineLayout.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2011-01-27  Dan Bernstein  <mitz@apple.com>
+
+        Reviewed by Sam Weinig.
+
+        REGRESSION (r76743): Uneven spacing in right-to-left justified text
+        https://bugs.webkit.org/show_bug.cgi?id=53225
+
+        Fixes failure in fast/text/atsui-spacing-features.html
+
+        There was an inconsistency between rendering code and font code in the interpretation of
+        'after expansion' and 'trailing expansion'. Changed all code to interpret these in terms of
+        visual order rather than logical.
+
+        * platform/graphics/Font.cpp:
+        (WebCore::Font::expansionOpportunityCount): Added a text direction parameter and changed to
+        iterate in visual order accordingly.
+        * platform/graphics/Font.h:
+        * platform/graphics/WidthIterator.cpp:
+        (WebCore::WidthIterator::WidthIterator): Pass the run direction to expansionOpportunityCount().
+        (WebCore::WidthIterator::advance): For right-to-left runs, evaluate the trailing expansion
+        condition with respect to the first character, which is the trailing character in visual order.
+        * platform/graphics/mac/ComplexTextController.cpp:
+        (WebCore::ComplexTextController::ComplexTextController): Pass the run direction to
+        expansionOpportunityCount().
+        * rendering/RenderBlockLineLayout.cpp:
+        (WebCore::RenderBlock::computeInlineDirectionPositionsForLine): Ditto.
+
 2011-01-26  Adam Roben  <aroben@apple.com>
 

trunk/Source/WebCore/platform/graphics/Font.cpp

@@ -459 +459 @@
 }
 
-unsigned Font::expansionOpportunityCount(const UChar* characters, size_t length, bool& isAfterExpansion)
+unsigned Font::expansionOpportunityCount(const UChar* characters, size_t length, TextDirection direction, bool& isAfterExpansion)
 {
     static bool expandAroundIdeographs = canExpandAroundIdeographsInComplexText();
     unsigned count = 0;
-    for (size_t i = 0; i < length; ++i) {
-        UChar32 character = characters[i];
-        if (treatAsSpace(character)) {
-            count++;
-            isAfterExpansion = true;
-            continue;
+    if (direction == LTR) {
+        for (size_t i = 0; i < length; ++i) {
+            UChar32 character = characters[i];
+            if (treatAsSpace(character)) {
+                count++;
+                isAfterExpansion = true;
+                continue;
+            }
+            if (U16_IS_LEAD(character) && i + 1 < length && U16_IS_TRAIL(characters[i + 1])) {
+                character = U16_GET_SUPPLEMENTARY(character, characters[i + 1]);
+                i++;
+            }
+            if (expandAroundIdeographs && isCJKIdeograph(character)) {
+                if (!isAfterExpansion)
+                    count++;
+                count++;
+                isAfterExpansion = true;
+                continue;
+            }
+            isAfterExpansion = false;
         }
-        if (U16_IS_LEAD(character) && i + 1 < length && U16_IS_TRAIL(characters[i + 1])) {
-            character = U16_GET_SUPPLEMENTARY(character, characters[i + 1]);
-            i++;
+    } else {
+        for (size_t i = length; i > 0; --i) {
+            UChar32 character = characters[i - 1];
+            if (treatAsSpace(character)) {
+                count++;
+                isAfterExpansion = true;
+                continue;
+            }
+            if (U16_IS_TRAIL(character) && i > 1 && U16_IS_LEAD(characters[i - 2])) {
+                character = U16_GET_SUPPLEMENTARY(characters[i - 2], character);
+                i--;
+            }
+            if (expandAroundIdeographs && isCJKIdeograph(character)) {
+                if (!isAfterExpansion)
+                    count++;
+                count++;
+                isAfterExpansion = true;
+                continue;
+            }
+            isAfterExpansion = false;
         }
-        if (expandAroundIdeographs && isCJKIdeograph(character)) {
-            if (!isAfterExpansion)
-                count++;
-            count++;
-            isAfterExpansion = true;
-            continue;
-        }
-        isAfterExpansion = false;
     }
     return count;

trunk/Source/WebCore/platform/graphics/Font.h

@@ -30 +30 @@
 #include "FontFallbackList.h"
 #include "SimpleFontData.h"
+#include "TextDirection.h"
 #include "TypesettingFeatures.h"
 #include <wtf/HashMap.h>
@@ -144 +145 @@
     static bool isCJKIdeographOrSymbol(UChar32);
 
-    static unsigned expansionOpportunityCount(const UChar*, size_t length, bool& isAfterExpansion);
+    static unsigned expansionOpportunityCount(const UChar*, size_t length, TextDirection, bool& isAfterExpansion);
 
 #if PLATFORM(QT)

trunk/Source/WebCore/platform/graphics/WidthIterator.cpp

@@ -65 +65 @@
     else {
         bool isAfterExpansion = true;
-        unsigned expansionOpportunityCount = Font::expansionOpportunityCount(m_run.characters(), m_end, isAfterExpansion);
+        unsigned expansionOpportunityCount = Font::expansionOpportunityCount(m_run.characters(), m_end, m_run.ltr() ? LTR : RTL, isAfterExpansion);
         if (isAfterExpansion && !m_run.allowsTrailingExpansion())
             expansionOpportunityCount--;
@@ -178 +178 @@
             if (treatAsSpace || (expandAroundIdeographs && Font::isCJKIdeograph(c))) {
                 // Distribute the run's total expansion evenly over all expansion opportunities in the run.
-                if (m_expansion && (m_run.allowsTrailingExpansion() || currentCharacter + clusterLength < static_cast<size_t>(m_run.length()))) {
+                if (m_expansion && (m_run.allowsTrailingExpansion() || (m_run.ltr() && currentCharacter + clusterLength < static_cast<size_t>(m_run.length()))
+                    || (m_run.rtl() && currentCharacter))) {
                     float previousExpansion = m_expansion;
                     if (!treatAsSpace && !m_isAfterExpansion) {

trunk/Source/WebCore/platform/graphics/mac/ComplexTextController.cpp

@@ -85 +85 @@
     else {
         bool isAfterExpansion = true;
-        unsigned expansionOpportunityCount = Font::expansionOpportunityCount(m_run.characters(), m_end, isAfterExpansion);
+        unsigned expansionOpportunityCount = Font::expansionOpportunityCount(m_run.characters(), m_end, m_run.ltr() ? LTR : RTL, isAfterExpansion);
         if (isAfterExpansion && !m_run.allowsTrailingExpansion())
             expansionOpportunityCount--;

trunk/Source/WebCore/rendering/RenderBlockLineLayout.cpp

@@ -325 +325 @@
 
             if (textAlign == JUSTIFY && r != trailingSpaceRun) {
-                unsigned opportunitiesInRun = Font::expansionOpportunityCount(rt->characters() + r->m_start, r->m_stop - r->m_start, isAfterExpansion);
+                unsigned opportunitiesInRun = Font::expansionOpportunityCount(rt->characters() + r->m_start, r->m_stop - r->m_start, r->m_box->direction(), isAfterExpansion);
                 expansionOpportunities.append(opportunitiesInRun);
                 expansionOpportunityCount += opportunitiesInRun;