Bug 93290 - getBBox() returns (0,0) when width or height is zero.
: getBBox() returns (0,0) when width or height is zero.
Status: REOPENED
: WebKit
SVG
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
: EasyFix
:
:
  Show dependency treegraph
 
Reported: 2012-08-06 12:37 PST by
Modified: 2014-03-20 14:36 PST (History)


Attachments
Testcase (322 bytes, text/html)
2012-08-06 12:37 PST, Philip Rogers
no flags Details
Patch about first step (5.29 KB, patch)
2012-08-14 18:38 PST, Changhun Kang
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-08 (642.96 KB, application/zip)
2012-08-14 19:21 PST, WebKit Review Bot
no flags Details
Patch (6.85 KB, patch)
2012-08-18 23:33 PST, Changhun Kang
no flags Review Patch | Details | Formatted Diff | Diff
Patch (6.78 KB, patch)
2012-08-21 05:05 PST, Changhun Kang
buildbot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-08-06 12:37:17 PST
Created an attachment (id=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 From 2012-08-06 13:28:02 PST -------
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 From 2012-08-06 14:13:57 PST -------
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 From 2012-08-06 14:39:46 PST -------
Reopening, now with a link to my inquiry on www-svg: http://lists.w3.org/Archives/Public/www-svg/2012Aug/0014.html
------- Comment #4 From 2012-08-14 18:38:20 PST -------
Created an attachment (id=158473) [details]
Patch
------- Comment #5 From 2012-08-14 18:43:59 PST -------
(From update of attachment 158473 [details])

1) Rects/Ellipses currently have an isEmpty() check in updateShapeFromElement() that should be changed to isZero()
------- Comment #6 From 2012-08-14 18:59:50 PST -------
(From update of attachment 158473 [details])

1) Rects/Ellipses currently have an isEmpty() check in updateShapeFromElement() that should be changed to isZero()
------- Comment #7 From 2012-08-14 19:21:35 PST -------
(From update of attachment 158473 [details])
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 From 2012-08-14 19:21:42 PST -------
Created an attachment (id=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 From 2012-08-14 21:35:22 PST -------
(From update of attachment 158473 [details])
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 From 2012-08-14 23:00:58 PST -------
I'll check your comment, and upload a patch soon.
------- Comment #11 From 2012-08-18 23:33:28 PST -------
Created an attachment (id=159282) [details]
Patch
------- Comment #12 From 2012-08-19 08:03:07 PST -------
(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.

> 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 From 2012-08-21 05:05:41 PST -------
Created an attachment (id=159661) [details]
Patch
------- Comment #14 From 2012-08-21 05:15:15 PST -------
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..?



> 
> > 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 From 2012-08-23 10:29:24 PST -------
(In reply to comment #14)
> I appreciate for your review.
> > (From update of attachment 159282 [details] [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 From 2013-01-17 02:21:36 PST -------
(From update of attachment 159661 [details])
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 From 2014-02-05 10:53:36 PST -------
(From update of attachment 159661 [details])
Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.