Bug 83568 - [SVG] Nothing should be stroked when the stroke-width is 0
Summary: [SVG] Nothing should be stroked when the stroke-width is 0
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Pierre Rossi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-10 05:37 PDT by Pierre Rossi
Modified: 2012-05-14 13:48 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.20 KB, patch)
2012-04-10 05:55 PDT, Pierre Rossi
no flags Details | Formatted Diff | Diff
Patch (8.51 KB, patch)
2012-04-10 09:45 PDT, Pierre Rossi
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (6.34 MB, application/zip)
2012-04-10 16:29 PDT, WebKit Review Bot
no flags Details
Patch (9.43 KB, patch)
2012-04-25 05:38 PDT, Pierre Rossi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pierre Rossi 2012-04-10 05:37:41 PDT
[SVG] stroke-width:0 should be treated like "none"
Comment 1 Pierre Rossi 2012-04-10 05:47:22 PDT
Unless I'm overlooking something, it seems to me when I read the spec [1] that we shouldn't leave it up to each individual port to ensure they respect it. In this particular case, I fail to see how delegating this decision does any good: it induces overhead at best, and non-compliance if we're a little less lucky.
In the case of the Qt port this was an issue: QPainter treats a zero-width pen as a cosmetic pen, which still caused that bug to be present.

[1] http://www.w3.org/TR/SVG/painting.html#StrokeWidthProperty
Comment 2 Pierre Rossi 2012-04-10 05:55:36 PDT
Created attachment 136440 [details]
Patch
Comment 3 Alexis Menard (darktears) 2012-04-10 06:45:49 PDT
(In reply to comment #1)
> Unless I'm overlooking something, it seems to me when I read the spec [1] that we shouldn't leave it up to each individual port to ensure they respect it. In this particular case, I fail to see how delegating this decision does any good: it induces overhead at best, and non-compliance if we're a little less lucky.
> In the case of the Qt port this was an issue: QPainter treats a zero-width pen as a cosmetic pen, which still caused that bug to be present.
> 
> [1] http://www.w3.org/TR/SVG/painting.html#StrokeWidthProperty

Well I disagree a bit the spec says : "A zero value causes no stroke to be painted." but that doesn't mean it doesn't have a stroke, it has a stroke property defined but the stroke width is 0 and in that case the spec define how it should renders.
Comment 4 Pierre Rossi 2012-04-10 06:53:25 PDT
@Alexis: ah, you're absolutely right. Ok, let's try something else then :)
Comment 5 Pierre Rossi 2012-04-10 09:45:02 PDT
Created attachment 136472 [details]
Patch
Comment 6 WebKit Review Bot 2012-04-10 16:29:27 PDT
Comment on attachment 136472 [details]
Patch

Attachment 136472 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12386048

New failing tests:
svg/custom/js-update-style.svg
svg/custom/path-zero-strokewidth.svg
Comment 7 WebKit Review Bot 2012-04-10 16:29:33 PDT
Created attachment 136570 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 8 Pierre Rossi 2012-04-12 10:03:28 PDT
It seems Chromium has the same issue.
Comment 9 Nikolas Zimmermann 2012-04-25 03:40:56 PDT
Comment on attachment 136472 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136472&action=review

Thanks for tackling this, some comments leading to r-:

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:123
> +    if (style()->svgStyle()->strokeWidth().isZero())
> +        return;

How about adding "hasVisibleStroke" to SVGRenderStyle, which returns "hasStroke() && !strokeWidth().isZero()".
Then this could turn into: if (!style()->svgStyle()->hasVisibleStroke()) return;

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:104
> +    if (!style()->svgStyle()->strokeWidth().isZero())

Ditto.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:276
> +    if (this->style()->svgStyle()->strokeWidth().isZero())

Ditto, not listing further occurrences of this.

> LayoutTests/ChangeLog:7
> +

You should also mark this as needs rebaseline for chromium.
Look for the SVG Tests section in platform/chromium/test_expectations.txt, and add BUGWK83568 : svg/custom/path-zero-strokewidth.svg = IMAGE, same for the other failure.
Comment 10 Pierre Rossi 2012-04-25 05:38:42 PDT
Created attachment 138791 [details]
Patch
Comment 11 Nikolas Zimmermann 2012-04-25 06:11:32 PDT
Comment on attachment 138791 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138791&action=review

r=me, but please make sure this builds after you landed it, as there are no EWS results available.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:276
> +    if (!this->style()->svgStyle()->hasVisibleStroke())

Can't you used the passed in RenderStyle* here? I think you can.
Comment 12 Pierre Rossi 2012-04-25 07:12:48 PDT
(In reply to comment #11)

Landed manually with suggested change in r115201.

> (From update of attachment 138791 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138791&action=review
> 
> r=me, but please make sure this builds after you landed it, as there are no EWS results available.

Alright, watching the bots now.
> 
> > Source/WebCore/rendering/svg/RenderSVGShape.cpp:276
> > +    if (!this->style()->svgStyle()->hasVisibleStroke())
> 
> Can't you used the passed in RenderStyle* here? I think you can.

And I think you're right :)

Thanks
Comment 13 Stephen Chenney 2012-05-14 13:48:08 PDT
Committed r116992: <http://trac.webkit.org/changeset/116992>
Comment 14 Stephen Chenney 2012-05-14 13:48:49 PDT
Updated Chromium expectations. Now we're really done.