[Chromium] Layout Test svg/custom/transform-with-shadow-and-gradient.svg is failing

• Component: Tools / Tests
• Status: RESOLVED FIXED

[James Robinson]

The following layout test is failing on chromium linux:

svg/custom/transform-with-shadow-and-gradient.svg

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=svg%2Fcustom%2Ftransform-with-shadow-and-gradient.svg

The stroked circle and rectangle have black centers when they should be white.  So far this has only failed on linux, but it may just have not cycled on the other bots yet.

[James Robinson]

Update: yes, this fails on all platforms that use skia (not just linux).

[Stephen Chenney]

This may be an SVG bug, or maybe a Skia bug.

The problematic shapes are these:

    <circle transform="scale(4,4)" style="-webkit-svg-shadow: black 1px 1px 1px;" stroke="url(#gradient)" fill="transparent" cx="25" cy="100" r="20"></circle>
    <rect transform="scale(32,32)" style="-webkit-svg-shadow: black 1px 1px 1px;" stroke="url(#gradient)" fill="transparent" x="10" y="10" width="5" height="5"></rect>

Note the fill is transparent. When constructing the shadow, we are clearly not considering the object to be unfilled. We are instead presumably considering it filled and drawing the shadow for a filled shape.

[Stephen White]

I haven't looked into this in detail, but if SVG is creating a bitmap from the primitive, then drawing a shadow from the bitmap's alpha, that's a known issue in Skia.  I have a fix in the works, but it hasn't landed yet:  http://codereview.appspot.com/5981063/

[Stephen White]

There was also another issue with shadows on hairline paths (width <= 1px), which junov@ recently fix:  http://code.google.com/p/skia/source/detail?r=3592.  I don't know if WebKit's chromium DEPS have updated past that Skia revision yet.

[Stephen Chenney]

https://code.google.com/p/chromium/issues/detail?id=178401

[Florin Malita]

Doesn't look like we're rasterizing the shapes - we're just passing drawXXX calls to Skia.

I can confirm that this is caused by fill='transparent' (when using fill='none' the shadows are drawn correctly). I have a couple of thoughts on this, each may be a separate bug & fix this issue independently:

* at a high level, is 'transparent' not equivalent to 'none' for SVG fill/stroke? If so, we need to treat the same.

* shadows appear to observe the fill/stroke alpha when specified via opacity attributes, but not when specified as part of the color (rgba(x,x,x,a)). This seems inconsistent.

[Florin Malita]

Created attachment 190281 [details]
Inconsistent shadow.

[Florin Malita]

(In reply to comment #6)
> Doesn't look like we're rasterizing the shapes - we're just passing drawXXX calls to Skia.

Never mind, we are drawing offscreen due to shadows being implemented via transparency layers.

[Florin Malita]

(In reply to comment #6)
> * shadows appear to observe the fill/stroke alpha when specified via opacity attributes, but not when specified as part of the color (rgba(x,x,x,a)). This seems inconsistent.

The explanation for this discrepancy is that an opacity attribute forces yet another transparency layer.

Mike helped me trace the shadow alpha behavior, and it turns out to be caused by the transfer mode we use in  GraphicsContext::setPlatformShadow(). According to comments there, we ignore the alpha for CSS (and currently SVG), and only use it for Canvas:

        // CSS wants us to ignore the original's alpha, but Canvas wants us to
        // modulate with it. Using shadowsIgnoreTransforms to tell us that we're
        // in a Canvas, we change the colormode to kDst_Mode, so we don't overwrite
        // it with our layer's (default opaque-black) color.
        colorMode = SkXfermode::kDst_Mode;

We've verified that forcing the transfer mode to kDst_Mode (instead of the default kSrc_Mode) fixes the problem and aligns the results with Safari. Surprisingly, this did not appear to break CSS box-shadow. After some digging around, it turns out that RenderBoxModelObject::paintBoxShadow() already forces the paint color to opaque black (and actually draws the shadow on its own, not as part of the background fill) so this precaution appears to be unnecessary:

SVG: shadows are applied as part of the fill/stroke drawing and observe the paint alpha (kDst_Mode is what we want)
CSS: shadows are applied in a separate pass, using an opaque paint per spec (kDst_Mode still OK since the color is opaque)

It seems that we can safely always use kDst_Mode here, what do you think?

[Florin Malita]

Created attachment 190516 [details]
Patch

[Stephen Chenney]

Comment on attachment 190516 [details]
Patch

lgtm. R=me.

[Stephen Chenney]

Wait for style and linux ews before committing.

[WebKit Review Bot]

Attachment 190516 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/chromium-linux/svg/css/text-shadow-multiple-expected.png', u'LayoutTests/platform/chromium-linux/svg/custom/transform-with-shadow-and-gradient-expected.png', u'LayoutTests/platform/chromium/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp']" exit_code: 1
LayoutTests/platform/chromium-linux/svg/css/text-shadow-multiple-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 190516 [details]
Patch

Clearing flags on attachment: 190516

Committed r144195: <http://trac.webkit.org/changeset/144195>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.