Bug 1573249 - patch 2 - Don't apply skip-ink to runs of CJK text, because it looks bad with many fonts. r=dholbert

Differential Revision: https://phabricator.services.mozilla.com/D42529 --HG-- extra : moz-landing-system : lando
2019-08-21 15:07:20 +00:00 · 2019-08-21 15:07:20 +00:00 · 129e7436f7
--- a/gfx/thebes/gfxFont.cpp
+++ b/gfx/thebes/gfxFont.cpp
@ -3202,6 +3202,8 @@ bool gfxFont::InitFakeSmallCapsRun(DrawTarget* aDrawTarget,
    smallCapsFont = this;
  }

+  bool isCJK = gfxTextRun::IsCJKScript(aScript);
+
  enum RunCaseAction { kNoChange, kUppercaseReduce, kUppercase };

  RunCaseAction runAction = kNoChange;
@ -3262,7 +3264,7 @@ bool gfxFont::InitFakeSmallCapsRun(DrawTarget* aDrawTarget,
        case kNoChange:
          // just use the current font and the existing string
          aTextRun->AddGlyphRun(f, aMatchType, aOffset + runStart, true,
-                                aOrientation);
+                                aOrientation, isCJK);
          if (!f->SplitAndInitTextRun(aDrawTarget, aTextRun, aText + runStart,
                                      aOffset + runStart, runLength, aScript,
                                      aOrientation)) {
@ -3299,7 +3301,7 @@ bool gfxFont::InitFakeSmallCapsRun(DrawTarget* aDrawTarget,
            RefPtr<gfxTextRun> tempRun(gfxTextRun::Create(
                &params, convertedString.Length(), aTextRun->GetFontGroup(),
                gfx::ShapedTextFlags(), nsTextFrameUtils::Flags()));
-            tempRun->AddGlyphRun(f, aMatchType, 0, true, aOrientation);
+            tempRun->AddGlyphRun(f, aMatchType, 0, true, aOrientation, isCJK);
            if (!f->SplitAndInitTextRun(
                    aDrawTarget, tempRun.get(), convertedString.BeginReading(),
                    0, convertedString.Length(), aScript, aOrientation)) {
@ -3317,7 +3319,7 @@ bool gfxFont::InitFakeSmallCapsRun(DrawTarget* aDrawTarget,
            }
          } else {
            aTextRun->AddGlyphRun(f, aMatchType, aOffset + runStart, true,
-                                  aOrientation);
+                                  aOrientation, isCJK);
            if (!f->SplitAndInitTextRun(
                    aDrawTarget, aTextRun, convertedString.BeginReading(),
                    aOffset + runStart, runLength, aScript, aOrientation)) {
--- a/gfx/thebes/gfxTextRun.cpp
+++ b/gfx/thebes/gfxTextRun.cpp
@ -1247,7 +1247,7 @@ uint32_t gfxTextRun::FindFirstGlyphRunContaining(uint32_t aOffset) const {

 void gfxTextRun::AddGlyphRun(gfxFont* aFont, FontMatchType aMatchType,
                             uint32_t aUTF16Offset, bool aForceNewRun,
-                             gfx::ShapedTextFlags aOrientation) {
+                             gfx::ShapedTextFlags aOrientation, bool aIsCJK) {
  NS_ASSERTION(aFont, "adding glyph run for null font!");
  NS_ASSERTION(aOrientation != gfx::ShapedTextFlags::TEXT_ORIENT_VERTICAL_MIXED,
               "mixed orientation should have been resolved");
@ -1258,7 +1258,7 @@ void gfxTextRun::AddGlyphRun(gfxFont* aFont, FontMatchType aMatchType,
    // We don't currently have an array.
    if (!mSingleGlyphRun.mFont) {
      // This is the first glyph run: just store it directly.
-      mSingleGlyphRun.SetProperties(aFont, aOrientation, aMatchType);
+      mSingleGlyphRun.SetProperties(aFont, aOrientation, aIsCJK, aMatchType);
      mSingleGlyphRun.mCharacterOffset = aUTF16Offset;
      return;
    }
@ -1273,7 +1273,7 @@ void gfxTextRun::AddGlyphRun(gfxFont* aFont, FontMatchType aMatchType,
                 "Glyph runs out of order (and run not forced)");

    // Don't append a run if the font is already the one we want
-    if (lastGlyphRun->Matches(aFont, aOrientation, aMatchType)) {
+    if (lastGlyphRun->Matches(aFont, aOrientation, aIsCJK, aMatchType)) {
      return;
    }

@ -1284,7 +1284,7 @@ void gfxTextRun::AddGlyphRun(gfxFont* aFont, FontMatchType aMatchType,
      // font as the new one wants, merge with it instead of creating
      // adjacent runs with the same font
      if (numGlyphRuns > 1 && mGlyphRunArray[numGlyphRuns - 2].Matches(
-                                  aFont, aOrientation, aMatchType)) {
+                                  aFont, aOrientation, aIsCJK, aMatchType)) {
        mGlyphRunArray.TruncateLength(numGlyphRuns - 1);
        if (mGlyphRunArray.Length() == 1) {
          ConvertFromGlyphRunArray();
@ -1292,7 +1292,7 @@ void gfxTextRun::AddGlyphRun(gfxFont* aFont, FontMatchType aMatchType,
        return;
      }

-      lastGlyphRun->SetProperties(aFont, aOrientation, aMatchType);
+      lastGlyphRun->SetProperties(aFont, aOrientation, aIsCJK, aMatchType);
      return;
    }
  }
@ -1306,7 +1306,7 @@ void gfxTextRun::AddGlyphRun(gfxFont* aFont, FontMatchType aMatchType,
  }

  GlyphRun* glyphRun = mGlyphRunArray.AppendElement();
-  glyphRun->SetProperties(aFont, aOrientation, aMatchType);
+  glyphRun->SetProperties(aFont, aOrientation, aIsCJK, aMatchType);
  glyphRun->mCharacterOffset = aUTF16Offset;
 }

@ -1333,7 +1333,8 @@ void gfxTextRun::SortGlyphRuns() {
    // just be skipped; the last GlyphRun will cover its character range.
    MOZ_ASSERT(run.mFont != nullptr);
    if (!prevRun ||
-        !prevRun->Matches(run.mFont, run.mOrientation, run.mMatchType)) {
+        !prevRun->Matches(run.mFont, run.mOrientation, run.mIsCJK,
+                          run.mMatchType)) {
      // If two font runs have the same character offset, Sort() will have
      // randomized their order!
      MOZ_ASSERT(prevRun == nullptr ||
@ -1469,6 +1470,7 @@ void gfxTextRun::CopyGlyphDataFrom(gfxTextRun* aSource, Range aRange,
    gfxFont* font = iter.GetGlyphRun()->mFont;
    MOZ_ASSERT(!prevRun || !prevRun->Matches(iter.GetGlyphRun()->mFont,
                                             iter.GetGlyphRun()->mOrientation,
+                                             iter.GetGlyphRun()->mIsCJK,
                                             FontMatchType::Kind::kUnspecified),
               "Glyphruns not coalesced?");
 #ifdef DEBUG
@ -1494,7 +1496,7 @@ void gfxTextRun::CopyGlyphDataFrom(gfxTextRun* aSource, Range aRange,

    AddGlyphRun(font, iter.GetGlyphRun()->mMatchType,
                start - aRange.start + aDest, false,
-                iter.GetGlyphRun()->mOrientation);
+                iter.GetGlyphRun()->mOrientation, iter.GetGlyphRun()->mIsCJK);
  }
 }

@ -1524,8 +1526,13 @@ void gfxTextRun::SetSpaceGlyph(gfxFont* aFont, DrawTarget* aDrawTarget,
      aDrawTarget, &space, 1, gfxShapedWord::HashMix(0, ' '), Script::LATIN,
      vertical, mAppUnitsPerDevUnit, flags, roundingFlags, nullptr);
  if (sw) {
+    const GlyphRun* prevRun = TrailingGlyphRun();
+    bool isCJK = prevRun && prevRun->mFont == aFont &&
+                         prevRun->mOrientation == aOrientation
+                     ? prevRun->mIsCJK
+                     : false;
    AddGlyphRun(aFont, FontMatchType::Kind::kUnspecified, aCharIndex, false,
-                aOrientation);
+                aOrientation, isCJK);
    CopyGlyphDataFrom(sw, aCharIndex);
    GetCharacterGlyphs()[aCharIndex].SetIsSpace();
  }
@ -1549,8 +1556,13 @@ bool gfxTextRun::SetSpaceGlyphIfSimple(gfxFont* aFont, uint32_t aCharIndex,
    return false;
  }

+  const GlyphRun* prevRun = TrailingGlyphRun();
+  bool isCJK = prevRun && prevRun->mFont == aFont &&
+                       prevRun->mOrientation == aOrientation
+                   ? prevRun->mIsCJK
+                   : false;
  AddGlyphRun(aFont, FontMatchType::Kind::kUnspecified, aCharIndex, false,
-              aOrientation);
+              aOrientation, isCJK);
  CompressedGlyph g =
      CompressedGlyph::MakeSimpleGlyph(spaceWidthAppUnits, spaceGlyph);
  if (aSpaceChar == ' ') {
@ -2139,7 +2151,7 @@ already_AddRefed<gfxTextRun> gfxFontGroup::MakeSpaceTextRun(
    // them, and always create at least size 1 fonts, i.e. they still
    // render something for size 0 fonts.
    textRun->AddGlyphRun(font, FontMatchType::Kind::kUnspecified, 0, false,
-                         orientation);
+                         orientation, false);
  } else {
    if (font->GetSpaceGlyph()) {
      // Normally, the font has a cached space glyph, so we can avoid
@ -2177,7 +2189,7 @@ already_AddRefed<gfxTextRun> gfxFontGroup::MakeBlankTextRun(
    orientation = ShapedTextFlags::TEXT_ORIENT_VERTICAL_UPRIGHT;
  }
  textRun->AddGlyphRun(GetFirstValidFont(), FontMatchType::Kind::kUnspecified,
-                       0, false, orientation);
+                       0, false, orientation, false);
  return textRun.forget();
 }

@ -2475,6 +2487,7 @@ void gfxFontGroup::InitScriptRun(DrawTarget* aDrawTarget, gfxTextRun* aTextRun,
  ComputeRanges(fontRanges, aString, aLength, aRunScript, orientation);
  uint32_t numRanges = fontRanges.Length();
  bool missingChars = false;
+  bool isCJK = gfxTextRun::IsCJKScript(aRunScript);

  for (uint32_t r = 0; r < numRanges; r++) {
    const TextRange& range = fontRanges[r];
@ -2485,7 +2498,7 @@ void gfxFontGroup::InitScriptRun(DrawTarget* aDrawTarget, gfxTextRun* aTextRun,
      // common case - just do glyph layout and record the
      // resulting positioned glyphs
      aTextRun->AddGlyphRun(matchedFont, range.matchType, aOffset + runStart,
-                            (matchedLength > 0), range.orientation);
+                            (matchedLength > 0), range.orientation, isCJK);
      if (!matchedFont->SplitAndInitTextRun(
              aDrawTarget, aTextRun, aString + runStart, aOffset + runStart,
              matchedLength, aRunScript, range.orientation)) {
@ -2520,7 +2533,7 @@ void gfxFontGroup::InitScriptRun(DrawTarget* aDrawTarget, gfxTextRun* aTextRun,
        RefPtr<gfxFont> subSuperFont = matchedFont->GetSubSuperscriptFont(
            aTextRun->GetAppUnitsPerDevUnit());
        aTextRun->AddGlyphRun(subSuperFont, range.matchType, aOffset + runStart,
-                              (matchedLength > 0), range.orientation);
+                              (matchedLength > 0), range.orientation, isCJK);
        if (!subSuperFont->SplitAndInitTextRun(
                aDrawTarget, aTextRun, aString + runStart, aOffset + runStart,
                matchedLength, aRunScript, range.orientation)) {
@ -2554,7 +2567,7 @@ void gfxFontGroup::InitScriptRun(DrawTarget* aDrawTarget, gfxTextRun* aTextRun,

        // do glyph layout and record the resulting positioned glyphs
        aTextRun->AddGlyphRun(matchedFont, range.matchType, aOffset + runStart,
-                              (matchedLength > 0), range.orientation);
+                              (matchedLength > 0), range.orientation, isCJK);
        if (!matchedFont->SplitAndInitTextRun(
                aDrawTarget, aTextRun, aString + runStart, aOffset + runStart,
                matchedLength, aRunScript, range.orientation)) {
@ -2565,7 +2578,7 @@ void gfxFontGroup::InitScriptRun(DrawTarget* aDrawTarget, gfxTextRun* aTextRun,
    } else {
      aTextRun->AddGlyphRun(mainFont, FontMatchType::Kind::kFontGroup,
                            aOffset + runStart, (matchedLength > 0),
-                            range.orientation);
+                            range.orientation, isCJK);
    }

    if (!matchedFont) {
--- a/gfx/thebes/gfxTextRun.h
+++ b/gfx/thebes/gfxTextRun.h
@ -471,21 +471,24 @@ class gfxTextRun : public gfxShapedText {
    mozilla::gfx::ShapedTextFlags
        mOrientation;  // gfxTextRunFactory::TEXT_ORIENT_* value
    FontMatchType mMatchType;
+    bool mIsCJK;  // Whether the text was a CJK script run (used to decide if
+                  // text-decoration-skip-ink should not be applied)

    // Set up the properties (but NOT offset) of the GlyphRun.
    void SetProperties(gfxFont* aFont,
-                       mozilla::gfx::ShapedTextFlags aOrientation,
+                       mozilla::gfx::ShapedTextFlags aOrientation, bool aIsCJK,
                       FontMatchType aMatchType) {
      mFont = aFont;
      mOrientation = aOrientation;
+      mIsCJK = aIsCJK;
      mMatchType = aMatchType;
    }

    // Return whether the GlyphRun matches the given properties;
    // the given FontMatchType will be added to the run if not present.
    bool Matches(gfxFont* aFont, mozilla::gfx::ShapedTextFlags aOrientation,
-                 FontMatchType aMatchType) {
-      if (mFont == aFont && mOrientation == aOrientation) {
+                 bool aIsCJK, FontMatchType aMatchType) {
+      if (mFont == aFont && mOrientation == aOrientation && mIsCJK == aIsCJK) {
        mMatchType.kind |= aMatchType.kind;
        if (mMatchType.generic == mozilla::StyleGenericFontFamily::None) {
          mMatchType.generic = aMatchType.generic;
@ -496,6 +499,27 @@ class gfxTextRun : public gfxShapedText {
    }
  };

+  // Script run codes that we will mark as CJK to suppress skip-ink behavior.
+  static inline bool IsCJKScript(Script aScript) {
+    switch (aScript) {
+      case Script::BOPOMOFO:
+      case Script::HAN:
+      case Script::HANGUL:
+      case Script::HIRAGANA:
+      case Script::KATAKANA:
+      case Script::KATAKANA_OR_HIRAGANA:
+      case Script::SIMPLIFIED_HAN:
+      case Script::TRADITIONAL_HAN:
+      case Script::JAPANESE:
+      case Script::KOREAN:
+      case Script::HAN_WITH_BOPOMOFO:
+      case Script::JAMO:
+        return true;
+      default:
+        return false;
+    }
+  }
+
  class MOZ_STACK_CLASS GlyphRunIterator {
   public:
    GlyphRunIterator(const gfxTextRun* aTextRun, Range aRange,
@ -554,7 +578,7 @@ class gfxTextRun : public gfxShapedText {
   */
  void AddGlyphRun(gfxFont* aFont, FontMatchType aMatchType,
                   uint32_t aUTF16Offset, bool aForceNewRun,
-                   mozilla::gfx::ShapedTextFlags aOrientation);
+                   mozilla::gfx::ShapedTextFlags aOrientation, bool aIsCJK);
  void ResetGlyphRuns() {
    if (mHasGlyphRunArray) {
      MOZ_ASSERT(mGlyphRunArray.Length() > 1);
@ -638,6 +662,13 @@ class gfxTextRun : public gfxShapedText {
      return &mSingleGlyphRun;
    }
  }
+
+  const GlyphRun* TrailingGlyphRun() const {
+    uint32_t count;
+    const GlyphRun* runs = GetGlyphRuns(&count);
+    return count ? runs + count - 1 : nullptr;
+  }
+
  // Returns the index of the GlyphRun containing the given offset.
  // Returns mGlyphRuns.Length() when aOffset is mCharacterCount.
  uint32_t FindFirstGlyphRunContaining(uint32_t aOffset) const;
--- a/layout/generic/nsTextRunTransformations.cpp
+++ b/layout/generic/nsTextRunTransformations.cpp
@ -134,7 +134,7 @@ void MergeCharactersInTextRun(gfxTextRun* aDest, gfxTextRun* aSrc,
  while (iter.NextRun()) {
    const gfxTextRun::GlyphRun* run = iter.GetGlyphRun();
    aDest->AddGlyphRun(run->mFont, run->mMatchType, offset, false,
-                       run->mOrientation);
+                       run->mOrientation, run->mIsCJK);

    bool anyMissing = false;
    uint32_t mergeRunStart = iter.GetStringStart();
--- a/layout/mathml/nsMathMLChar.cpp
+++ b/layout/mathml/nsMathMLChar.cpp
@ -519,7 +519,7 @@ already_AddRefed<gfxTextRun> nsOpenTypeTable::MakeTextRun(
                         nsTextFrameUtils::Flags());
  textRun->AddGlyphRun(aFontGroup->GetFirstValidFont(),
                       FontMatchType::Kind::kFontGroup, 0, false,
-                       gfx::ShapedTextFlags::TEXT_ORIENT_HORIZONTAL);
+                       gfx::ShapedTextFlags::TEXT_ORIENT_HORIZONTAL, false);
  // We don't care about CSS writing mode here;
  // math runs are assumed to be horizontal.
  gfxTextRun::DetailedGlyph detailedGlyph;
--- a/layout/painting/nsCSSRendering.cpp
+++ b/layout/painting/nsCSSRendering.cpp
@ -4100,11 +4100,15 @@ void nsCSSRendering::PaintDecorationLine(

  while (iter.NextRun()) {
    if (iter.GetGlyphRun()->mOrientation ==
-        mozilla::gfx::ShapedTextFlags::TEXT_ORIENT_VERTICAL_UPRIGHT) {
+            mozilla::gfx::ShapedTextFlags::TEXT_ORIENT_VERTICAL_UPRIGHT ||
+        iter.GetGlyphRun()->mIsCJK) {
      // We don't support upright text in vertical modes currently
      // (see https://bugzilla.mozilla.org/show_bug.cgi?id=1572294),
      // but we do need to update textPos so that following runs will be
      // correctly positioned.
+      // We also don't apply skip-ink to CJK text runs because many fonts
+      // have an underline that looks really bad if this is done
+      // (see https://bugzilla.mozilla.org/show_bug.cgi?id=1573249).
      textPos.fX +=
          textRun->GetAdvanceWidth(
              gfxTextRun::Range(iter.GetStringStart(), iter.GetStringEnd()),