Bug 42387 - SVG - stroke-width:0 bug with stroke other than "none"
Summary: SVG - stroke-width:0 bug with stroke other than "none"
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on: 42487
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-15 10:17 PDT by Fady Samuel
Modified: 2010-08-06 07:59 PDT (History)
9 users (show)

See Also:


Attachments
stroke-width:0 fix (10.98 KB, patch)
2010-07-15 10:25 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
stroke-width:0 fix (11.05 KB, patch)
2010-07-15 14:38 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
stroke-width:0 fix (11.05 KB, patch)
2010-07-15 14:39 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
stroke-width:0 fix (11.09 KB, patch)
2010-07-16 07:41 PDT, Fady Samuel
eric: review-
Details | Formatted Diff | Diff
stroke-width:0 fix (52.05 KB, patch)
2010-07-16 12:50 PDT, Fady Samuel
eric: review-
Details | Formatted Diff | Diff
stroke-width:0 fix (25.88 KB, patch)
2010-07-16 14:26 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
stroke-width:0 fix (406.67 KB, patch)
2010-07-23 15:11 PDT, Fady Samuel
krit: review-
krit: commit-queue-
Details | Formatted Diff | Diff
Animated Box bug (606 bytes, image/svg+xml)
2010-08-04 12:49 PDT, Fady Samuel
no flags Details
stroke-width:0 fix (71.97 KB, patch)
2010-08-04 13:00 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
stroke-width:0 fix (63.06 KB, patch)
2010-08-04 17:30 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Revised fix (129.75 KB, patch)
2010-08-06 00:29 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Revised fix - v2 (129.42 KB, patch)
2010-08-06 02:24 PDT, Nikolas Zimmermann
krit: review+
krit: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fady Samuel 2010-07-15 10:17:56 PDT
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.
Comment 1 Fady Samuel 2010-07-15 10:25:04 PDT
Created attachment 61683 [details]
stroke-width:0 fix
Comment 2 Darin Adler 2010-07-15 10:37:49 PDT
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
Comment 3 Fady Samuel 2010-07-15 14:38:29 PDT
Created attachment 61725 [details]
stroke-width:0 fix

Fixed minor style issue. Tested for broken layout tests.nothing new appears to break.
Comment 4 Fady Samuel 2010-07-15 14:39:34 PDT
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.
Comment 5 WebKit Commit Bot 2010-07-15 17:52:54 PDT
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 eseidel@chromium.org...
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).
Comment 6 Fady Samuel 2010-07-16 07:41:03 PDT
Created attachment 61804 [details]
stroke-width:0 fix

Accidentally deleted REVIEWED BY line in the layout test changelog.
Comment 7 Stephen White 2010-07-16 07:43:48 PDT
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?
Comment 8 Eric Seidel (no email) 2010-07-16 07:45:43 PDT
Comment on attachment 61804 [details]
stroke-width:0 fix

Your ChangeLog is wrong again.
Comment 9 Fady Samuel 2010-07-16 08:17:23 PDT
(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.
Comment 10 Fady Samuel 2010-07-16 12:50:31 PDT
Created attachment 61837 [details]
stroke-width:0 fix

Added more expectations according to conversation with Stephen White.
Comment 11 Stephen White 2010-07-16 13:26:21 PDT
LGTM, thanks.  (I'm not a reviewer, but perhaps Darin or Eric can take another look).
Comment 12 Eric Seidel (no email) 2010-07-16 13:39:13 PDT
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.
Comment 13 Fady Samuel 2010-07-16 14:26:40 PDT
Created attachment 61848 [details]
stroke-width:0 fix

Got rid of scrollbars in layout tests
Comment 14 David Levin 2010-07-16 14:41:47 PDT
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 @@
> +2010-07-16  Fady Samuel  <fsamuel@chromium.org>
> +
> +        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):
> +
Comment 15 David Levin 2010-07-16 14:44:02 PDT
Comment on attachment 61848 [details]
stroke-width:0 fix

OK. The images are different.

In the future please consider doing the ChangeLog as described.
Comment 16 WebKit Commit Bot 2010-07-16 15:42:45 PDT
Comment on attachment 61848 [details]
stroke-width:0 fix

Clearing flags on attachment: 61848

Committed r63593: <http://trac.webkit.org/changeset/63593>
Comment 17 WebKit Commit Bot 2010-07-16 15:42:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Tony Chang 2010-07-16 16:36:08 PDT
(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.
Comment 19 Tony Chang 2010-07-16 16:46:30 PDT
Reverted in http://trac.webkit.org/changeset/63598
Comment 20 Eric Seidel (no email) 2010-07-22 18:25:15 PDT
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.
Comment 21 Eric Seidel (no email) 2010-07-22 18:26:00 PDT
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.
Comment 22 Fady Samuel 2010-07-23 15:11:47 PDT
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.
Comment 23 Dirk Schulze 2010-07-27 00:07:51 PDT
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.
Comment 24 Fady Samuel 2010-07-28 07:44:04 PDT
> 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?
Comment 25 Fady Samuel 2010-07-28 08:09:22 PDT
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/
Comment 26 Nikolas Zimmermann 2010-07-28 08:13:19 PDT
(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?
Comment 27 Fady Samuel 2010-07-28 10:33:17 PDT
(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.
Comment 28 Nikolas Zimmermann 2010-07-29 07:37:50 PDT
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.
Comment 29 Fady Samuel 2010-07-30 14:04:12 PDT
(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...
Comment 30 Nikolas Zimmermann 2010-07-30 14:10:51 PDT
(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.
Comment 31 Fady Samuel 2010-08-04 10:28:59 PDT
(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.
Comment 32 Fady Samuel 2010-08-04 12:49:31 PDT
Created attachment 63482 [details]
Animated Box bug
Comment 33 Fady Samuel 2010-08-04 13:00:46 PDT
Created attachment 63484 [details]
stroke-width:0 fix

Removed Skia changes.
Comment 34 Fady Samuel 2010-08-04 17:30:37 PDT
Created attachment 63518 [details]
stroke-width:0 fix

Fixed some broken expectations
Comment 35 Nikolas Zimmermann 2010-08-06 00:29:17 PDT
Created attachment 63703 [details]
Revised fix

Came up with a better approach to fix the problem: Stop storing CSSValue/CSSValueList objects in SVGRenderStyle.
Comment 36 WebKit Review Bot 2010-08-06 00:30:42 PDT
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.
Comment 37 Nikolas Zimmermann 2010-08-06 02:24:05 PDT
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
Comment 38 Nikolas Zimmermann 2010-08-06 03:35:24 PDT
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.
Comment 39 Dirk Schulze 2010-08-06 04:01:23 PDT
Comment on attachment 63706 [details]
Revised fix - v2

r=me, please take a look at the buildbots on landing.
Comment 40 Nikolas Zimmermann 2010-08-06 04:22:41 PDT
Landed in r64830. watching bots to assure mac is not broken (just compiled from scratch, leopard debug build, worked fine.)
Comment 41 Nikolas Zimmermann 2010-08-06 05:10:10 PDT
(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.
Comment 42 WebKit Review Bot 2010-08-06 07:30:42 PDT
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
Comment 43 Nikolas Zimmermann 2010-08-06 07:45:08 PDT
(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.
Comment 44 Nikolas Zimmermann 2010-08-06 07:59:41 PDT
Rebaselined win results in r64850. That's it!