From 129e7436f712f5ff41c6230e211cd270ad4768c3 Mon Sep 17 00:00:00 2001 From: Jonathan Kew Date: Wed, 21 Aug 2019 15:07:20 +0000 Subject: [PATCH] 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 --- gfx/thebes/gfxFont.cpp | 8 ++-- gfx/thebes/gfxTextRun.cpp | 45 +++++++++++++-------- gfx/thebes/gfxTextRun.h | 39 ++++++++++++++++-- layout/generic/nsTextRunTransformations.cpp | 2 +- layout/mathml/nsMathMLChar.cpp | 2 +- layout/painting/nsCSSRendering.cpp | 6 ++- 6 files changed, 76 insertions(+), 26 deletions(-) diff --git a/gfx/thebes/gfxFont.cpp b/gfx/thebes/gfxFont.cpp index 4af35940c5ea..13ce01e3240e 100644 --- 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 tempRun(gfxTextRun::Create( ¶ms, 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)) { diff --git a/gfx/thebes/gfxTextRun.cpp b/gfx/thebes/gfxTextRun.cpp index 7932c79a4eb8..d9a979c2bf40 100644 --- 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 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 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 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) { diff --git a/gfx/thebes/gfxTextRun.h b/gfx/thebes/gfxTextRun.h index f09e43a1586a..6165d741551f 100644 --- 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; diff --git a/layout/generic/nsTextRunTransformations.cpp b/layout/generic/nsTextRunTransformations.cpp index 10015419c5d9..405e0a7c4ae8 100644 --- 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(); diff --git a/layout/mathml/nsMathMLChar.cpp b/layout/mathml/nsMathMLChar.cpp index a42b4baa081a..16220e06b900 100644 --- a/layout/mathml/nsMathMLChar.cpp +++ b/layout/mathml/nsMathMLChar.cpp @@ -519,7 +519,7 @@ already_AddRefed 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; diff --git a/layout/painting/nsCSSRendering.cpp b/layout/painting/nsCSSRendering.cpp index b83834aeb624..813d2a688f02 100644 --- 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()),