Summary: | [SVG] Nothing should be stroked when the stroke-width is 0 | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Pierre Rossi <pierre.rossi> | ||||||||||
Component: | New Bugs | Assignee: | Pierre Rossi <pierre.rossi> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | dglazkov, kling, menard, pdr, schenney, webkit.review.bot, zimmermann | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Pierre Rossi
2012-04-10 05:37:41 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 Created attachment 136440 [details]
Patch
(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. @Alexis: ah, you're absolutely right. Ok, let's try something else then :) Created attachment 136472 [details]
Patch
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 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
It seems Chromium has the same issue. 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. Created attachment 138791 [details]
Patch
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. (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 Committed r116992: <http://trac.webkit.org/changeset/116992> Updated Chromium expectations. Now we're really done. |