Bug 42387 – SVG - stroke-width:0 bug with stroke other than "none"

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Fady Samuel]

The renderer is apparently getting confused by the combination of
"stroke-width:0" with a "stroke" other than "none". Removing the stroke
cures the problem. 

The issue is skia attempts to generate strokes even when stroke-width is zero. The patch attached fixes the issue by detecting this case and avoiding stroking.

[Fady Samuel]

Created attachment 61683 [details]
stroke-width:0 fix

[Darin Adler]

Comment on attachment 61683 [details]
stroke-width:0 fix

There's an extra change log entry in there.

>      } else
> -        platformContext()->setupPaintForStroking(&paint, 0, 0);
> +        if (!platformContext()->setupPaintForStroking(&paint, 0, 0))
> +            return;

Need to add braces here because this is now a multi-line else body.

Otherwise looks fine. r=me

[Fady Samuel]

Created attachment 61725 [details]
stroke-width:0 fix

Fixed minor style issue. Tested for broken layout tests.nothing new appears to break.

[Fady Samuel]

Created attachment 61726 [details]
stroke-width:0 fix

Fixed a minor style issue. Ran layout tests to see if anything broke. No new tests are broken by this patch.

[WebKit Commit Bot]

Comment on attachment 61726 [details]
stroke-width:0 fix

Rejecting patch 61726 from commit-queue.

Unexpected failure when processing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 61726, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=61726&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=42387&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 61726 from bug 42387.
ERROR: /Users/eseidel/Projects/CommitQueue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Fady Samuel]

Created attachment 61804 [details]
stroke-width:0 fix

Accidentally deleted REVIEWED BY line in the layout test changelog.

[Stephen White]

It's not clear to me why this page has only GTK tests results (since GTK doesn't even use Skia).  Shouldn't we have results for mac and chromium-win, at a minimum?

[Eric Seidel (no email)]

Comment on attachment 61804 [details]
stroke-width:0 fix

Your ChangeLog is wrong again.

[Fady Samuel]

(In reply to comment #7)
> It's not clear to me why this page has only GTK tests results (since GTK doesn't even use Skia).  Shouldn't we have results for mac and chromium-win, at a minimum?

Ohh I'm sorry. I ran the WebKit run-webkit-kits, instead of the chromium run_webkit_tests to produce the baseline, for this chromium-specific webkit bug. Thanks for pointing that out.

[Fady Samuel]

Created attachment 61837 [details]
stroke-width:0 fix

Added more expectations according to conversation with Stephen White.

[Stephen White]

LGTM, thanks.  (I'm not a reviewer, but perhaps Darin or Eric can take another look).

[Eric Seidel (no email)]

Comment on attachment 61837 [details]
stroke-width:0 fix

There is no need to show scrollbars.  Just make the <svg> smaller.  Then you shouldn't need differences between cr-linux and mac.

[Fady Samuel]

Created attachment 61848 [details]
stroke-width:0 fix

Got rid of scrollbars in layout tests

[David Levin]

Comment on attachment 61848 [details]
stroke-width:0 fix

No need for chromium-mac results.


> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 0fc0719..dbd9b17 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,22 @@
> [phone]  Fady Samuel  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +        
> +        Avoids adding stroke when stroke-width is zero.

Move to function level in the changelog (and put "Ditto." after the first function).

(because function level comments are preferred in WK ChangeLogs.

> +
> +        SVG - stroke-width:0 bug with stroke other than "none"
> +        https://bugs.webkit.org/show_bug.cgi?id=42387
> +
> +        Test: svg/stroke/path-zero-strokewidth-test.svg
> +
> +        * platform/graphics/skia/GraphicsContextSkia.cpp:
> +        (WebCore::GraphicsContext::drawConvexPolygon):

Right here (is what I mean by function level).

> +        (WebCore::GraphicsContext::drawEllipse):
> +        (WebCore::GraphicsContext::drawLine):
> +        (WebCore::GraphicsContext::strokeArc):
> +        (WebCore::GraphicsContext::strokePath):
> +        (WebCore::GraphicsContext::strokeRect):
> +

[David Levin]

Comment on attachment 61848 [details]
stroke-width:0 fix

OK. The images are different.

In the future please consider doing the ChangeLog as described.

[WebKit Commit Bot]

Comment on attachment 61848 [details]
stroke-width:0 fix

Clearing flags on attachment: 61848

Committed r63593: <http://trac.webkit.org/changeset/63593>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

(In reply to comment #17)
> All reviewed patches have been landed.  Closing bug.

This broke the following tests on windows:
  media/video-volume-slider.html = IMAGE
  svg/W3C-SVG-1.1/animate-elem-40-t.svg = IMAGE
  svg/stroke/path-zero-strokewidth-test.svg = IMAGE

and the following on linux:
  media/video-volume-slider.html = IMAGE
  svg/W3C-SVG-1.1/animate-elem-40-t.svg = IMAGE

and probably the same on Mac.

I'm going to revert for now since it's not obvious to me what fix is.

[Tony Chang]

Reverted in http://trac.webkit.org/changeset/63598

[Eric Seidel (no email)]

Comment on attachment 61683 [details]
stroke-width:0 fix

Cleared Darin Adler's review+ from obsolete attachment 61683 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 61726 [details]
stroke-width:0 fix

Cleared Darin Adler's review+ from obsolete attachment 61726 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Fady Samuel]

Created attachment 62465 [details]
stroke-width:0 fix

So the initial patch broke some already flaky layout tests. It turns out fixing this bug revealed a couple of others. First, the video controls in the video element were not rendered correctly as the stroke-width was not specified. Secondly, stroke-width defaults to 0, this patch defaults it to 1 now as the spec describes. 

This bug corresponds to Issue 15461 on chromium.org:

http://code.google.com/p/chromium/issues/detail?id=15461

I may be missing some more layout tests. There may need to be more rebaselining in the gtk platform and in the windows platforms. My chromium-mac/mac platforms were baselined on Snow Leopard, this may or may not be what is expected.

[Dirk Schulze]

Comment on attachment 62465 [details]
stroke-width:0 fix

(In reply to comment #22)
> Created an attachment (id=62465) [details]
> stroke-width:0 fix
> 
> So the initial patch broke some already flaky layout tests. It turns out fixing this bug revealed a couple of others. First, the video controls in the video element were not rendered correctly as the stroke-width was not specified. Secondly, stroke-width defaults to 0, this patch defaults it to 1 now as the spec describes. 
> 
> This bug corresponds to Issue 15461 on chromium.org:
> 
> http://code.google.com/p/chromium/issues/detail?id=15461
> 
> I may be missing some more layout tests. There may need to be more rebaselining in the gtk platform and in the windows platforms. My chromium-mac/mac platforms were baselined on Snow Leopard, this may or may not be what is expected.

Great patch! Just some notes. IIRC the default for stroke-width is not 1px but 1. So no need for SVG_RS_DEFINE_ATTRIBUTE_DATAREF_WITH_INITIAL_REFCOUNTED2.

Also, this patch fixes more than one issue. The SVG fix and the Skia changes should be seperated of each other.
Is it possible to check for !stroke-width in RenderPath's fillAndStrokePath? This could avoid building a gradient or pattern if we don't draw the stroke anyway.

[Fady Samuel]

> Great patch! Just some notes. IIRC the default for stroke-width is not 1px but 1. So no need for SVG_RS_DEFINE_ATTRIBUTE_DATAREF_WITH_INITIAL_REFCOUNTED2.
> 
I'm confused by this comment. I still need a default CSSPrimitiveValue, do I not? Previously the default was NULL, which translated into stroke-width:0 throughout the code. What UnitType is appropriate here?

[Fady Samuel]

I'm not sure how authoritative this blog is, it appears to be written by a Mozilla engineer, Jonathan Watt. He suggests, 'pixels' is the correct unit:

"Happily, in SVG, px units are defined to be equivalent to the units established by the current coordinate system (user units). In other words, wherever you would otherwise have omitted the unit from a length assigned to a property, use the px unit instead. For example, instead of writing:

<text stroke-width="2" style="font-size:20;" ...>

write:

<text stroke-width="2px" style="font-size:20px;" ...>"

http://jwatt.org/svg/authoring/

[Nikolas Zimmermann]

(In reply to comment #24)
> > Great patch! Just some notes. IIRC the default for stroke-width is not 1px but 1. So no need for SVG_RS_DEFINE_ATTRIBUTE_DATAREF_WITH_INITIAL_REFCOUNTED2.

Quote, from SVG spec: "One px unit is defined to be equal to one user unit. Thus, a length of "5px" is the same as a length of "5"."

> > 
> I'm confused by this comment. I still need a default CSSPrimitiveValue, do I not? Previously the default was NULL, which translated into stroke-width:0 throughout the code. What UnitType is appropriate here?

As stroke-width is a CSSValue and as you need a default value of "1", using "1px" is fine. Though the new macro is wrong. You're not allowed to create a static local CSSValue! That's a bad idea, the render style is shared, that would lead to problems.

The real fix is to move away from storing CSSValue in SVGRenderStyle. As you may have noticed, RenderStyle doesn't do that for HTML/CSS properties. It's a special old SVG solution.

We really want to store floats for things like stroke-width / stroke-dashoffset, the problem is how to handle percentage values. If you grep for cssPrimitiveToLength, you'll see constructs like:
float strokeWidth = SVGRenderStyle::cssPrimitiveToLength(this, svgStyle->strokeWidth(), 1.0f);
in RenderPath.cpp as example.

cssPrimitiveToLength contains special code to handle percentage values, as they have to be evaluated relative to the viewport (as SVG defines it).

I need to think more about this, but it's a good opportunity to cleanup SVGRenderStyle/Defs which is a crude hack at the moment, so much macro usage.

Need to leave for today, maybe we can find some time to chat on IRC.
I'd encourage you to cleanup SVGRenderStyle/Defs first, before doing any further work on it. A good first step would be to remove the macro usage alltogether, there's even a FIXME for that.

Do you have some time to work on this?

[Fady Samuel]

(In reply to comment #26)
> (In reply to comment #24)
> > > Great patch! Just some notes. IIRC the default for stroke-width is not 1px but 1. So no need for SVG_RS_DEFINE_ATTRIBUTE_DATAREF_WITH_INITIAL_REFCOUNTED2.
> 
> Quote, from SVG spec: "One px unit is defined to be equal to one user unit. Thus, a length of "5px" is the same as a length of "5"."
> 
> > > 
> > I'm confused by this comment. I still need a default CSSPrimitiveValue, do I not? Previously the default was NULL, which translated into stroke-width:0 throughout the code. What UnitType is appropriate here?
> 
> As stroke-width is a CSSValue and as you need a default value of "1", using "1px" is fine. Though the new macro is wrong. You're not allowed to create a static local CSSValue! That's a bad idea, the render style is shared, that would lead to problems.
> 
> The real fix is to move away from storing CSSValue in SVGRenderStyle. As you may have noticed, RenderStyle doesn't do that for HTML/CSS properties. It's a special old SVG solution.
> 
> We really want to store floats for things like stroke-width / stroke-dashoffset, the problem is how to handle percentage values. If you grep for cssPrimitiveToLength, you'll see constructs like:
> float strokeWidth = SVGRenderStyle::cssPrimitiveToLength(this, svgStyle->strokeWidth(), 1.0f);
> in RenderPath.cpp as example.
> 
> cssPrimitiveToLength contains special code to handle percentage values, as they have to be evaluated relative to the viewport (as SVG defines it).
> 
> I need to think more about this, but it's a good opportunity to cleanup SVGRenderStyle/Defs which is a crude hack at the moment, so much macro usage.
> 
> Need to leave for today, maybe we can find some time to chat on IRC.
> I'd encourage you to cleanup SVGRenderStyle/Defs first, before doing any further work on it. A good first step would be to remove the macro usage alltogether, there's even a FIXME for that.
> 
> Do you have some time to work on this?

So this sounds like a lot of work, but I can continue to work on it. So a simple solution for removing macros would be to simply expand all the macros, but I doubt that's what you had in mind. Will discuss this further on IRC.

[Nikolas Zimmermann]

As I mentioned on IRC, I have no clue why there is a bug at all.
The CSSValues stored in SVGRenderStyle are not used directly, but always through: the cssPrimitiveToLength() method in SVGRenderStyle, which is given a default value:
float strokeWidth = SVGRenderStyle::cssPrimitiveToLength(this, svgStyle->strokeWidth(), 1.0f);

Of course SVGRenderStyle should move away from storing CSSValues, and behave more like RenderStyle (no macro usage, no CSSValues storage, but floats..) but still I don't see the bug yet.

[Fady Samuel]

(In reply to comment #28)
> As I mentioned on IRC, I have no clue why there is a bug at all.
> The CSSValues stored in SVGRenderStyle are not used directly, but always through: the cssPrimitiveToLength() method in SVGRenderStyle, which is given a default value:
> float strokeWidth = SVGRenderStyle::cssPrimitiveToLength(this, svgStyle->strokeWidth(), 1.0f);
> 
> Of course SVGRenderStyle should move away from storing CSSValues, and behave more like RenderStyle (no macro usage, no CSSValues storage, but floats..) but still I don't see the bug yet.

Found the bug! third_party/WebKit/WebCore/css/SVGCSSStyleSelector.cpp doesn't use cssPrimitiveToLength at all. It also uses nasty macros :S Will have a patch out ready for review either today or after the weekend...

[Nikolas Zimmermann]

(In reply to comment #29)
> (In reply to comment #28)
> > As I mentioned on IRC, I have no clue why there is a bug at all.
> > The CSSValues stored in SVGRenderStyle are not used directly, but always through: the cssPrimitiveToLength() method in SVGRenderStyle, which is given a default value:
> > float strokeWidth = SVGRenderStyle::cssPrimitiveToLength(this, svgStyle->strokeWidth(), 1.0f);
> > 
> > Of course SVGRenderStyle should move away from storing CSSValues, and behave more like RenderStyle (no macro usage, no CSSValues storage, but floats..) but still I don't see the bug yet.
> 
> Found the bug! third_party/WebKit/WebCore/css/SVGCSSStyleSelector.cpp doesn't use cssPrimitiveToLength at all. It also uses nasty macros :S Will have a patch out ready for review either today or after the weekend...

I don't think it can use this, as the Element* context is missing, which is needed to resolve percentual values....

Off to bed now, we can chat later.

[Fady Samuel]

(In reply to comment #30)
> (In reply to comment #29)
> > (In reply to comment #28)
> > > As I mentioned on IRC, I have no clue why there is a bug at all.
> > > The CSSValues stored in SVGRenderStyle are not used directly, but always through: the cssPrimitiveToLength() method in SVGRenderStyle, which is given a default value:
> > > float strokeWidth = SVGRenderStyle::cssPrimitiveToLength(this, svgStyle->strokeWidth(), 1.0f);
> > > 
> > > Of course SVGRenderStyle should move away from storing CSSValues, and behave more like RenderStyle (no macro usage, no CSSValues storage, but floats..) but still I don't see the bug yet.
> > 
> > Found the bug! third_party/WebKit/WebCore/css/SVGCSSStyleSelector.cpp doesn't use cssPrimitiveToLength at all. It also uses nasty macros :S Will have a patch out ready for review either today or after the weekend...
> 
> I don't think it can use this, as the Element* context is missing, which is needed to resolve percentual values....
> 
> Off to bed now, we can chat later.

I think I didn't communicate my understanding of the issue well. I have attached an svg that exhibits the issues. A box has the animation assigned to it that transitions from the default stroke-width to a stroke-width of 4 back to default. Currently, the default stroke-width has no value and so SMILTimeContainer::baseValueFor sets the attribute baseValue as being "" (empty string) which is later interpreted to mean 0 in SVGAnimateElement::resetToBaseValue I believe. 

I could mess around with all the SVG animation stuff or I could simply change initialStrokeWidth. Updating initialStrokeWidth seems like the simplest and cleanest solution to me.

[Fady Samuel]

Created attachment 63482 [details]
Animated Box bug

[Fady Samuel]

Created attachment 63484 [details]
stroke-width:0 fix

Removed Skia changes.

[Fady Samuel]

Created attachment 63518 [details]
stroke-width:0 fix

Fixed some broken expectations

[Nikolas Zimmermann]

Created attachment 63703 [details]
Revised fix

Came up with a better approach to fix the problem: Stop storing CSSValue/CSSValueList objects in SVGRenderStyle.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/rendering/style/SVGRenderStyleDefs.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 26 files


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

[Nikolas Zimmermann]

Created attachment 63706 [details]
Revised fix - v2

Still had a bug with stroke-dasharrays initial value, should be "none" (when evaluating through getComputedStyle).
Note that this fixes the "Animated box bug" example, as well as svg/W3C-SVG-1.1/animate-elem-40-t.svg

[Nikolas Zimmermann]

Mac EWS gives no output, I guess it's a warning that leads to a build failure.
Most likely the "unsigned int length = dashes->length()", as length() is a size_t, changed locally.

[Dirk Schulze]

Comment on attachment 63706 [details]
Revised fix - v2

r=me, please take a look at the buildbots on landing.

[Nikolas Zimmermann]

Landed in r64830. watching bots to assure mac is not broken (just compiled from scratch, leopard debug build, worked fine.)

[Nikolas Zimmermann]

(In reply to comment #40)
> Landed in r64830. watching bots to assure mac is not broken (just compiled from scratch, leopard debug build, worked fine.)

Landed a build fix in r64832, landed two missing results in r64837.
Should be fixed now, closing bug when I'm sure.

[WebKit Review Bot]

http://trac.webkit.org/changeset/64830 might have broken GTK Linux 32-bit Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/64829
http://trac.webkit.org/changeset/64830

[Nikolas Zimmermann]

(In reply to comment #42)
> http://trac.webkit.org/changeset/64830 might have broken GTK Linux 32-bit Release
> The following changes are on the blame list:
> http://trac.webkit.org/changeset/64829
> http://trac.webkit.org/changeset/64830

Landed gtk rebaseline of 1 test in r64848.

[Nikolas Zimmermann]

Rebaselined win results in r64850. That's it!