Elements with rendering disabled due to dimensions should not contribute to parent bounding box

• Component: SVG
• Status: RESOLVED FIXED

[Nikos Andronikos]

Recently the SVG WG clarified that paths with no data (e.g. missing the d attribute) must not contribute to any ancestor element's bounding box. See http://www.w3.org/2014/03/06-svg-minutes.html#item04

This should also be the case for basic shapes. A rect, circle or ellipse should not contribute to an ancestor element's bounding box when rendering is disabled due to width/rx <= 0 or height/ry <= 0.
The text of SVG 1.1 was lacking in detail in this area, but it has been improved in SVG 2, with the updated text coming from SVG Tiny 1.2.
See http://www.w3.org/TR/SVG2/coords.html#BoundingBoxes

See http://jsfiddle.net/tH289/1/ for a test

nb: FireFox appears to have made this fix in FireFox 30.

[Nikos Andronikos]

patch to follow

[Nikos Andronikos]

Created attachment 233585 [details]
Patch

[Dirk Schulze]

Comment on attachment 233585 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=233585&action=review

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:164
> +    return m_fillBoundingBox.isEmpty();

Don't think that this is correct: M20,20,L20,20z is a valid path and has a bounding box of 20,20,0,0. Maybe check if the created Path is empty?

> Source/WebCore/rendering/svg/RenderSVGShape.h:70
> +        // returning false by default ensures backwards compatibility for

s/returning/Returning/

> LayoutTests/ChangeLog:9
> +        Test, for each element type, that when rendering is disabled, that element does not contribute
> +        to the bounding box for an ancestor element.

Could you write W3C tests that can then be committed to the W3C in parallel please?

http://testthewebforward.org/docs/testharness-tutorial.html

[Nikos Andronikos]

Created attachment 233776 [details]
Patch

[Nikos Andronikos]

> Could you write W3C tests that can then be committed to the W3C in parallel please?
> 
> http://testthewebforward.org/docs/testharness-tutorial.html

Thanks for the r/v Dirk, I'll look into doing this as well.

[Build Bot]

Comment on attachment 233776 [details]
Patch

Attachment 233776 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6665580408995840

New failing tests:
svg/W3C-SVG-1.1/shapes-rect-02-t.svg

[Build Bot]

Created attachment 233781 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Nikos Andronikos]

Created attachment 233783 [details]
Patch

[Dirk Schulze]

Comment on attachment 233783 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=233783&action=review

r=me

> LayoutTests/svg/custom/getBBox-perpendicular-polygon.svg:1
> +<?xml version="1.0" encoding="UTF-8"?>

This is just noise and we usually remove that from tests. Ditto for the other tests.

[Nikos Andronikos]

Created attachment 233786 [details]
Patch

[Build Bot]

Comment on attachment 233786 [details]
Patch

Attachment 233786 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6500298256285696

New failing tests:
media/video-ended-event-negative-playback.html

[Build Bot]

Created attachment 233796 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Nikos Andronikos]

I've had a look at the one failing test.

It doesn't use svg and doesn't hit any of the code changed in this patch.

My feeling is that the test is just flaky. 
Is there a way to kick off the tests again, or can a reviewer make a decision to disregard the result?

[Nikos Andronikos]

Created attachment 234473 [details]
Patch

[Darin Adler]

Comment on attachment 234473 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234473&action=review

I’m going to say review+ because I think this code change is helpful, gets us closer to a correct implementation, but I think there are mistakes, and tests that cover edge cases would likely show us this.

> Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:158
> +    return m_fillBoundingBox.isEmpty();

Likely the same problem with m_usePathFallback that I mention below in RenderSVGRect::isRenderingDisabled.

> Source/WebCore/rendering/svg/RenderSVGEllipse.h:39
> +    virtual bool isRenderingDisabled() const override;

Our usual practice is to make such overrides private since we only intend to call the functions polymorphically. If we called this directly it would probably be a programming mistake.

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:166
> +    // For a polygon, polyline and path, rendering is disabled if the d
> +    // or points attribute is missing or empty.
> +    return path().isEmpty();

This comment doesn’t match this code. It’s not good to have that comment before code that is entirely different. If we keep the comment it needs to explain why the code below is correct.

I don’t fully understand why the path will only be empty in the case mentioned above. It might be true, but if it’s true, it’s definitely not obvious.

> Source/WebCore/rendering/svg/RenderSVGPath.h:38
> +    virtual bool isRenderingDisabled() const override;

Can make this private for same reason as above.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:157
> +    // Some combinations of attributes cause SVG elements
> +    // to not render.

I don’t think this comment clarifies; it’s kind of an oblique remark.

To give one example of how it’s misleading, to me it makes it sound like this function prevents an SVG element from rendering. But it doesn’t. This function is only used to decide whether this shape contributes to its parent’s bounding box, and does not directly affect rendering.

It’s also confusing because this function doesn’t check the values of attributes on an SVG element. Instead it checks a computed box that depends not only on values of attributes, but also on other things such as the length context and even on the vector effect (non-scaling stroke).

Besides being non-clear, the comment does not belong here. If it was important to have this comment, we would not want it just on one particular override of isRenderingDisabled since it applies equally to all isRenderingDisabled functions. Maybe we’d put it at the call site or in the RenderSVGShape::isRenderingDisabled function.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:159
> +    // For rects a negative dimension is an error and
> +    // a zero dimension disables rendering.

This comment is a bit mysterious (does not explain how the error relates to the rendering disabled issue). But also, if a comment like this was needed, we’d want one in RenderSVGEllipse::isRenderingDisabled too. I suggest just omitting the comment. If we keep it, then we should make the comment more pertinent, explaining exactly why this is the correct code.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:160
> +    return m_fillBoundingBox.isEmpty();

I think this is not quite right. The RenderSVGRect::updateShapeFromElement function has a different set of conditions that determine wither rendering is enabled and this doesn’t seem to quite match them. In particular, I think a correct version of this function needs to look at m_usePathFallback, not just m_fillBoundingBox. When m_usePathFallback is true, it’s possible that m_fillBoundingBox contains an old, now-invalid bounding box that was computed earlier. Some kind of check that involves path() is likely needed in that case.

> Source/WebCore/rendering/svg/RenderSVGRect.h:43
> +    virtual bool isRenderingDisabled() const override;

Can make this private for same reason as above.

> Source/WebCore/rendering/svg/RenderSVGShape.h:73
> +    virtual bool isRenderingDisabled() const
> +    {
> +        // Returning false by default ensures backwards compatibility for
> +        // elements that don't implement this method.
> +        return false;
> +    }

The base implementation should not be put here in the class definition. I suggest putting it in the .cpp file, since it won’t be inlined since it’s going to be a virtual function call.

The comment does not have enough context. Today, the term “backwards compatibility” makes sense to us; the compatibility is with how the code was before this patch. But in the future it won’t make sense because nothing will indicate that at one time this function didn’t exist. Instead the right way to explain this is that any class that does not override this function will never disable rendering.

[Darin Adler]

Comment on attachment 234473 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234473&action=review

>> Source/WebCore/rendering/svg/RenderSVGShape.h:73
>> +    }
> 
> The base implementation should not be put here in the class definition. I suggest putting it in the .cpp file, since it won’t be inlined since it’s going to be a virtual function call.
> 
> The comment does not have enough context. Today, the term “backwards compatibility” makes sense to us; the compatibility is with how the code was before this patch. But in the future it won’t make sense because nothing will indicate that at one time this function didn’t exist. Instead the right way to explain this is that any class that does not override this function will never disable rendering.

Actually that was wrong. The right way to explain this is something more like any shape class that does not override this function will always “contribute to the bounding box of its parent”. Because this function does not control rendering at all. In fact, we might need to name it something different given what it does and does not do.

[Nikos Andronikos]

Hi Darin, thanks for the review.

Hopefully my patch (to follow) will address most of the issues you pointed out.
A couple of comments below...

(In reply to comment #15)
> (From update of attachment 234473 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234473&action=review
<snip> 
> > Source/WebCore/rendering/svg/RenderSVGEllipse.h:39
> > +    virtual bool isRenderingDisabled() const override;
> 
> Our usual practice is to make such overrides private since we only intend to call the functions polymorphically. If we called this directly it would probably be a programming mistake.
> 

I did notice that seemed to be the practice, but wasn't totally sure.
I figured making this public would elicit a comment to help my understanding. So thanks.

> 
> > Source/WebCore/rendering/svg/RenderSVGRect.cpp:160
> > +    return m_fillBoundingBox.isEmpty();
> 
> I think this is not quite right. The RenderSVGRect::updateShapeFromElement function has a different set of conditions that determine wither rendering is enabled and this doesn’t seem to quite match them. In particular, I think a correct version of this function needs to look at m_usePathFallback, not just m_fillBoundingBox. When m_usePathFallback is true, it’s possible that m_fillBoundingBox contains an old, now-invalid bounding box that was computed earlier. Some kind of check that involves path() is likely needed in that case.


RenderSVGShape::updateShapeFromElement, which is called in the fall back case always sets m_fillBoundingBox.
So here we can ignore which method is used for rendering. 
The test GetBBox-container-hiddenchild.html contains a rect and an ellipse element that hit the fall back code paths to ensure this is correct.

Personally I would consider it a bug if m_fillBoundingBox contained out of date info but I don't think that can be the case.

> 
> > Source/WebCore/rendering/svg/RenderSVGRect.h:43
> > +    virtual bool isRenderingDisabled() const override;
> 
> Can make this private for same reason as above.
> 
> > Source/WebCore/rendering/svg/RenderSVGShape.h:73
> > +    virtual bool isRenderingDisabled() const
> > +    {
> > +        // Returning false by default ensures backwards compatibility for
> > +        // elements that don't implement this method.
> > +        return false;
> > +    }
> 
> The base implementation should not be put here in the class definition. I suggest putting it in the .cpp file, since it won’t be inlined since it’s going to be a virtual function call.
> 
> The comment does not have enough context. Today, the term “backwards compatibility” makes sense to us; the compatibility is with how the code was before this patch. But in the future it won’t make sense because nothing will indicate that at one time this function didn’t exist. Instead the right way to explain this is that any class that does not override this function will never disable rendering.

On further thought, I think it might be better to make this pure virtual since the conditions for each element are mostly different and there's no reason not to implement it for all sub classes.

[Nikos Andronikos]

Created attachment 234480 [details]
Patch

[Darin Adler]

Comment on attachment 234480 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234480&action=review

> Source/WebCore/rendering/svg/RenderSVGPath.h:38
> +    virtual bool isRenderingDisabled() const override;

Should be private as discussed in last patch feedback.

[Nikos Andronikos]

Created attachment 234532 [details]
Patch

[Radu Stavila]

Comment on attachment 234532 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234532&action=review

> Source/WebCore/rendering/svg/RenderSVGShape.h:60
> +    virtual bool isRenderingDisabled() const = 0;

Just a thought: wouldn't it be better to have this not pure virtual and instead have an implementation somethink like "return isEmpty()"?

[Nikos Andronikos]

(In reply to comment #21)
> (From update of attachment 234532 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234532&action=review
> 
> > Source/WebCore/rendering/svg/RenderSVGShape.h:60
> > +    virtual bool isRenderingDisabled() const = 0;
> 
> Just a thought: wouldn't it be better to have this not pure virtual and instead have an implementation somethink like "return isEmpty()"?

My feeling is no.
At the moment, two implementations match, but this might change in future and the base implementation might then no longer be the most common.
It's also probably good for authors of future sub classes of RenderSVGShape to know that they should implement this method with consideration for the specification of the particular SVG element they are implementing.

On another topic...
Any suggestion on what to do with the red efl build? It doesn't look related to this patch. What would the normal procedure be in this case? Is there any way to re-run the build without uploading another patch?

[Gyuyoung Kim]

(In reply to comment #22)

> On another topic...
> Any suggestion on what to do with the red efl build? It doesn't look related to this patch. What would the normal procedure be in this case? Is there any way to re-run the build without uploading another patch?

AFAIK, we have to re-upload the patch to run ews again. Anyway, it looks this efl ews alarm looks false alarm. I succeed to build EFL port with this patch locally. Please ignore efl alarm.

[Dirk Schulze]

Comment on attachment 234532 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 234532 [details]
Patch

Clearing flags on attachment: 234532

Committed r171046: <http://trac.webkit.org/changeset/171046>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.