Bug 93290 - getBBox() returns (0,0) when width or height is zero.
: getBBox() returns (0,0) when width or height is zero.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: SVG
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Nikos Andronikos
: EasyFix
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-06 12:37 PDT by Philip Rogers
Modified: 2014-06-01 22:01 PDT (History)
20 users (show)

See Also:


Attachments
Testcase (322 bytes, text/html)
2012-08-06 12:37 PDT, Philip Rogers
no flags Details
Patch about first step (5.29 KB, patch)
2012-08-14 18:38 PDT, Changhun Kang
no flags Details | Formatted Diff | Diff
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 Details
Patch (6.85 KB, patch)
2012-08-18 23:33 PDT, Changhun Kang
no flags Details | Formatted Diff | Diff
Patch (6.78 KB, patch)
2012-08-21 05:05 PDT, Changhun Kang
no flags Details | Formatted Diff | Diff
Patch (14.27 KB, patch)
2014-05-25 21:57 PDT, Nikos Andronikos
no flags Details | Formatted Diff | Diff
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 Details
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 Details
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 Details
Patch (44.29 KB, patch)
2014-05-28 05:25 PDT, Nikos Andronikos
no flags Details | Formatted Diff | Diff
Patch (44.40 KB, patch)
2014-05-28 18:19 PDT, Nikos Andronikos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philip Rogers 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.
Comment 1 Dirk Schulze 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.
Comment 2 Philip Rogers 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
Comment 3 Philip Rogers 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
Comment 4 Changhun Kang 2012-08-14 18:38:20 PDT
Created attachment 158473 [details]
Patch about first step
Comment 5 Changhun Kang 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()
Comment 6 Changhun Kang 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()
Comment 7 WebKit Review Bot 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
Comment 8 WebKit Review Bot 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
Comment 9 Philip Rogers 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.
Comment 10 Changhun Kang 2012-08-14 23:00:58 PDT
I'll check your comment, and upload a patch soon.
Comment 11 Changhun Kang 2012-08-18 23:33:28 PDT
Created attachment 159282 [details]
Patch
Comment 12 Dirk Schulze 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.
Comment 13 Changhun Kang 2012-08-21 05:05:41 PDT
Created attachment 159661 [details]
Patch
Comment 14 Changhun Kang 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.
Comment 15 Philip Rogers 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?
Comment 16 Build Bot 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
Comment 17 Andreas Kling 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.
Comment 18 Nikos Andronikos 2014-05-25 21:57:08 PDT
Created attachment 232057 [details]
Patch
Comment 19 Nikos Andronikos 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.
Comment 20 Build Bot 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
Comment 21 Build Bot 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
Comment 22 Build Bot 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
Comment 23 Build Bot 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
Comment 24 Build Bot 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
Comment 25 Build Bot 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
Comment 26 Philip Rogers 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).
Comment 27 Nikos Andronikos 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...
Comment 28 Philip Rogers 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...
Comment 29 Nikos Andronikos 2014-05-28 05:25:36 PDT
Created attachment 232191 [details]
Patch
Comment 30 Philip Rogers 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)
Comment 31 Nikos Andronikos 2014-05-28 18:19:56 PDT
Created attachment 232225 [details]
Patch
Comment 32 Philip Rogers 2014-05-28 18:41:06 PDT
Comment on attachment 232225 [details]
Patch

Looks great!
Comment 33 Nikos Andronikos 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!
Comment 34 Nikos Andronikos 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?
Comment 35 Philip Rogers 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.
Comment 36 WebKit Commit Bot 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>
Comment 37 WebKit Commit Bot 2014-06-01 22:01:53 PDT
All reviewed patches have been landed.  Closing bug.