Summary: | [Chromium] Layout Test svg/custom/transform-with-shadow-and-gradient.svg is failing | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Robinson <jamesr> | ||||||
Component: | Tools / Tests | Assignee: | Florin Malita <fmalita> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | fmalita, junov, krit, reed, schenney, senorblanco, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
James Robinson
2012-01-18 11:26:34 PST
Update: yes, this fails on all platforms that use skia (not just linux). 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. 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/ 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. 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. Created attachment 190281 [details]
Inconsistent shadow.
(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. (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? Created attachment 190516 [details]
Patch
Comment on attachment 190516 [details]
Patch
lgtm. R=me.
Wait for style and linux ews before committing. 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.
Comment on attachment 190516 [details] Patch Clearing flags on attachment: 190516 Committed r144195: <http://trac.webkit.org/changeset/144195> All reviewed patches have been landed. Closing bug. |