Properly support RTL text drawing in CoreText (#2022)

Now properly draws RTL text with support for text matrix manipulation.
Change CTLineGetOffsetForStringIndex and CTFramesetterCreateFrame to remove assumption of LTR text.

Fixes #1932

Frameworks/CoreGraphics/CGContext.mm

@@ -2089,6 +2089,12 @@ void CGContextSetPatternPhase(CGContextRef context, CGSize phase) {
             // First glyph's origin is at the given relative position for the glyph run
             CGPoint runningPosition{ glyphRuns[i].relativePosition.x, std::round(glyphRuns[i].relativePosition.y) };
             for (size_t j = 0; j < run->glyphCount; ++j) {
+                if (_GlyphRunIsRTL(*run)) {
+                    // Translate position of glyph by advance
+                    // Need to translate each glyph by its own advance because it's RTL
+                    runningPosition.x -= run->glyphAdvances[j];
+                }
+
                 // Invert position by text transformation
                 CGPoint transformedPosition = CGPointApplyAffineTransform(runningPosition, invertedTextTransformation);
 
@@ -2097,18 +2103,21 @@ void CGContextSetPatternPhase(CGContextRef context, CGSize phase) {
                 positions[j] = DWRITE_GLYPH_OFFSET{ transformedPosition.x + run->glyphOffsets[j].advanceOffset,
                                                     std::round(transformedPosition.y + run->glyphOffsets[j].ascenderOffset) };
 
-                // Translate position of next glyph by current glyph's advance
-                runningPosition.x += run->glyphAdvances[j];
+                if (!_GlyphRunIsRTL(*run)) {
+                    // Translate position of next glyph by current glyph's advance
+                    runningPosition.x += run->glyphAdvances[j];
+                }
             }
 
+            // Already compensated for RTL glyph positions, so set bidiLevel to 0
             auto transformedGlyphRun = std::make_shared<DWRITE_GLYPH_RUN>(DWRITE_GLYPH_RUN{ run->fontFace,
                                                                                             run->fontEmSize,
                                                                                             run->glyphCount,
                                                                                             run->glyphIndices,
                                                                                             zeroAdvances.get(),
                                                                                             positions.data(),
                                                                                             run->isSideways,
-                                                                                            run->bidiLevel });
+                                                                                            0 });
             createdRuns.emplace_back(transformedGlyphRun);
             runs.emplace_back(GlyphRunData{ transformedGlyphRun.get(), CGPointZero, glyphRuns[i].attributes });
         }
@@ -2128,7 +2137,7 @@ void CGContextSetPatternPhase(CGContextRef context, CGSize phase) {
                                                                        runData.run->glyphOffsets,
                                                                        runData.run->glyphCount,
                                                                        runData.run->isSideways,
-                                                                       ((runData.run->bidiLevel & 1) == 1),
+                                                                       _GlyphRunIsRTL(*(runData.run)),
                                                                        sink.Get()));
         }
         RETURN_IF_FAILED(sink->Close());

Frameworks/CoreText/CTFramesetter.mm

@@ -95,9 +95,9 @@ CTFrameRef CTFramesetterCreateFrame(CTFramesetterRef framesetterRef, CFRange ran
         (lineBreakMode == kCTLineBreakByClipping || lineBreakMode == kCTLineBreakByTruncatingHead ||
          lineBreakMode == kCTLineBreakByTruncatingTail || lineBreakMode == kCTLineBreakByTruncatingMiddle)) {
         for (size_t i = 0; i < ret->_lineOrigins.size(); ++i) {
-            if (CTLineGetTypographicBounds(static_cast<CTLineRef>([ret->_lines objectAtIndex:i]), nullptr, nullptr, nullptr) >
-                frameRect.size.width) {
-                ret->_lineOrigins[i].x = frameRect.origin.x;
+            _CTLine* line = [ret->_lines objectAtIndex:i];
+            if (line->_width > frameRect.size.width) {
+                ret->_lineOrigins[i].x -= line->_relativeXOffset;
             }
         }
     }

Frameworks/CoreText/CTLine.mm

@@ -48,7 +48,6 @@ - (instancetype)copyWithZone:(NSZone*)zone {
     ret->_glyphCount = _glyphCount;
     ret->_runs.attach([_runs copy]);
     ret->_relativeXOffset = _relativeXOffset;
-    ret->_relativeYOffset = _relativeYOffset;
 
     return ret;
 }
@@ -227,13 +226,13 @@ void CTLineDraw(CTLineRef lineRef, CGContextRef ctx) {
 
     _CTLine* line = static_cast<_CTLine*>(lineRef);
     std::vector<GlyphRunData> runs;
-    CGPoint relativePosition = CGPointZero;
+
+    // Translate by the inverse of the relativeXOffset to draw at the text position
+    CGPoint relativePosition = { -line->_relativeXOffset, 0 };
     for (size_t i = 0; i < [line->_runs count]; ++i) {
         _CTRun* curRun = [line->_runs objectAtIndex:i];
-        if (i > 0) {
-            // Adjusts x position relative to the last run drawn
-            relativePosition.x += curRun->_relativeXOffset;
-        }
+        // Adjusts x position relative to the last run drawn
+        relativePosition.x += curRun->_relativeXOffset;
         runs.emplace_back(GlyphRunData{ &curRun->_dwriteGlyphRun, relativePosition, (CFDictionaryRef)curRun->_attributes.get() });
     }
 
@@ -341,22 +340,27 @@ CFIndex CTLineGetStringIndexForPosition(CTLineRef lineRef, CGPoint position) {
         return kCFNotFound;
     }
 
-    CGFloat currPos = 0;
-
+    CGFloat curPos = 0;
     for (_CTRun* run in static_cast<id<NSFastEnumeration>>(line->_runs)) {
+        curPos += run->_relativeXOffset;
+        CGFloat runPos = curPos;
         for (int i = 0; i < run->_dwriteGlyphRun.glyphCount; i++) {
-            currPos += run->_dwriteGlyphRun.glyphAdvances[i];
-            if (currPos >= position.x) {
-                return run->_stringIndices[i];
+            if (_GlyphRunIsRTL(run->_dwriteGlyphRun)) {
+                if (runPos <= position.x) {
+                    return run->_stringIndices[i];
+                }
+
+                runPos -= run->_dwriteGlyphRun.glyphAdvances[i];
+            } else {
+                runPos += run->_dwriteGlyphRun.glyphAdvances[i];
+                if (runPos >= position.x) {
+                    return run->_stringIndices[i];
+                }
             }
         }
     }
 
-    if (currPos < position.x) {
-        return line->_strRange.location + line->_strRange.length;
-    }
-
-    return kCFNotFound;
+    return line->_strRange.location + line->_strRange.length;
 }
 
 /**
@@ -375,15 +379,22 @@ CGFloat CTLineGetOffsetForStringIndex(CTLineRef lineRef, CFIndex charIndex, CGFl
                                 run->_stringIndices.begin() - 1;
 
                     if (index >= 0) {
-                        ret = std::accumulate(run->_dwriteGlyphRun.glyphAdvances, run->_dwriteGlyphRun.glyphAdvances + index, ret);
+                        if (_GlyphRunIsRTL(run->_dwriteGlyphRun)) {
+                            ret += std::accumulate(run->_dwriteGlyphRun.glyphAdvances,
+                                                   run->_dwriteGlyphRun.glyphAdvances + index,
+                                                   run->_relativeXOffset,
+                                                   std::minus<CGFloat>());
+                        } else {
+                            ret += std::accumulate(run->_dwriteGlyphRun.glyphAdvances,
+                                                   run->_dwriteGlyphRun.glyphAdvances + index,
+                                                   run->_relativeXOffset);
+                        }
                     }
 
                     break;
                 }
 
-                ret = std::accumulate(run->_dwriteGlyphRun.glyphAdvances,
-                                      run->_dwriteGlyphRun.glyphAdvances + run->_dwriteGlyphRun.glyphCount,
-                                      ret);
+                ret += run->_relativeXOffset;
             }
         }
     }

Frameworks/CoreText/CTRun.mm

@@ -107,7 +107,7 @@ CTRunStatus CTRunGetStatus(CTRunRef runRef) {
     if (runRef) {
         _CTRun* run = static_cast<_CTRun*>(runRef);
 
-        if (run->_dwriteGlyphRun.bidiLevel & 1) {
+        if (_GlyphRunIsRTL(run->_dwriteGlyphRun)) {
             ret |= kCTRunStatusRightToLeft;
             if (!std::is_sorted(run->_stringIndices.cbegin(), run->_stringIndices.cend(), std::greater<UINT16>())) {
                 ret |= kCTRunStatusNonMonotonic;

Frameworks/CoreText/DWriteWrapper_CoreText.mm

@@ -20,6 +20,7 @@
 
 #import "DWriteWrapper_CoreText.h"
 #import "CoreTextInternal.h"
+#import "CGContextInternal.h"
 
 #import <LoggingNative.h>
 #import <StringHelpers.h>
@@ -113,25 +114,29 @@ bool _CloneDWriteGlyphRun(_In_ DWRITE_GLYPH_RUN const* src, _Outptr_ DWRITE_GLYP
 static inline HRESULT __DWriteTextFormatApplyParagraphStyle(const ComPtr<IDWriteTextFormat>& textFormat, CTParagraphStyleRef settings) {
     CTTextAlignment alignment = kCTNaturalTextAlignment;
     if (CTParagraphStyleGetValueForSpecifier(settings, kCTParagraphStyleSpecifierAlignment, sizeof(CTTextAlignment), &alignment)) {
-        RETURN_IF_FAILED(textFormat->SetTextAlignment(__CTAlignmentToDWrite(alignment)));
+        if (alignment != kCTNaturalTextAlignment) {
+            RETURN_IF_FAILED(textFormat->SetTextAlignment(__CTAlignmentToDWrite(alignment)));
+        }
     }
 
     CTWritingDirection direction;
     if (CTParagraphStyleGetValueForSpecifier(settings, kCTParagraphStyleSpecifierBaseWritingDirection, sizeof(direction), &direction)) {
-        DWRITE_READING_DIRECTION dwriteDirection = DWRITE_READING_DIRECTION_LEFT_TO_RIGHT;
-        if (direction == kCTWritingDirectionRightToLeft) {
-            dwriteDirection = DWRITE_READING_DIRECTION_RIGHT_TO_LEFT;
-
-            // DWrite alignment is based upon reading direction whereas CoreText alignment is constant
-            // so we have to flip the writing direction
-            if (alignment == kCTRightTextAlignment || alignment == kCTNaturalTextAlignment) {
-                RETURN_IF_FAILED(textFormat->SetTextAlignment(DWRITE_TEXT_ALIGNMENT_LEADING));
-            } else if (alignment == kCTLeftTextAlignment) {
-                RETURN_IF_FAILED(textFormat->SetTextAlignment(DWRITE_TEXT_ALIGNMENT_TRAILING));
+        if (direction != kCTWritingDirectionNatural) {
+            DWRITE_READING_DIRECTION dwriteDirection = DWRITE_READING_DIRECTION_LEFT_TO_RIGHT;
+            if (direction == kCTWritingDirectionRightToLeft) {
+                dwriteDirection = DWRITE_READING_DIRECTION_RIGHT_TO_LEFT;
+
+                // DWrite alignment is based upon reading direction whereas CoreText alignment is constant
+                // so we have to flip the writing direction
+                if (alignment == kCTRightTextAlignment || alignment == kCTNaturalTextAlignment) {
+                    RETURN_IF_FAILED(textFormat->SetTextAlignment(DWRITE_TEXT_ALIGNMENT_LEADING));
+                } else if (alignment == kCTLeftTextAlignment) {
+                    RETURN_IF_FAILED(textFormat->SetTextAlignment(DWRITE_TEXT_ALIGNMENT_TRAILING));
+                }
             }
-        }
 
-        RETURN_IF_FAILED(textFormat->SetReadingDirection(dwriteDirection));
+            RETURN_IF_FAILED(textFormat->SetReadingDirection(dwriteDirection));
+        }
     }
 
     CTLineBreakMode lineBreakMode;
@@ -371,7 +376,7 @@ HRESULT STDMETHODCALLTYPE IsPixelSnappingDisabled(_In_opt_ void* clientDrawingCo
     };
 
     HRESULT STDMETHODCALLTYPE GetCurrentTransform(_In_opt_ void* clientDrawingContext, _Out_ DWRITE_MATRIX* transform) throw() {
-        *transform = {1, 0, 0, 1, 0, 0};
+        *transform = { 1, 0, 0, 1, 0, 0 };
         return S_OK;
     };
 
@@ -501,16 +506,19 @@ HRESULT STDMETHODCALLTYPE GetPixelsPerDip(_In_opt_ void* clientDrawingContext, _
         if ([runs count] > 0) {
             prevYPosForDraw = yPos;
             line->_runs = runs;
-            line->_strRange.location = static_cast<_CTRun*>(line->_runs[0])->_range.location;
+            _CTRun* firstRun = static_cast<_CTRun*>(runs[0]);
+            line->_strRange.location = firstRun->_range.location;
             line->_strRange.length = stringRange;
             line->_glyphCount = glyphCount;
-            line->_relativeXOffset = static_cast<_CTRun*>(line->_runs[0])->_relativeXOffset;
-            line->_relativeYOffset = static_cast<_CTRun*>(line->_runs[0])->_relativeYOffset;
+            line->_relativeXOffset = firstRun->_relativeXOffset;
+            if (_GlyphRunIsRTL(firstRun->_dwriteGlyphRun)) {
+                // First run is RTL so line's offset is position isn't the same
+                line->_relativeXOffset -= line->_width;
+            }
 
             CGPoint lineOrigin = CGPointZero;
-            if (static_cast<_CTRun*>([line->_runs objectAtIndex:0])->_dwriteGlyphRun.glyphCount != 0) {
-                lineOrigin = { static_cast<_CTRun*>(line->_runs[0])->_glyphOrigins[0].x,
-                               static_cast<_CTRun*>(line->_runs[0])->_glyphOrigins[0].y };
+            if (firstRun->_dwriteGlyphRun.glyphCount != 0) {
+                lineOrigin = { firstRun->_glyphOrigins[0].x, firstRun->_glyphOrigins[0].y };
             }
 
             [frame->_lines addObject:line];

Frameworks/include/CGContextInternal.h

@@ -54,6 +54,10 @@ struct GlyphRunData {
 
 COREGRAPHICS_EXPORT void _CGContextDrawGlyphRuns(CGContextRef ctx, GlyphRunData* glyphRuns, size_t runCount);
 
+inline bool _GlyphRunIsRTL(const DWRITE_GLYPH_RUN& run) {
+    return (run.bidiLevel & 1);
+}
+
 COREGRAPHICS_EXPORT const CFStringRef _kCGCharacterShapeAttributeName;
 COREGRAPHICS_EXPORT const CFStringRef _kCGFontAttributeName;
 COREGRAPHICS_EXPORT const CFStringRef _kCGKernAttributeName;

Frameworks/include/CoreGraphics/DWriteWrapper.h

@@ -82,3 +82,4 @@ inline uint32_t _CTToDWriteFontTableTag(uint32_t tag) {
     // CT has the opposite byte order of DWrite, so we need 'BASE' -> 'ESAB'
     return ((tag & 0xff) << 24) | ((tag & 0xff00) << 8) | ((tag & 0xff0000) >> 8) | ((tag & 0xff000000) >> 24);
 }
+

Frameworks/include/CoreTextInternal.h

@@ -78,7 +78,6 @@ inline void _SafeRelease(T** p) {
 @public
     CFRange _strRange;
     CGFloat _relativeXOffset;
-    CGFloat _relativeYOffset;
     CGFloat _width;
     NSUInteger _glyphCount;
     StrongId<NSMutableArray<_CTRun*>> _runs;

tests/unittests/CoreGraphics.drawing/CTDrawingTests.cpp

@@ -620,3 +620,68 @@ static constexpr CGFloat c_rotations[] = { 0.0, 45.0, -30.0 };
 INSTANTIATE_TEST_CASE_P(TextDrawing,
                         TextDrawingMode,
                         ::testing::Combine(::testing::ValuesIn(c_textDrawingModes), ::testing::ValuesIn(c_rotations)));
+
+class RTLText : public WhiteBackgroundTest<PixelByPixelImageComparator<PixelComparisonModeMask<>>>,
+                public ::testing::WithParamInterface<CGFloat> {
+    CFStringRef CreateOutputFilename() {
+        CGFloat degrees = GetParam();
+        return CFStringCreateWithFormat(nullptr, nullptr, CFSTR("TestImage.RTLText.Rotated.%.0f.png"), degrees);
+    }
+};
+
+TEXT_DRAW_TEST_P(RTLText, DrawRTLText) {
+    CGContextRef context = GetDrawingContext();
+    CGRect bounds = GetDrawingBounds();
+
+    // Creates path with current rectangle
+    woc::unique_cf<CGMutablePathRef> path{ CGPathCreateMutable() };
+    CGPathAddRect(path.get(), nullptr, bounds);
+
+    CGAffineTransform textMatrix = CGContextGetTextMatrix(context);
+    CGContextSetTextMatrix(context, CGAffineTransformRotate(textMatrix, GetParam() * M_PI / 180.0));
+    // Create style setting to match given alignment
+    CTParagraphStyleSetting setting[2];
+    CTTextAlignment alignment = kCTRightTextAlignment;
+    setting[0].spec = kCTParagraphStyleSpecifierAlignment;
+    setting[0].valueSize = sizeof(CTTextAlignment);
+    setting[0].value = &alignment;
+    CTWritingDirection writingDirection = kCTWritingDirectionRightToLeft;
+    setting[1].spec = kCTParagraphStyleSpecifierBaseWritingDirection;
+    setting[1].valueSize = sizeof(CTWritingDirection);
+    setting[1].value = &writingDirection;
+
+    woc::unique_cf<CTParagraphStyleRef> paragraphStyle{ CTParagraphStyleCreate(setting, std::extent<decltype(setting)>::value) };
+    woc::unique_cf<CTFontRef> myCFFont{ CTFontCreateWithName(CFSTR("Arial"), 20, nullptr) };
+
+    CFStringRef keys[2] = { kCTFontAttributeName, kCTParagraphStyleAttributeName };
+    CFTypeRef values[2] = { myCFFont.get(), paragraphStyle.get() };
+
+    woc::unique_cf<CFDictionaryRef> dict{ CFDictionaryCreate(nullptr,
+                                                             (const void**)keys,
+                                                             (const void**)values,
+                                                             std::extent<decltype(keys)>::value,
+                                                             &kCFTypeDictionaryKeyCallBacks,
+                                                             &kCFTypeDictionaryValueCallBacks) };
+
+    woc::unique_cf<CFAttributedStringRef> attrString{
+        CFAttributedStringCreate(nullptr,
+                                 CFSTR("אבל אני חייב להסביר לך איך כל הרעיון מוטעה של בגנות הנאה ומשבחי כאב נולד ואני אתן לך את חשבון מלא "
+                                       "של המערכת, להרצות את משנתו בפועל של החוקר הגדול של האמת, הבונה-אדון האדם אושר. אף אחד לא דוחה, לא "
+                                       "אוהב, או נמנע התענוג עצמו, כי זה תענוג, אלא בגלל מי לא יודע איך להמשיך הנאה רציונלי נתקלים התוצאות "
+                                       "כי הם מאוד כואב. גם שוב האם יש מישהו שאוהב או רודף או רצונות לקבל הכאב של עצמה, כי זה כאב, אבל "
+                                       "בגלל לעתים הנסיבות להתרחש בו עמל וכאב יכולים להשיג לו קצת עונג רב. כדי לקחת דוגמה טריוויאלית, מי "
+                                       "מאיתנו לא מתחייבת פעילות גופנית מאומצת, אלא כדי להשיג יתרון כלשהו ממנו? אבל למי יש זכות למצוא דופי "
+                                       "אדם שבוחר ליהנות הנאה שאין לה השלכות מעצבנות, או מי ימנע כאב מייצר שום הנאה כתוצאה?"),
+                                 dict.get())
+    };
+
+    woc::unique_cf<CTFramesetterRef> framesetter{ CTFramesetterCreateWithAttributedString(attrString.get()) };
+
+    // Creates frame for framesetter with current attributed string
+    woc::unique_cf<CTFrameRef> frame{ CTFramesetterCreateFrame(framesetter.get(), CFRangeMake(0, 0), path, NULL) };
+
+    // Draws the text in the frame
+    CTFrameDraw(frame.get(), context);
+}
+
+INSTANTIATE_TEST_CASE_P(RTLTextDrawing, RTLText, ::testing::ValuesIn(c_rotations));