Bug 93290 – getBBox() returns (0,0) when width or height is zero.

• Component: SVG
• Status: RESOLVED FIXED

[Philip Rogers]

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.

[Dirk Schulze]

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]

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]

Reopening, now with a link to my inquiry on www-svg: http://lists.w3.org/Archives/Public/www-svg/2012Aug/0014.html

[Changhun Kang]

Created attachment 158473 [details]
Patch about first step

[Changhun Kang]

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 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 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]

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 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]

I'll check your comment, and upload a patch soon.

[Changhun Kang]

Created attachment 159282 [details]
Patch

[Dirk Schulze]

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]

Created attachment 159661 [details]
Patch

[Changhun Kang]

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]

(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 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 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]

Created attachment 232057 [details]
Patch

[Nikos Andronikos]

(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 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]

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 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]

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 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]

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 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]

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]

(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]

Created attachment 232191 [details]
Patch

[Philip Rogers]

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]

Created attachment 232225 [details]
Patch

[Philip Rogers]

Comment on attachment 232225 [details]
Patch

Looks great!

[Nikos Andronikos]

(In reply to comment #32)
> (From update of attachment 232225 [details])
> Looks great!

Thanks for taking the time to review!

[Nikos Andronikos]

(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]

(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 on attachment 232225 [details]
Patch

Clearing flags on attachment: 232225

Committed r169522: <http://trac.webkit.org/changeset/169522>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.