REGRESSION: Text with a zero offset, zero blur shadow vanishes
authormmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 25 Sep 2014 02:32:06 +0000 (02:32 +0000)
committermmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 25 Sep 2014 02:32:06 +0000 (02:32 +0000)
https://bugs.webkit.org/show_bug.cgi?id=136801

Reviewed by Darin Adler.

Source/WebCore:

This patch performs some cleanup regarding TextPainter's shadow logic and handles an
additional case of empty shadows. Previously, there was tight coupling between
applyShadowToGraphicalContext() and paintTextWithShadows(), as they both used a
collection of variables to determine how shadows are to be drawn. This complexity has
been moved into a helper class, ShadowApplier, which performs what
applyShadowToGraphicsContext() used to do, and cleans up correctly in its destructor.
This removes the tight coupling mentioned earlier.

Test: fast/text/empty-shadow.html

* rendering/InlineTextBox.cpp:
(WebCore::InlineTextBox::applyShadowToGraphicsContext): Moved to ShadowApplier.
* rendering/InlineTextBox.h: Deleted applyShadowToGraphicsContext signature.
* rendering/TextPainter.cpp:
(WebCore::ShadowApplier::ShadowApplier): Perform the contents of applyShadowToGraphicsContext()
(WebCore::ShadowApplier::~ShadowApplier): Undo the work done previously
(WebCore::paintTextWithShadows): Create a ShadowApplier to do the relevant shadow work. In addition,
refactor some boolean flags to more meaningful ones with relation to the computation at hand.
(WebCore::isEmptyShadow): Moved to TextPainter.h, named shadowIsCompletelyCoveredByText()
* rendering/TextPainter.h:
(WebCore::ShadowApplier::ShadowApplier): Moved from InlineTextBox::applyShadowToGraphicsContext().
(WebCore::ShadowApplier::extraOffset): Getter.
(WebCore::ShadowApplier::~ShadowApplier): Moved from TextPainter::paintTextWithShadows().
(WebCore::isLastShadowIteration): Helper function.
(WebCore::shadowIsCompletelyCoveredByText): Determines whether or not we should not draw the shadow.
* rendering/svg/SVGInlineTextBox.cpp:
(WebCore::SVGInlineTextBox::paintTextWithShadows): Update to use ShadowApplier.

LayoutTests:

This test should be a comprehensive test of empty shadows. It tests every
combination of one and two shadows being empty, as well as transparent and
opaque text.

After updating fast/text/empty-shadow.html, fast/text/empty-shadow-with-color.html
is no longer necessary.

* fast/text/empty-shadow-expected.html:
* fast/text/empty-shadow-with-color-expected.html: Removed.
* fast/text/empty-shadow-with-color.html: Removed.
* fast/text/empty-shadow.html:

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

LayoutTests/ChangeLog
LayoutTests/fast/text/empty-shadow-expected.html
LayoutTests/fast/text/empty-shadow-with-color-expected.html [deleted file]
LayoutTests/fast/text/empty-shadow-with-color.html [deleted file]
LayoutTests/fast/text/empty-shadow.html
Source/WebCore/ChangeLog
Source/WebCore/rendering/InlineTextBox.cpp
Source/WebCore/rendering/InlineTextBox.h
Source/WebCore/rendering/TextPainter.cpp
Source/WebCore/rendering/TextPainter.h
Source/WebCore/rendering/svg/SVGInlineTextBox.cpp

index 70dc208fb58f94c93820cba3a7a9b740b8c48356..899ece2ce95425057d336439ea2fcfd95e0a7875 100644 (file)
@@ -1,3 +1,22 @@
+2014-09-22  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        REGRESSION: Text with a zero offset, zero blur shadow vanishes
+        https://bugs.webkit.org/show_bug.cgi?id=136801
+
+        Reviewed by Darin Adler.
+
+        This test should be a comprehensive test of empty shadows. It tests every
+        combination of one and two shadows being empty, as well as transparent and
+        opaque text.
+
+        After updating fast/text/empty-shadow.html, fast/text/empty-shadow-with-color.html
+        is no longer necessary.
+
+        * fast/text/empty-shadow-expected.html:
+        * fast/text/empty-shadow-with-color-expected.html: Removed.
+        * fast/text/empty-shadow-with-color.html: Removed.
+        * fast/text/empty-shadow.html:
+
 2014-09-24  Shivakumar JM  <shiva.jm@samsung.com>
 
         Add New Test for overrideMimeType in XMLHttpRequest.
index ce2b96b0bd02f9cfc8dfc69977163c952f6636a8..2b3faea5c40fb3dd2e931781bae008ee492dadda 100644 (file)
@@ -1,3 +1,20 @@
-This tests that text drawn with text-shadows of radius 0 and (0, 0) offset are not drawn.
-This is a better outcome than them being drawn in an ugly way.
-This test is successful if the text below is completely invisible.
+<p>This tests that text drawn with text-shadows of radius 0 and (0, 0) offset are not drawn.</p>
+<p>In the following tests, the actual text is green while the shadows are blue.</p>
+
+<div style="color: green;">
+<div style="text-shadow: 5px 5px blue;">Text with a single shadow.</div>
+<div>Text with one empty (undrawn) shadow.</div>
+<div style="text-shadow: 5px 5px blue, 10px 10px blue;">Text with two shadows</div>
+<div style="text-shadow: 5px 5px blue;">Text with two shadows: first drawn and second empty (undrawn).</div>
+<div style="text-shadow: 10px 10px blue;">Text with two shadows: first empty (undrawn) and second drawn.</div>
+<div>Text with two empty (undrawn) shadows.</div>
+</div>
+
+<div style="color: transparent;">
+<div style="text-shadow: 5px 5px blue;">Transparent text with a single shadow.</div>
+<div style="color: blue;">Transparent text with one empty shadow.</div>
+<div style="text-shadow: 5px 5px blue, 10px 10px blue;">Transparent text with two shadows</div>
+<div style="color: blue; text-shadow: 7px 7px blue;">Transparent text with two shadows: first drawn and second empty.</div>
+<div style="color: blue; text-shadow: 10px 10px blue;">Transparent text with two shadows: first drawn and second empty.</div>
+<div style="color: blue; position: relative;"><div style="position: absolute; left: 0px; top: 0px;">Transparent text with two empty shadows.</div><div style="position: absolute; left: 0px; top: 0px;">Transparent text with two empty shadows.</div></div>
+</div>
diff --git a/LayoutTests/fast/text/empty-shadow-with-color-expected.html b/LayoutTests/fast/text/empty-shadow-with-color-expected.html
deleted file mode 100644 (file)
index 45a4d92..0000000
+++ /dev/null
@@ -1,15 +0,0 @@
-<!DOCTYPE html>
-<html>
-<head>
-<style>
-#test {
-    color: green;
-    font-size: 16px;
-}
-</style>
-</head>
-<body>
-This test makes sure that text drawn with an empty shadow is colored correctly. It passes if the following text is green.
-<div id="test">This should be green.</div>
-</body>
-</html>
diff --git a/LayoutTests/fast/text/empty-shadow-with-color.html b/LayoutTests/fast/text/empty-shadow-with-color.html
deleted file mode 100644 (file)
index 3fdf95c..0000000
+++ /dev/null
@@ -1,17 +0,0 @@
-<!DOCTYPE html>
-<html>
-<head>
-<style>
-#test {
-    color: green;
-    font-size: 16px;
-    text-shadow: red 20px 20px, red 0px 0px;
-    overflow: hidden;
-}
-</style>
-</head>
-<body>
-This test makes sure that text drawn with an empty shadow is colored correctly. It passes if the following text is green.
-<div id="test">This should be green.</div>
-</body>
-</html>
index 427cdd8c63c9a182a5c0df1ad28d31d44b24a3cf..2b3e77834caed22999ed808e3550466efa0b8491 100644 (file)
@@ -1,4 +1,20 @@
-This tests that text drawn with text-shadows of radius 0 and (0, 0) offset are not drawn.
-This is a better outcome than them being drawn in an ugly way.
-This test is successful if the text below is completely invisible.
-<div style="color:#fff;text-shadow:0 0 #000">This is some text</div>
+<p>This tests that text drawn with text-shadows of radius 0 and (0, 0) offset are not drawn.</p>
+<p>In the following tests, the actual text is green while the shadows are blue.</p>
+
+<div style="color: green;">
+<div style="text-shadow: 5px 5px blue;">Text with a single shadow.</div>
+<div style="text-shadow: 0px 0px blue;">Text with one empty (undrawn) shadow.</div>
+<div style="text-shadow: 5px 5px blue, 10px 10px blue;">Text with two shadows</div>
+<div style="text-shadow: 5px 5px blue, 0px 0px blue;">Text with two shadows: first drawn and second empty (undrawn).</div>
+<div style="text-shadow: 0px 0px blue, 10px 10px blue;">Text with two shadows: first empty (undrawn) and second drawn.</div>
+<div style="text-shadow: 0px 0px blue, 0px 0px blue;">Text with two empty (undrawn) shadows.</div>
+</div>
+
+<div style="color: transparent;">
+<div style="text-shadow: 5px 5px blue;">Transparent text with a single shadow.</div>
+<div style="text-shadow: 0px 0px blue;">Transparent text with one empty shadow.</div>
+<div style="text-shadow: 5px 5px blue, 10px 10px blue;">Transparent text with two shadows</div>
+<div style="text-shadow: 7px 7px blue, 0px 0px blue;">Transparent text with two shadows: first drawn and second empty.</div>
+<div style="text-shadow: 0px 0px blue, 10px 10px blue;">Transparent text with two shadows: first drawn and second empty.</div>
+<div style="text-shadow: 0px 0px blue, 0px 0px blue;">Transparent text with two empty shadows.</div>
+</div>
index 376b6130e642ca9366f7808612d5a3db14369fe5..19e3a914a9f1e050569ce58bf9cb8dd5f1268cac 100644 (file)
@@ -1,3 +1,38 @@
+2014-09-22  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        REGRESSION: Text with a zero offset, zero blur shadow vanishes
+        https://bugs.webkit.org/show_bug.cgi?id=136801
+
+        Reviewed by Darin Adler.
+
+        This patch performs some cleanup regarding TextPainter's shadow logic and handles an
+        additional case of empty shadows. Previously, there was tight coupling between
+        applyShadowToGraphicalContext() and paintTextWithShadows(), as they both used a
+        collection of variables to determine how shadows are to be drawn. This complexity has
+        been moved into a helper class, ShadowApplier, which performs what
+        applyShadowToGraphicsContext() used to do, and cleans up correctly in its destructor.
+        This removes the tight coupling mentioned earlier.
+
+        Test: fast/text/empty-shadow.html
+
+        * rendering/InlineTextBox.cpp:
+        (WebCore::InlineTextBox::applyShadowToGraphicsContext): Moved to ShadowApplier.
+        * rendering/InlineTextBox.h: Deleted applyShadowToGraphicsContext signature.
+        * rendering/TextPainter.cpp:
+        (WebCore::ShadowApplier::ShadowApplier): Perform the contents of applyShadowToGraphicsContext()
+        (WebCore::ShadowApplier::~ShadowApplier): Undo the work done previously
+        (WebCore::paintTextWithShadows): Create a ShadowApplier to do the relevant shadow work. In addition,
+        refactor some boolean flags to more meaningful ones with relation to the computation at hand.
+        (WebCore::isEmptyShadow): Moved to TextPainter.h, named shadowIsCompletelyCoveredByText()
+        * rendering/TextPainter.h:
+        (WebCore::ShadowApplier::ShadowApplier): Moved from InlineTextBox::applyShadowToGraphicsContext().
+        (WebCore::ShadowApplier::extraOffset): Getter.
+        (WebCore::ShadowApplier::~ShadowApplier): Moved from TextPainter::paintTextWithShadows().
+        (WebCore::isLastShadowIteration): Helper function.
+        (WebCore::shadowIsCompletelyCoveredByText): Determines whether or not we should not draw the shadow.
+        * rendering/svg/SVGInlineTextBox.cpp:
+        (WebCore::SVGInlineTextBox::paintTextWithShadows): Update to use ShadowApplier.
+
 2014-09-24  Brian J. Burg  <burg@cs.washington.edu>
 
         Web Inspector: subtract elapsed time while debugger is paused from profile nodes
index 2600caa7e9df790b6438cb52694adab8c2ca5ad5..b2da58bb55cf90a61eeb8804d9c20aa1dca49ff6 100644 (file)
@@ -424,34 +424,6 @@ bool InlineTextBox::nodeAtPoint(const HitTestRequest& request, HitTestResult& re
     return false;
 }
 
-FloatSize InlineTextBox::applyShadowToGraphicsContext(GraphicsContext& context, const ShadowData* shadow, const FloatRect& textRect, bool stroked, bool opaque, bool horizontal, bool& didSaveContext)
-{
-    if (!shadow)
-        return FloatSize();
-
-    FloatSize extraOffset;
-    int shadowX = horizontal ? shadow->x() : shadow->y();
-    int shadowY = horizontal ? shadow->y() : -shadow->x();
-    FloatSize shadowOffset(shadowX, shadowY);
-    int shadowRadius = shadow->radius();
-    const Color& shadowColor = shadow->color();
-
-    if (shadow->next() || stroked || !opaque) {
-        FloatRect shadowRect(textRect);
-        shadowRect.inflate(shadow->paintingExtent());
-        shadowRect.move(shadowOffset);
-        context.save();
-        context.clip(shadowRect);
-
-        extraOffset = FloatSize(0, 2 * textRect.height() + std::max(0.0f, shadowOffset.height()) + shadowRadius);
-        shadowOffset -= extraOffset;
-        didSaveContext = true;
-    }
-
-    context.setShadow(shadowOffset, shadowRadius, shadowColor, context.fillColorSpace());
-    return extraOffset;
-}
-
 static inline bool emphasisPositionHasNeitherLeftNorRight(TextEmphasisPosition emphasisPosition)
 {
     return !(emphasisPosition & TextEmphasisPositionLeft) && !(emphasisPosition & TextEmphasisPositionRight);
index e89a2cd9c7f98899ab473efb7427e38464a79c9b..6f818c45b17bbfa877227f237a467193571ab443 100644 (file)
@@ -154,9 +154,6 @@ public:
     virtual int offsetForPosition(float x, bool includePartialGlyphs = true) const;
     virtual float positionForOffset(int offset) const;
 
-    // Needs to be public, so the static paintTextWithShadows() function can use it.
-    static FloatSize applyShadowToGraphicsContext(GraphicsContext&, const ShadowData*, const FloatRect& textRect, bool stroked, bool opaque, bool horizontal, bool& didSaveContext);
-
 protected:
     void paintCompositionBackground(GraphicsContext*, const FloatPoint& boxOrigin, const RenderStyle&, const Font&, int startPos, int endPos);
     void paintDocumentMarkers(GraphicsContext*, const FloatPoint& boxOrigin, const RenderStyle&, const Font&, bool background);
index fa1d79a00f9f192b8c8bf1a24774df44e8f7f5c4..4807b04055fe61f818fd3d71412cc7e55fc3a472 100644 (file)
@@ -65,11 +65,52 @@ static void drawTextOrEmphasisMarks(GraphicsContext& context, const Font& font,
         context.drawEmphasisMarks(font, textRun, emphasisMark, point + IntSize(0, emphasisMarkOffset), from, to);
 }
 
-static bool isEmptyShadow(const ShadowData* shadow)
+ShadowApplier::ShadowApplier(GraphicsContext& context, const ShadowData* shadow, const FloatRect& textRect, bool lastShadowIterationShouldDrawText, bool opaque, FontOrientation orientation)
+    : m_context(context)
+    , m_shadow(shadow)
+    , m_onlyDrawsShadow(!isLastShadowIteration() || !lastShadowIterationShouldDrawText)
+    , m_avoidDrawingShadow(shadowIsCompletelyCoveredByText(opaque))
+    , m_nothingToDraw(shadow && m_avoidDrawingShadow && m_onlyDrawsShadow)
+    , m_didSaveContext(false)
 {
-    if (!shadow)
-        return false;
-    return shadow->location() == IntPoint() && !shadow->radius();
+    if (!shadow || m_nothingToDraw) {
+        m_shadow = nullptr;
+        return;
+    }
+
+    int shadowX = orientation == Horizontal ? shadow->x() : shadow->y();
+    int shadowY = orientation == Horizontal ? shadow->y() : -shadow->x();
+    FloatSize shadowOffset(shadowX, shadowY);
+    int shadowRadius = shadow->radius();
+    const Color& shadowColor = shadow->color();
+
+    // When drawing shadows, we usually clip the context to the area the shadow will reside, and then
+    // draw the text itself outside the clipped area (so only the shadow shows up). However, we can
+    // often draw the *last* shadow and the text itself in a single call.
+    if (m_onlyDrawsShadow) {
+        FloatRect shadowRect(textRect);
+        shadowRect.inflate(shadow->paintingExtent());
+        shadowRect.move(shadowOffset);
+        context.save();
+        context.clip(shadowRect);
+
+        m_didSaveContext = true;
+        m_extraOffset = FloatSize(0, 2 * textRect.height() + std::max(0.0f, shadowOffset.height()) + shadowRadius);
+        shadowOffset -= m_extraOffset;
+    }
+
+    if (!m_avoidDrawingShadow)
+        context.setShadow(shadowOffset, shadowRadius, shadowColor, context.fillColorSpace());
+}
+
+ShadowApplier::~ShadowApplier()
+{
+    if (!m_shadow)
+        return;
+    if (m_onlyDrawsShadow)
+        m_context.restore();
+    else if (!m_avoidDrawingShadow)
+        m_context.clearShadow();
 }
 
 static void paintTextWithShadows(GraphicsContext& context, const Font& font, const TextRun& textRun, const AtomicString& emphasisMark,
@@ -79,20 +120,19 @@ static void paintTextWithShadows(GraphicsContext& context, const Font& font, con
     Color fillColor = context.fillColor();
     ColorSpace fillColorSpace = context.fillColorSpace();
     bool opaque = !fillColor.hasAlpha();
+    bool lastShadowIterationShouldDrawText = !stroked && opaque;
     if (!opaque)
         context.setFillColor(Color::black, fillColorSpace);
 
     do {
-        if (isEmptyShadow(shadow)) {
+        ShadowApplier shadowApplier(context, shadow, boxRect, lastShadowIterationShouldDrawText, opaque, horizontal ? Horizontal : Vertical);
+        if (shadowApplier.nothingToDraw()) {
             shadow = shadow->next();
             continue;
         }
 
-        IntSize extraOffset;
-        bool didSaveContext = false;
-        if (shadow)
-            extraOffset = roundedIntSize(InlineTextBox::applyShadowToGraphicsContext(context, shadow, boxRect, stroked, opaque, horizontal, didSaveContext));
-        else if (!opaque)
+        IntSize extraOffset = roundedIntSize(shadowApplier.extraOffset());
+        if (!shadow && !opaque)
             context.setFillColor(fillColor, fillColorSpace);
 
         if (startOffset <= endOffset)
@@ -107,13 +147,8 @@ static void paintTextWithShadows(GraphicsContext& context, const Font& font, con
         if (!shadow)
             break;
 
-        if (didSaveContext)
-            context.restore();
-        else
-            context.clearShadow();
-
         shadow = shadow->next();
-    } while (shadow || stroked || !opaque);
+    } while (shadow || !lastShadowIterationShouldDrawText);
 }
 
 void TextPainter::paintText()
index 96f83aa2b31341966d36f5934b8a576c3d0407d7..1ca5f10f5b80898f1918803e61a2fabdde2e2209 100644 (file)
@@ -25,6 +25,7 @@
 
 #include "AffineTransform.h"
 #include "DashArray.h"
+#include "FontOrientation.h"
 #include "RenderText.h"
 
 namespace WebCore {
@@ -73,6 +74,34 @@ private:
     bool m_textBoxIsHorizontal;
 };
 
+class ShadowApplier {
+public:
+    ShadowApplier(GraphicsContext&, const ShadowData*, const FloatRect& textRect, bool lastShadowIterationShouldDrawText = true, bool opaque = false, FontOrientation = Horizontal);
+    FloatSize extraOffset() const { return m_extraOffset; }
+    bool nothingToDraw() const { return m_nothingToDraw; }
+    bool didSaveContext() const { return m_didSaveContext; }
+    ~ShadowApplier();
+
+private:
+    bool isLastShadowIteration()
+    {
+        return m_shadow && !m_shadow->next();
+    }
+
+    bool shadowIsCompletelyCoveredByText(bool textIsOpaque)
+    {
+        return textIsOpaque && shadow && m_shadow->location() == IntPoint() && !m_shadow->radius();
+    }
+
+    FloatSize m_extraOffset;
+    GraphicsContext& m_context;
+    const ShadowData* m_shadow;
+    bool m_onlyDrawsShadow : 1;
+    bool m_avoidDrawingShadow : 1;
+    bool m_nothingToDraw : 1;
+    bool m_didSaveContext : 1;
+};
+
 } // namespace WebCore
 
 #endif // TextPainter_h
index cdecda471a9092b9888dd2465349df693c30605f..4116a4c8bcec6907a6f18833e24653ccd2b0b3e4 100644 (file)
@@ -37,6 +37,7 @@
 #include "SVGResourcesCache.h"
 #include "SVGRootInlineBox.h"
 #include "SVGTextRunRenderingContext.h"
+#include "TextPainter.h"
 
 namespace WebCore {
 
@@ -594,23 +595,17 @@ void SVGInlineTextBox::paintTextWithShadows(GraphicsContext* context, RenderStyl
         if (!prepareGraphicsContextForTextPainting(context, scalingFactor, textRun, style))
             break;
 
-        FloatSize extraOffset;
-        bool didSaveContext = false;
-        if (shadow)
-            extraOffset = applyShadowToGraphicsContext(*context, shadow, shadowRect, false /* stroked */, true /* opaque */, true /* horizontal */, didSaveContext);
+        {
+            ShadowApplier shadowApplier(*context, shadow, shadowRect);
 
-        context->save();
-        context->scale(FloatSize(1 / scalingFactor, 1 / scalingFactor));
-
-        scaledFont.drawText(context, textRun, textOrigin + extraOffset, startPosition, endPosition);
+            if (!shadowApplier.didSaveContext())
+                context->save();
+            context->scale(FloatSize(1 / scalingFactor, 1 / scalingFactor));
 
-        context->restore();
+            scaledFont.drawText(context, textRun, textOrigin + shadowApplier.extraOffset(), startPosition, endPosition);
 
-        if (shadow) {
-            if (didSaveContext)
+            if (!shadowApplier.didSaveContext())
                 context->restore();
-            else
-                context->clearShadow();
         }
 
         restoreGraphicsContextAfterTextPainting(context, textRun);