Attachment 349635 Details for Bug 189448 – Patch

[patch] Patch

bug-189448-20180912235709.patch (text/plain), 14.64 KB, created by Myles C. Maxfield on 2018-09-12 23:57:10 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Myles C. Maxfield

Created: 2018-09-12 23:57:10 PDT

Size: 14.64 KB

patch

obsolete

>Subversion Revision: 235966
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index f024f6b73f371ab33db786c71111c6a47a774f40..d9a3cdcfc4d8008faa46faeb71ba23b25c1b5c57 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,40 @@
>+2018-09-12  Myles C. Maxfield  <mmaxfield@apple.com>
>+
>+        Synthetic bold additional advances need to be applied after shaping
>+        https://bugs.webkit.org/show_bug.cgi?id=189448
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        When laying out text, the first step is to gather the glyphs and advances that correspond to the
>+        text you're trying to lay out. Then, when you have those, you apply the shaping engine which is
>+        implemented by Core Text (on the Cocoa ports). The shaping engine will modify the sequence of
>+        glyphs and advances according to rules in the font and rules in the engine.
>+
>+        The shaping engine expects that it is passed the glyphs and advances directly from the font.
>+        However, we were previously applying synthetic bold before shaping. This means the shaping engine
>+        was operating on the wrong data, and therefore the result was bogus. We didn't notice until now
>+        because most of the transformations will preserve things like synthetic bold, but some fonts (like
>+        Osaka) won't.
>+
>+        It's only a problem in the simple text codepath because the complex text codepath already applies
>+        the synthetic bold after shaping. This is because, in the complex text codepath, the glyph mapping
>+        step and the shaping step are both run by the same individual Core Text function call. We couldn't
>+        split them up even if we wanted to.
>+
>+        The solution is to apply synthetic bold after the shaping step.
>+
>+        Test: fast/text/simple-synthetic-bold.html
>+
>+        * platform/graphics/Font.h:
>+        (WebCore::Font::syntheticBoldOffset const):
>+        (WebCore::Font::widthForGlyph const):
>+        * platform/graphics/WidthIterator.cpp:
>+        (WebCore::WidthIterator::applyFontTransforms):
>+        (WebCore::WidthIterator::advanceInternal):
>+        * platform/graphics/WidthIterator.h:
>+        * platform/graphics/cocoa/FontCocoa.mm:
>+        (WebCore::Font::platformWidthForGlyph const):
>+
> 2018-09-11  Ryosuke Niwa  <rniwa@webkit.org>
> 
>         imported/w3c/web-platform-tests/shadow-dom/form-control-form-attribute.html hits assertion
>diff --git a/Source/WebCore/platform/graphics/Font.h b/Source/WebCore/platform/graphics/Font.h
>index 52edbae94537fa720d4714b181ed26f727b6e2be..47fe41730328d2db2992c93aa2fe79f901fa63ab 100644
>--- a/Source/WebCore/platform/graphics/Font.h
>+++ b/Source/WebCore/platform/graphics/Font.h
>@@ -157,9 +157,7 @@ public:
>         m_adjustedSpaceWidth = spaceWidth;
>     }
> 
>-#if USE(CG) || USE(DIRECT2D) || USE(CAIRO)
>     float syntheticBoldOffset() const { return m_syntheticBoldOffset; }
>-#endif
> 
>     Glyph spaceGlyph() const { return m_spaceGlyph; }
>     void setSpaceGlyph(Glyph spaceGlyph) { m_spaceGlyph = spaceGlyph; }
>@@ -301,9 +299,7 @@ private:
>     float m_spaceWidth { 0 };
>     float m_adjustedSpaceWidth { 0 };
> 
>-#if USE(CG) || USE(DIRECT2D) || USE(CAIRO)
>     float m_syntheticBoldOffset { 0 };
>-#endif
> 
>     unsigned m_treatAsFixedPitch : 1;
>     unsigned m_isInterstitial : 1; // Whether or not this custom font is the last resort placeholder for a loading font
>@@ -356,15 +352,13 @@ ALWAYS_INLINE float Font::widthForGlyph(Glyph glyph) const
>         return width;
> 
> #if ENABLE(OPENTYPE_VERTICAL)
>-    if (m_verticalData) {
>-#if USE(CG) || USE(DIRECT2D) || USE(CAIRO)
>-        width = m_verticalData->advanceHeight(this, glyph) + m_syntheticBoldOffset;
>-#else
>+    if (m_verticalData)
>         width = m_verticalData->advanceHeight(this, glyph);
>+    else
> #endif
>-    } else
>-#endif
>+    {
>         width = platformWidthForGlyph(glyph);
>+    }
> 
>     m_glyphToWidthMap.setMetricsForGlyph(glyph, width);
>     return width;
>diff --git a/Source/WebCore/platform/graphics/FontCascade.cpp b/Source/WebCore/platform/graphics/FontCascade.cpp
>index 08692534a4747c0bc8229d5a4701c40f58060491..726e33060f79228db9f0571681c1e235eea1b0dc 100644
>--- a/Source/WebCore/platform/graphics/FontCascade.cpp
>+++ b/Source/WebCore/platform/graphics/FontCascade.cpp
>@@ -369,6 +369,7 @@ float FontCascade::widthOfTextRange(const TextRun& run, unsigned from, unsigned
> #endif
>     } else {
>         WidthIterator simpleIterator(this, run, fallbackFonts);
>+        // FIXME: Passing a glyph buffer is mandatory if you want correct results.
>         simpleIterator.advance(from, nullptr);
>         offsetBeforeRange = simpleIterator.runWidthSoFar();
>         simpleIterator.advance(to, nullptr);
>@@ -1519,6 +1520,7 @@ float FontCascade::floatWidthForSimpleText(const TextRun& run, HashSet<const Fon
> {
>     WidthIterator it(this, run, fallbackFonts, glyphOverflow);
>     GlyphBuffer glyphBuffer;
>+    // FIXME: Passing a glyph buffer is mandatory if you want correct results.
>     it.advance(run.length(), (enableKerning() || requiresShaping()) ? &glyphBuffer : nullptr);
> 
>     if (glyphOverflow) {
>diff --git a/Source/WebCore/platform/graphics/WidthIterator.cpp b/Source/WebCore/platform/graphics/WidthIterator.cpp
>index 9bde879703f6a4df114996a2a8cc77e1c717fc59..e36fb7c3076513ccb959754d2d4e430a76ed9817 100644
>--- a/Source/WebCore/platform/graphics/WidthIterator.cpp
>+++ b/Source/WebCore/platform/graphics/WidthIterator.cpp
>@@ -94,6 +94,10 @@ inline float WidthIterator::applyFontTransforms(GlyphBuffer* glyphBuffer, bool l
> {
>     ASSERT_UNUSED(previousCharacter, shouldApplyFontTransforms(glyphBuffer, lastGlyphCount, previousCharacter) != WidthIterator::TransformsType::None);
> 
>+    // FIXME: This is totally busted.
>+    // The only way you can get correct answers is if you do glyph shaping,
>+    // and the only way we do shaping is if you pass in a non-null glyph buffer.
>+    // However, the API to WidthIterator marks the GlyphBuffer as optional.
>     if (!glyphBuffer)
>         return 0;
> 
>@@ -125,7 +129,9 @@ inline float WidthIterator::applyFontTransforms(GlyphBuffer* glyphBuffer, bool l
>         const OriginalAdvancesForCharacterTreatedAsSpace& originalAdvances = charactersTreatedAsSpace[i].second;
>         if (spaceOffset && !originalAdvances.characterIsSpace)
>             glyphBuffer->advances(spaceOffset - 1)->setWidth(originalAdvances.advanceBeforeCharacter);
>-        glyphBuffer->advances(spaceOffset)->setWidth(originalAdvances.advanceAtCharacter);
>+        // The width of the space glyph isn't inflated by the synthetic bold width.
>+        // We subtract it here, and add it back in inside advanceInternal() so the net effect is it doesn't change.
>+        glyphBuffer->advances(spaceOffset)->setWidth(originalAdvances.advanceAtCharacter - font->syntheticBoldOffset());
>     }
>     charactersTreatedAsSpace.clear();
> 
>@@ -213,16 +219,13 @@ inline unsigned WidthIterator::advanceInternal(TextIterator& textIterator, Glyph
> 
>         // Now that we have a glyph and font data, get its width.
>         float width;
>-        if (character == '\t' && m_run.allowTabs())
>+        if (character == tabCharacter && m_run.allowTabs())
>             width = m_font->tabWidth(*font, m_run.tabSize(), m_run.xPos() + m_runWidthSoFar + widthSinceLastRounding);
>-        else {
>+        else
>             width = font->widthForGlyph(glyph);
> 
>-            // SVG uses horizontalGlyphStretch(), when textLength is used to stretch/squeeze text.
>-            width *= m_run.horizontalGlyphStretch();
>-        }
>-
>         if (font != lastFontData && width) {
>+            auto beginningOfTransformation = lastGlyphCount;
>             auto transformsType = shouldApplyFontTransforms(glyphBuffer, lastGlyphCount, previousCharacter);
>             if (transformsType != TransformsType::None) {
>                 m_runWidthSoFar += applyFontTransforms(glyphBuffer, m_run.ltr(), lastGlyphCount, lastFontData, previousCharacter, transformsType == TransformsType::Forced, charactersTreatedAsSpace);
>@@ -230,17 +233,20 @@ inline unsigned WidthIterator::advanceInternal(TextIterator& textIterator, Glyph
>                     glyphBuffer->shrink(lastGlyphCount);
>             }
> 
>+            if (glyphBuffer) {
>+                for ( ; beginningOfTransformation < lastGlyphCount; ++beginningOfTransformation) {
>+                    auto* advancePtr = glyphBuffer->advances(beginningOfTransformation);
>+                    auto shapedWidth = advancePtr->width();
>+                    auto updatedWidth = (shapedWidth + font->syntheticBoldOffset()) * m_run.horizontalGlyphStretch();
>+                    m_runWidthSoFar += updatedWidth - shapedWidth;
>+                    advancePtr->setWidth(updatedWidth);
>+                }
>+            }
>+
>             lastFontData = font;
>             if (m_fallbackFonts && font != &primaryFont) {
>-                // FIXME: This does a little extra work that could be avoided if
>-                // glyphDataForCharacter() returned whether it chose to use a small caps font.
>-                if (!m_font->isSmallCaps() || character == u_toupper(character))
>-                    m_fallbackFonts->add(font);
>-                else {
>-                    const GlyphData& uppercaseGlyphData = m_font->glyphDataForCharacter(u_toupper(character), rtl);
>-                    if (uppercaseGlyphData.font != &primaryFont)
>-                        m_fallbackFonts->add(uppercaseGlyphData.font);
>-                }
>+                ASSERT(!m_font->isSmallCaps());
>+                m_fallbackFonts->add(font);
>             }
>         }
> 
>@@ -333,7 +339,7 @@ inline unsigned WidthIterator::advanceInternal(TextIterator& textIterator, Glyph
>         widthSinceLastRounding += width;
> 
>         if (glyphBuffer)
>-            glyphBuffer->add(glyph, font, (rtl ? oldWidth + lastRoundingWidth : width), currentCharacter);
>+            glyphBuffer->add(glyph, font, rtl ? oldWidth + lastRoundingWidth : width, currentCharacter);
> 
>         lastRoundingWidth = width - oldWidth;
> 
>@@ -353,12 +359,23 @@ inline unsigned WidthIterator::advanceInternal(TextIterator& textIterator, Glyph
>     }
> 
>     auto transformsType = shouldApplyFontTransforms(glyphBuffer, lastGlyphCount, previousCharacter);
>+    auto beginningOfTransformation = lastGlyphCount;
>     if (transformsType != TransformsType::None) {
>         m_runWidthSoFar += applyFontTransforms(glyphBuffer, m_run.ltr(), lastGlyphCount, lastFontData, previousCharacter, transformsType == TransformsType::Forced, charactersTreatedAsSpace);
>         if (glyphBuffer)
>             glyphBuffer->shrink(lastGlyphCount);
>     }
> 
>+    if (glyphBuffer) {
>+        for ( ; beginningOfTransformation < glyphBuffer->advancesCount(); ++beginningOfTransformation) {
>+            auto* advancePtr = glyphBuffer->advances(beginningOfTransformation);
>+            auto shapedWidth = advancePtr->width();
>+            auto updatedWidth = (shapedWidth + lastFontData->syntheticBoldOffset()) * m_run.horizontalGlyphStretch();
>+            m_runWidthSoFar += updatedWidth - shapedWidth;
>+            advancePtr->setWidth(updatedWidth);
>+        }
>+    }
>+
>     unsigned consumedCharacters = textIterator.currentIndex() - m_currentCharacter;
>     m_currentCharacter = textIterator.currentIndex();
>     m_runWidthSoFar += widthSinceLastRounding;
>diff --git a/Source/WebCore/platform/graphics/WidthIterator.h b/Source/WebCore/platform/graphics/WidthIterator.h
>index c45fdb8c36a3fe2f3ef86c8959ef5516a3901257..dc3f636eabbdca66d9529cd0a9f2e948de3b8a54 100644
>--- a/Source/WebCore/platform/graphics/WidthIterator.h
>+++ b/Source/WebCore/platform/graphics/WidthIterator.h
>@@ -53,6 +53,7 @@ public:
>     const TextRun& run() const { return m_run; }
>     float runWidthSoFar() const { return m_runWidthSoFar; }
> 
>+    // FIXME: these shouldn't be public.
>     const FontCascade* m_font;
> 
>     const TextRun& m_run;
>diff --git a/Source/WebCore/platform/graphics/cocoa/FontCocoa.mm b/Source/WebCore/platform/graphics/cocoa/FontCocoa.mm
>index f379106a0d219636f2bb7dc941d10b2134e11cb0..d1dc2b1b24590272a7650171c1690df167c0c34e 100644
>--- a/Source/WebCore/platform/graphics/cocoa/FontCocoa.mm
>+++ b/Source/WebCore/platform/graphics/cocoa/FontCocoa.mm
>@@ -594,7 +594,7 @@ float Font::platformWidthForGlyph(Glyph glyph) const
>         else
>             CTFontGetUnsummedAdvancesForGlyphsAndStyle(m_platformData.ctFont(), orientation, style, &glyph, &advance, 1);
>     }
>-    return advance.width + m_syntheticBoldOffset;
>+    return advance.width;
> }
> 
> bool Font::platformSupportsCodePoint(UChar32 character) const
>diff --git a/Source/WebCore/rendering/svg/SVGTextMetrics.cpp b/Source/WebCore/rendering/svg/SVGTextMetrics.cpp
>index 78e06b537c3139f60f03b26c8e769375716af5ed..064b4da1272efa834eb2345f79cbd42c2f88453b 100644
>--- a/Source/WebCore/rendering/svg/SVGTextMetrics.cpp
>+++ b/Source/WebCore/rendering/svg/SVGTextMetrics.cpp
>@@ -21,7 +21,6 @@
> #include "SVGTextMetrics.h"
> 
> #include "RenderSVGInlineText.h"
>-#include "WidthIterator.h"
> 
> namespace WebCore {
> 
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 0f7913db91fba7da667f8c1b20d3cdf67c230867..1a0844b08df420365cd5549640ce3c532700712b 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,12 @@
>+2018-09-12  Myles C. Maxfield  <mmaxfield@apple.com>
>+
>+        Synthetic bold additional advances need to be applied after shaping
>+        https://bugs.webkit.org/show_bug.cgi?id=189448
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * fast/text/simple-synthetic-bold.html: Added.
>+
> 2018-09-12  Dean Jackson  <dino@grorg.org>
> 
>         Header parsing for experimental and internal debug features
>diff --git a/LayoutTests/fast/text/simple-synthetic-bold.html b/LayoutTests/fast/text/simple-synthetic-bold.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..17402cf45b74abc9b63b9d04f2ab2f94ea94392a
>--- /dev/null
>+++ b/LayoutTests/fast/text/simple-synthetic-bold.html
>@@ -0,0 +1,29 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<meta charset="utf-8">
>+<script src="../../resources/js-test-pre.js"></script>
>+</head>
>+<body>
>+This test makes sure that the Osaka font has correct synthetic bold.
>+To run the test, select each character with your mouse.
>+The highlight should line up with each character properly.
>+<div style="font: bold 16px 'Osaka';"><span id="target" style="background: red;">ããããããããããããããã</span></div>
>+<script>
>+let element = document.getElementById("target");
>+let target = element.firstChild;
>+const selection = window.getSelection();
>+const range = document.createRange();
>+range.setStart(target, 0);
>+range.setEnd(target, 5);
>+selection.removeAllRanges();
>+selection.addRange(range);
>+if (window.internals) {
>+    var width = element.getBoundingClientRect().width;
>+    var selectionWidth = internals.selectionBounds().width;
>+    shouldBe("width", "selectionWidth * 3");
>+}
>+</script>
>+<script src="../../resources/js-test-post.js"></script>
>+</body>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 189448: 349240 | 349246 | 349248 | 349249 | 349251 | 349600 | 349602 | 349630 | 349634 | 349635 | 349645 | 349646 | 349649 | 349657 | 349665 | 349681 | 349692 | 349694 | 349703 | 349705 | 349710 | 349731 | 349732 | 349734 | 349737 | 349739 | 435524 | 435525 | 435528 | 436111 | 436125 | 436127 | 436132 | 436134 | 436141 | 436150 | 436152 | 436216 | 436607