REGRESSION (r76743): Uneven spacing in right-to-left justified text
authormitz@apple.com <mitz@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 27 Jan 2011 08:23:31 +0000 (08:23 +0000)
committermitz@apple.com <mitz@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 27 Jan 2011 08:23:31 +0000 (08:23 +0000)
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.

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

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/Font.cpp
Source/WebCore/platform/graphics/Font.h
Source/WebCore/platform/graphics/WidthIterator.cpp
Source/WebCore/platform/graphics/mac/ComplexTextController.cpp
Source/WebCore/rendering/RenderBlockLineLayout.cpp

index ed4097a..199059a 100644 (file)
@@ -1,3 +1,30 @@
+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>
 
         Don't create the Direct3D device before it's first needed
index 67f2b26..0f8ebcb 100644 (file)
@@ -458,29 +458,52 @@ bool Font::isCJKIdeographOrSymbol(UChar32 c)
     return isCJKIdeograph(c);
 }
 
-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 (U16_IS_LEAD(character) && i + 1 < length && U16_IS_TRAIL(characters[i + 1])) {
-            character = U16_GET_SUPPLEMENTARY(character, characters[i + 1]);
-            i++;
+    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 (expandAroundIdeographs && isCJKIdeograph(character)) {
-            if (!isAfterExpansion)
+    } else {
+        for (size_t i = length; i > 0; --i) {
+            UChar32 character = characters[i - 1];
+            if (treatAsSpace(character)) {
                 count++;
-            count++;
-            isAfterExpansion = true;
-            continue;
+                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;
         }
-        isAfterExpansion = false;
     }
     return count;
 }
index 30047ed..9ef4e28 100644 (file)
@@ -29,6 +29,7 @@
 #include "FontDescription.h"
 #include "FontFallbackList.h"
 #include "SimpleFontData.h"
+#include "TextDirection.h"
 #include "TypesettingFeatures.h"
 #include <wtf/HashMap.h>
 #include <wtf/HashSet.h>
@@ -143,7 +144,7 @@ public:
     static bool isCJKIdeograph(UChar32);
     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)
     QFont font() const;
index 4b27e91..65c99fb 100644 (file)
@@ -64,7 +64,7 @@ WidthIterator::WidthIterator(const Font* font, const TextRun& run, HashSet<const
         m_expansionPerOpportunity = 0;
     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--;
 
@@ -177,7 +177,8 @@ void WidthIterator::advance(int offset, GlyphBuffer* glyphBuffer)
             bool treatAsSpace = Font::treatAsSpace(c);
             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) {
                         // Take the expansion opportunity before this ideograph.
index fa198ab..673045b 100644 (file)
@@ -84,7 +84,7 @@ ComplexTextController::ComplexTextController(const Font* font, const TextRun& ru
         m_expansionPerOpportunity = 0;
     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--;
 
index cdfcaa0..83d5235 100644 (file)
@@ -324,7 +324,7 @@ void RenderBlock::computeInlineDirectionPositionsForLine(RootInlineBox* lineBox,
             RenderText* rt = toRenderText(r->m_object);
 
             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;
             }