RESOLVED FIXED 93290
getBBox() returns (0,0) when width or height is zero.
https://bugs.webkit.org/show_bug.cgi?id=93290
Summary getBBox() returns (0,0) when width or height is zero.
Philip Rogers
Reported 2012-08-06 12:37:17 PDT
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.
Attachments
Testcase (322 bytes, text/html)
2012-08-06 12:37 PDT, Philip Rogers
no flags
Patch about first step (5.29 KB, patch)
2012-08-14 18:38 PDT, Changhun Kang
no flags
Archive of layout-test-results from gce-cr-linux-08 (642.96 KB, application/zip)
2012-08-14 19:21 PDT, WebKit Review Bot
no flags
Patch (6.85 KB, patch)
2012-08-18 23:33 PDT, Changhun Kang
no flags
Patch (6.78 KB, patch)
2012-08-21 05:05 PDT, Changhun Kang
no flags
Patch (14.27 KB, patch)
2014-05-25 21:57 PDT, Nikos Andronikos
no flags
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (709.01 KB, application/zip)
2014-05-25 23:22 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (703.72 KB, application/zip)
2014-05-26 00:20 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (679.72 KB, application/zip)
2014-05-26 13:03 PDT, Build Bot
no flags
Patch (44.29 KB, patch)
2014-05-28 05:25 PDT, Nikos Andronikos
no flags
Patch (44.40 KB, patch)
2014-05-28 18:19 PDT, Nikos Andronikos
no flags
Dirk Schulze
Comment 1 2012-08-06 13:28:02 PDT
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.
Philip Rogers
Comment 2 2012-08-06 14:13:57 PDT
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
Philip Rogers
Comment 3 2012-08-06 14:39:46 PDT
Reopening, now with a link to my inquiry on www-svg: http://lists.w3.org/Archives/Public/www-svg/2012Aug/0014.html
Changhun Kang
Comment 4 2012-08-14 18:38:20 PDT
Created attachment 158473 [details] Patch about first step
Changhun Kang
Comment 5 2012-08-14 18:43:59 PDT
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()
Changhun Kang
Comment 6 2012-08-14 18:59:50 PDT
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()
WebKit Review Bot
Comment 7 2012-08-14 19:21:35 PDT
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
WebKit Review Bot
Comment 8 2012-08-14 19:21:42 PDT
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
Philip Rogers
Comment 9 2012-08-14 21:35:22 PDT
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.
Changhun Kang
Comment 10 2012-08-14 23:00:58 PDT
I'll check your comment, and upload a patch soon.
Changhun Kang
Comment 11 2012-08-18 23:33:28 PDT
Dirk Schulze
Comment 12 2012-08-19 08:03:07 PDT
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.
Changhun Kang
Comment 13 2012-08-21 05:05:41 PDT
Changhun Kang
Comment 14 2012-08-21 05:15:15 PDT
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.
Philip Rogers
Comment 15 2012-08-23 10:29:24 PDT
(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?
Build Bot
Comment 16 2013-01-17 02:21:36 PST
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
Andreas Kling
Comment 17 2014-02-05 10:53:36 PST
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.
Nikos Andronikos
Comment 18 2014-05-25 21:57:08 PDT
Nikos Andronikos
Comment 19 2014-05-25 21:59:55 PDT
(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.
Build Bot
Comment 20 2014-05-25 23:22:16 PDT
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
Build Bot
Comment 21 2014-05-25 23:22:22 PDT
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
Build Bot
Comment 22 2014-05-26 00:20:45 PDT
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
Build Bot
Comment 23 2014-05-26 00:20:52 PDT
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
Build Bot
Comment 24 2014-05-26 13:03:03 PDT
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
Build Bot
Comment 25 2014-05-26 13:03:11 PDT
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
Philip Rogers
Comment 26 2014-05-26 13:26:57 PDT
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).
Nikos Andronikos
Comment 27 2014-05-26 18:14:19 PDT
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...
Philip Rogers
Comment 28 2014-05-26 18:37:49 PDT
(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...
Nikos Andronikos
Comment 29 2014-05-28 05:25:36 PDT
Philip Rogers
Comment 30 2014-05-28 11:21:23 PDT
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)
Nikos Andronikos
Comment 31 2014-05-28 18:19:56 PDT
Philip Rogers
Comment 32 2014-05-28 18:41:06 PDT
Comment on attachment 232225 [details] Patch Looks great!
Nikos Andronikos
Comment 33 2014-05-28 18:41:54 PDT
(In reply to comment #32) > (From update of attachment 232225 [details]) > Looks great! Thanks for taking the time to review!
Nikos Andronikos
Comment 34 2014-06-01 19:00:40 PDT
(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?
Philip Rogers
Comment 35 2014-06-01 21:32:56 PDT
(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.
WebKit Commit Bot
Comment 36 2014-06-01 22:01:44 PDT
Comment on attachment 232225 [details] Patch Clearing flags on attachment: 232225 Committed r169522: <http://trac.webkit.org/changeset/169522>
WebKit Commit Bot
Comment 37 2014-06-01 22:01:53 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.