Created attachment 156745 [details] Testcase Original bug: http://code.google.com/p/chromium/issues/detail?id=140472 For rects/ellipses with width or height of 0, we incorrectly return the bounding box as being empty. Fixing this is a two step process: 1) Rects/Ellipses currently have an isEmpty() check in updateShapeFromElement() that should be changed to isZero() 2) When rects/ellipses fall back to renderSVGShape, their fill bounding box should not fall back. The reason is: the fill bounding box does not care about non-scaling-strokes or rounded corners, so we do not have to rely on Skia's path bounding box calculation (which, because it is a pixel library, will return (0,0)). This is a good starter bug.
I would rather mark this bug as invalid. The spec says that an element with 0 height or width (namely circle, rect and eclipse) is in error. Therefore it doesn't get render and has no bounding box. I am pretty sure that we do the same like Mozilla, since we discussed that before.
I don't think that is the case Dirk. Only negative values are in error. "A negative value is an error. A value of zero disables rendering of the element." [1] "Returns the tight bounding box in current user space ... on the geometry of all contained graphics elements, exclusive of stroking, clipping, masking and filter effects). Note that getBBox must return the actual bounding box at the time the method was called, even in case the element has not yet been rendered." [2] Together these seem to indicate that a bounding box should still be returned, regardless of whether the element has been rendered. [1] http://www.w3.org/TR/SVG/single-page.html#shapes-RectElementWidthAttribute [2] http://www.w3.org/TR/SVG/single-page.html#types-__svg__SVGLocatable__getBBox
Reopening, now with a link to my inquiry on www-svg: http://lists.w3.org/Archives/Public/www-svg/2012Aug/0014.html
Created attachment 158473 [details] Patch about first step
Comment on attachment 158473 [details] Patch about first step 1) Rects/Ellipses currently have an isEmpty() check in updateShapeFromElement() that should be changed to isZero()
Comment on attachment 158473 [details] Patch about first step Attachment 158473 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13502524 New failing tests: svg/W3C-SVG-1.1/shapes-ellipse-02-t.svg svg/W3C-SVG-1.1/animate-elem-32-t.svg svg/W3C-SVG-1.1/shapes-intro-01-t.svg svg/W3C-SVG-1.1/shapes-rect-02-t.svg
Created attachment 158481 [details] Archive of layout-test-results from gce-cr-linux-08 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-08 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 158473 [details] Patch about first step View in context: https://bugs.webkit.org/attachment.cgi?id=158473&action=review I'm not a reviewer but I know this code fairly well. Overall this is a good start! I left a note below about the bug that led to your test failures. There is also a subtle bug if you have a "fallback" case like: <rect vector-effect="non-scaling-stroke" height="100" width="0"/> Essentially what is happening is we "fall back" to path rendering code for shapes with non-scaling-strokes. To fix this you can make a very similar change in SVGPathData::updatePathFromRectElement() (and similarly for circle and ellipse). > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:69 > + if (m_radii.isZero() || m_radii.width() < 0 || m_radii.height() < 0) Can we update the comment here to say something like: Spec: A negative value is an error. A value of zero disables rendering of the element. There is a bug here because if the width or height is zero, we will end up creating the correct fill bounding box, but then we actually inflate the stroke bounding box so that it renders. One way to fix this is to add a check if the width or height is zero before inflating the stroke bounding box. > Source/WebCore/rendering/svg/RenderSVGRect.cpp:69 > + if (boundingBoxSize.isZero() || boundingBoxSize.width() < 0 || boundingBoxSize.height() < 0) Ditto about the comment and bug. > LayoutTests/ChangeLog:8 > + Add new test to check bounding box size which is (0, 100). I think it will be best to add three tests here: one for <rect>, one for <circle>, and one for <ellipse>. If you want to fix the non-scaling-stroke bug as well, you could probably test both rect and non-scaling-stroke rect in the same test. > LayoutTests/svg/custom/getBBox-has-one-zero-value.html:3 > +<!-- Test that svg bounding boxes has a zero width or hegith --> Can you change this to "Test that the bounding box is valid for a zero-width rect.", or something similar? It's a little more clear what exactly is being tested.
I'll check your comment, and upload a patch soon.
Created attachment 159282 [details] Patch
Comment on attachment 159282 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159282&action=review The patch looks good in general, i wonder about the term "is in error" . Currently you just disable rendering for zero and negative case. A test with negative width/height/r/rx/ry would be great as well. > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:74 > + bool renderable = m_radii.width() && m_radii.height(); No extra variable needed, you just call it once. > Source/WebCore/rendering/svg/RenderSVGRect.cpp:81 > + bool renderable = boundingBoxSize.width() && boundingBoxSize.height(); Ditto.
Created attachment 159661 [details] Patch
I appreciate for your review. > (From update of attachment 159282 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=159282&action=review > > The patch looks good in general, i wonder about the term "is in error" . Currently you just disable rendering for zero and negative case. A test with negative width/height/r/rx/ry would be great as well. I modified just zero-width or zero-height cases' behavior. So I add some those cases. If I check some negative value cases, does it need to make another test file? or just add in same file? Then, can the file name be misleading..? > > > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:74 > > + bool renderable = m_radii.width() && m_radii.height(); > > No extra variable needed, you just call it once. > > > Source/WebCore/rendering/svg/RenderSVGRect.cpp:81 > > + bool renderable = boundingBoxSize.width() && boundingBoxSize.height(); > > Ditto.
(In reply to comment #14) > I appreciate for your review. > > (From update of attachment 159282 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=159282&action=review > > > > The patch looks good in general, i wonder about the term "is in error" . Currently you just disable rendering for zero and negative case. A test with negative width/height/r/rx/ry would be great as well. > > I modified just zero-width or zero-height cases' behavior. So I add some those cases. If I check some negative value cases, does it need to make another test file? or just add in same file? Then, can the file name be misleading..? This still doesn't address the non-scaling-stroke issue. Could you address that too?
Comment on attachment 159661 [details] Patch Attachment 159661 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/15913681 New failing tests: svg/as-image/img-preserveAspectRatio-support-2.html
Comment on attachment 159661 [details] Patch Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.
Created attachment 232057 [details] Patch
(In reply to comment #18) > Created an attachment (id=232057) [details] > Patch This patch will cause failures in the layout tests due to the repaint rect changing. Where it was 0x0 before, it is now the actual dimensions of the object + stroke (e.g. 52x2 for a 50x0 rect). I wanted to get feedback on whether this is ok, or if the repaint rect calculation needs to be updated to maintain the old values. The objects are still not rendered.
Comment on attachment 232057 [details] Patch Attachment 232057 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5914306650570752 New failing tests: svg/W3C-SVG-1.1/shapes-ellipse-02-t.svg svg/W3C-SVG-1.1/animate-elem-32-t.svg svg/W3C-SVG-1.1/shapes-intro-01-t.svg svg/W3C-SVG-1.1/shapes-rect-02-t.svg svg/custom/getBBox-js-circle-zerodimension.html svg/custom/getBBox-js-rect-zerodimension.html svg/W3C-SVG-1.1/shapes-circle-02-t.svg svg/custom/getBBox-js-ellipse-zerodimension.html
Created attachment 232061 [details] Archive of layout-test-results from webkit-ews-02 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-02 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 232057 [details] Patch Attachment 232057 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4996997736038400 New failing tests: svg/W3C-SVG-1.1/shapes-ellipse-02-t.svg svg/W3C-SVG-1.1/animate-elem-32-t.svg svg/W3C-SVG-1.1/shapes-intro-01-t.svg svg/W3C-SVG-1.1/shapes-rect-02-t.svg svg/custom/getBBox-js-circle-zerodimension.html svg/custom/getBBox-js-rect-zerodimension.html svg/W3C-SVG-1.1/shapes-circle-02-t.svg svg/custom/getBBox-js-ellipse-zerodimension.html
Created attachment 232063 [details] Archive of layout-test-results from webkit-ews-06 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-06 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 232057 [details] Patch Attachment 232057 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6069317388992512 New failing tests: svg/W3C-SVG-1.1/shapes-ellipse-02-t.svg svg/W3C-SVG-1.1/animate-elem-32-t.svg svg/W3C-SVG-1.1/shapes-intro-01-t.svg svg/W3C-SVG-1.1/shapes-rect-02-t.svg svg/custom/getBBox-js-circle-zerodimension.html svg/custom/getBBox-js-rect-zerodimension.html svg/W3C-SVG-1.1/shapes-circle-02-t.svg svg/custom/getBBox-js-ellipse-zerodimension.html
Created attachment 232092 [details] Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 232057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232057&action=review This patch is close but not quite right due to the fallback/non-fallback logic. Could you consider merging https://src.chromium.org/viewvc/blink?view=rev&revision=170097 instead? > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:55 > calculateRadiiAndCenter(); This function is only intended for the non-fallback codepath (it should probably assert that). This patch ends up calculating these values unnecessarily. > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:56 > + Please remove the extra tab spaces (here, and elsewhere).
Hi Phillp, Thanks for taking a look at the patch. I originally considered doing something similar to the Blink patch but didn't for a couple of reasons. For a rect, CGs path.addRect bails if either dimension is zero. Therefore I chose to not go to the fallback case at all if the rect is not going to render. For ellipse although CG doesn't bail, I noted that when the fallback path is followed, the ellipse will render if a non-scaling-stroke is used on a zero width object. It seems that the Blink fix suffers from this too (seen on Chrome Canary). http://jsfiddle.net/k2CTX/4/ In light of that, and to be consistent, again it seemed better to not go to the fall back case if the element is not supposed to render. I can make the change to stick with the fallback path for non scaling stroke, but will need to look at what other changes need to be made to stop the path rendering incorrectly. I guess we need to weigh up the performance cost of that change with the occasionally unnecessary call to calculateRadiiAndCenter()? Having said that, calculateRadiiAndCenter isn't doing much. I notice the Blink patch addresses my above question about the repaint rect. So do you think the correct course of action is to update the expected result in the layout tests here too? Will fix the tab whitespace. Still getting my install of xcode set up correctly...
(In reply to comment #27) > Hi Phillp, > > Thanks for taking a look at the patch. > > I originally considered doing something similar to the Blink patch but didn't for a couple of reasons. > > For a rect, CGs path.addRect bails if either dimension is zero. Therefore I chose to not go to the fallback case at all > if the rect is not going to render. > For ellipse although CG doesn't bail, I noted that when the fallback path is followed, the ellipse will > render if a non-scaling-stroke is used on a zero width object. > It seems that the Blink fix suffers from this too (seen on Chrome Canary). > http://jsfiddle.net/k2CTX/4/ > In light of that, and to be consistent, again it seemed better to not go to the fall back case if the element is not supposed to render. > > I can make the change to stick with the fallback path for non scaling stroke, but will need to look at what other changes need to be made to stop the path rendering incorrectly. > I guess we need to weigh up the performance cost of that change with the occasionally unnecessary call to calculateRadiiAndCenter()? > Having said that, calculateRadiiAndCenter isn't doing much. Excellent points. I completely missed this in my review. Because of how cheap calculateRadiiAndCenter is and how infrequently non-scaling-stroke is used, I think the approach you took is right after all. If possible, please include a word on this in your ChangeLog :) > > I notice the Blink patch addresses my above question about the repaint rect. So do you think the correct course of action is to update the expected result in the layout tests here too? Yeah, please update the expectations for those tests. > > Will fix the tab whitespace. Still getting my install of xcode set up correctly...
Created attachment 232191 [details] Patch
Comment on attachment 232191 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232191&action=review r+ with some minor nits. > Source/WebCore/ChangeLog:7 > + Please update this change log to describe the problem being solved. > Source/WebCore/ChangeLog:12 > + * rendering/svg/RenderSVGEllipse.cpp: Please add a line or so about what is changed in each of these function. > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:61 > + // Based on geometry, should we render? Nit: the geometry comment doesn't really help, lets just remove it. > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:63 > + if (m_radii.width() > 0 && m_radii.height() > 0) { Nit: isn't this if (!m_radii.isEmpty()) { > Source/WebCore/rendering/svg/RenderSVGRect.cpp:50 > + Nit: extra newline > Source/WebCore/rendering/svg/RenderSVGRect.cpp:67 > + if (boundingBoxSize.width() > 0 && boundingBoxSize.height() > 0) { Nit: if (!boundingBoxSize.isEmpty()) { > Source/WebCore/rendering/svg/RenderSVGRect.cpp:69 > + // fall back to RenderSVGShape Nit: // Fall back to RenderSVGShape. > LayoutTests/svg/custom/getBBox-js-circle-zerodimension.html:9 > + { Nit: { on the if line, and change the else line to } else { (here, and elsewhere)
Created attachment 232225 [details] Patch
Comment on attachment 232225 [details] Patch Looks great!
(In reply to comment #32) > (From update of attachment 232225 [details]) > Looks great! Thanks for taking the time to review!
(In reply to comment #32) > (From update of attachment 232225 [details]) > Looks great! What do I have to do to get this on the commit queue?
(In reply to comment #34) > (In reply to comment #32) > > (From update of attachment 232225 [details] [details]) > > Looks great! > > What do I have to do to get this on the commit queue? If you're a committer, just click "review patch" and change the "cq?" line to "cq+". I went ahead and clicked the button so this should be heading to the queue now.
Comment on attachment 232225 [details] Patch Clearing flags on attachment: 232225 Committed r169522: <http://trac.webkit.org/changeset/169522>
All reviewed patches have been landed. Closing bug.