Created attachment 119359 [details] Test case A line drawn along the border of an SVG element will be clipped if opacity != 1. In the attached test case there should be two lines of length 50px. The line with opacity=0.9 is not drawn but the line with opacity=1.0 is. (Found while diving into http://code.google.com/p/chromium/issues/detail?id=107510)
Created attachment 119654 [details] Fix incorrect stroke near borders
Comment on attachment 119654 [details] Fix incorrect stroke near borders Attachment 119654 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10935002 New failing tests: http/tests/inspector-enabled/dedicated-workers-list.html fast/repaint/scroll-fixed-reflected-layer.html fast/repaint/fixed-scale.html media/video-poster-blocked-by-willsendrequest.html css2.1/20110323/abspos-containing-block-initial-004b.htm fast/repaint/fixed-tranformed.html svg/batik/text/textAnchor.svg css2.1/20110323/abspos-containing-block-initial-004d.htm fast/repaint/moving-shadow-on-path.html fast/repaint/fixed-table-overflow-zindex.html http/tests/inspector/resource-tree/resource-tree-document-url.html
I believe you that this works... but it feels like a bit of a one-off-hack for this situation. Should we be doing this inflate in a more central place?
Comment on attachment 119654 [details] Fix incorrect stroke near borders View in context: https://bugs.webkit.org/attachment.cgi?id=119654&action=review Eric is right, the patch is not correct. Also what if the border is thicker than 1? You can not hard code the inflation. > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:94 > FloatRect repaintRect = object->repaintRectInLocalCoordinates(); The repaintRectInLocalCoordinates should already include the border. If we clip sty of the border away, it seems like a bug in repaintRectInLocalCoordinates(). Is it just for lines? Do you see that for SVGRects or SVGCircles as well?
Created attachment 121433 [details] Example of clipping fixed with this patch (In reply to comment #3) > I believe you that this works... but it feels like a bit of a one-off-hack for this situation. Should we be doing this inflate in a more central place? I agree it feels like a one-off hack but it's the result of clipping so deep in the stack. This codepath only occurs because a separate transparency layer is created for objects with opacity or shadow and we have to clip when drawing. That said, I really don't like a random 0.5 inflate in the middle of the code. I'll have a followup patch that switches to enclosingLayoutRect(object->repaintRectInLocalCoordinates()), which is used for similar purposes elsewhere. Instead of adding 0.5, enclosingLayoutRect uses floor on the top and left coordinates, and ceil on the right and bottom coordinates, which should be exactly what we need. (In reply to comment #4) > (From update of attachment 119654 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=119654&action=review > > Eric is right, the patch is not correct. Also what if the border is thicker than 1? You can not hard code the inflation. This extra padding is only to account for the "overdraw" outside the repaint rect, not the border itself. For instance, the rightmost line in the test extends from x=99.5 to x=100.5, but is actually drawn using pixels at x=99. Previously, we were clipping the pixels at x=99 and not drawing them. > > > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:94 > > FloatRect repaintRect = object->repaintRectInLocalCoordinates(); > > The repaintRectInLocalCoordinates should already include the border. If we clip sty of the border away, it seems like a bug in repaintRectInLocalCoordinates(). Is it just for lines? Do you see that for SVGRects or SVGCircles as well? Unfortunately it looks like repaintRect is just the minimum enclosing rect for the path and does not account for the overdraw. This bug occurs for all shapes (lines, rects, even circles) and this patch fixes them all. Looking at the diffs of the new failing tests, the current expected results clearly have clipping, which is fixed with this patch (see the attached image clipping.png).
Created attachment 121434 [details] Fix incorrect stroke near borders
Comment on attachment 121434 [details] Fix incorrect stroke near borders Attachment 121434 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11167056 New failing tests: svg/css/stars-with-shadow.html svg/css/composite-shadow-example.html svg/css/composite-shadow-with-opacity.html fast/repaint/moving-shadow-on-path.html svg/custom/container-opacity-clip-viewBox.svg svg/custom/marker-opacity.svg svg/custom/js-update-bounce.svg
The reviewers were right to question an inflate() so deep in the code--in exploring this more I found this clip bug affects Skia and CG differently. I've played with some other ways of fixing this but the end result is that this patch here is too simplistic. I'd like to abandon my approach here for now, as it's clear there are at least a couple bugs that I need to tackle separately. (Some details) Skia's clip() is not antialiased, which causes the clipped aliasing on all non-integer-aligned shapes. This is actually different from the border bug on semi-transparent lines, which seems to affect CG and Skia, but was erroneously "fixed" by inflating the repaint rect.
(In reply to comment #8) > The reviewers were right to question an inflate() so deep in the code--in exploring this more I found this clip bug affects Skia and CG differently. I've played with some other ways of fixing this but the end result is that this patch here is too simplistic. Can your remove the review flag please? Or should it still get reviewed?
Created attachment 141743 [details] Fix SVG clipping in SVGRenderingContext This bit me again over the weekend so I'd like to resurrect this bug. Looking into this issue again, I think the proposed fix before actually wasn't too local after all. It looks like every other instance of clipping for transparency layers snaps the values; there are many examples in RenderLayer. Eric and Dirk, do you still think this approach is too local? The new patch has a much better testcase but needs rebaselines and is not ready to land yet--just looking for a review of the approach.
Comment on attachment 141743 [details] Fix SVG clipping in SVGRenderingContext Attachment 141743 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12685448 New failing tests: svg/as-background-image/svg-as-background-6.html fast/backgrounds/svg-as-mask.html svg/css/stars-with-shadow.html svg/css/composite-shadow-example.html svg/css/composite-shadow-with-opacity.html svg/as-background-image/svg-as-background-5.html fast/repaint/moving-shadow-on-path.html svg/custom/container-opacity-clip-viewBox.svg svg/custom/marker-opacity.svg svg/custom/js-update-bounce.svg
Created attachment 141787 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
do you still want this reviewed?
I took the test case from Chrome bug into JSFiddle: Link - https://jsfiddle.net/2b4avqng/show It seems to work fine in Safari 16.2 and matching Chrome Canary 111 and Firefox Nightly 110. Similarly the attached test case is also matching in all browser, although it is not showing the expected result of two lines except just one. Do we need to do anything further? Thanks!
I am unable to reproduce this bug, so marking this as 'RESOLVED CONFIGURATION CHANGED', please reopen if it is still reproducible.