Bug 72796 – [SVG] getBBox on a <g> within symbol returns 0 but with defs it works correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Raks]

When we do a getBBox on the <g> element inside a symbol, the value returned is 0,0
But if the <g> element is inside a <defs>, correct value is returned.

Following is the SVG code, If we replace symbol with defs we get the values correctly
Getting the BBox on a symbol is very important because we are making a library of graphical units using symbol and want to know the dimensions of elements in the symbol so that appropriate transformations can be done when the symbol is actually used with the <use> tag.

Moreover as per standards since we are doing a getBBox on a <g> which is SVGLocatable, getBBox should work correctly


<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1 Tiny//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11-tiny.dtd">
<svg baseProfile="tiny" display="inherit" version="1.1" xml:space="preserve" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:ev="http://www.w3.org/2001/xml-events">
    <script><![CDATA[
        window.onload = function(){
            var l2 = document.getElementById('g0') ;
			alert(l2.getBBox().width) ;
			//alert(l2);
        }
    ]]></script>
    <symbol>
        <g id="g0">			 
           <circle id="g1" cx="200" cy="200" r="20" fill="red"/>
           <circle cx="200" cy="100" r="20"/>
           <animateTransform repeatCount="indefinite" attributeName="transform" begin="0" type="translate" additive="sum" fill="freeze"  dur="2" keyTimes="0;0.5;1" values="200 150; 20 150;200 150"/>
           <animateTransform repeatCount="indefinite" attributeName="transform" begin="0" additive="sum" type="scale" fill="freeze"  dur="2" keyTimes="0;0.5;1" values="2 2;1 1;2 2"/>
           <animateTransform repeatCount="indefinite" attributeName="transform" begin="0" additive="sum" type="translate" fill="freeze"  dur="2" keyTimes="0;0.5;1" values="-200 -150; -20 -150;-200 -150"/>
       </g>
    </symbol>
    <use xlink:href="#symbol0" fill="green">
        <animate calcMode="linear" attributeName="display" begin="0" dur="2" keyTimes="0;0.2;0.5;0.8;1" values="inherit;none;inherit;none;inherit"/>
    </use>
    <use x="200" xlink:href="#symbol0" fill="blue"/>
</svg>

[Leo Yang]

Created attachment 119819 [details]
Patch

With this patch, some test cases need to be re-baselined. I'll grab the updated test results after the patch in.

[WebKit Review Bot]

Comment on attachment 119819 [details]
Patch

Attachment 119819 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10988462

New failing tests:
tables/mozilla/bugs/bug4527.html
http/tests/inspector-enabled/dedicated-workers-list.html
svg/carto.net/slider.svg
svg/custom/use-clipped-hit.svg
svg/carto.net/window.svg
media/video-poster-blocked-by-willsendrequest.html
svg/custom/use-css-events.svg
svg/custom/struct-use-09-b.svg
svg/custom/relative-sized-use-on-symbol.xhtml
svg/custom/relative-sized-use-without-attributes-on-symbol.xhtml
svg/custom/relative-sized-shadow-tree-content-with-symbol.xhtml
http/tests/inspector/resource-tree/resource-tree-document-url.html
svg/custom/embedding-external-svgs.xhtml

[Nikolas Zimmermann]

(In reply to comment #1)
> Created an attachment (id=119819) [details]
> Patch
> 
> With this patch, some test cases need to be re-baselined. I'll grab the updated test results after the patch in.

I'd like to see the results before giving r+ - can you upload them?

[Leo Yang]

(In reply to comment #3)
> (In reply to comment #1)
> > Created an attachment (id=119819) [details] [details]
> > Patch
> > 
> > With this patch, some test cases need to be re-baselined. I'll grab the updated test results after the patch in.
> 
> I'd like to see the results before giving r+ - can you upload them?

OK, will update the patch.

[Leo Yang]

Created attachment 120537 [details]
Patch v2

Also adding re-baselined test results for mac and qt platform.

[WebKit Review Bot]

Comment on attachment 120537 [details]
Patch v2

Attachment 120537 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11040104

New failing tests:
http/tests/inspector-enabled/dedicated-workers-list.html
svg/carto.net/slider.svg
svg/custom/use-clipped-hit.svg
svg/carto.net/window.svg
svg/custom/use-modify-target-symbol.svg
svg/custom/use-css-events.svg
svg/custom/struct-use-09-b.svg
svg/custom/relative-sized-use-on-symbol.xhtml
css2.1/20110323/abspos-containing-block-initial-004b.htm
css2.1/20110323/abspos-containing-block-initial-004d.htm
svg/custom/relative-sized-shadow-tree-content-with-symbol.xhtml
svg/custom/relative-sized-use-without-attributes-on-symbol.xhtml
svg/custom/embedding-external-svgs.xhtml

[Dirk Schulze]

Comment on attachment 120537 [details]
Patch v2

Makes sense for me. I assume that some chromium results need to get updated. Sadly there is a server error and I can't see which tests. Please fix that before landing. r=me

[Leo Yang]

(In reply to comment #7)
> (From update of attachment 120537 [details])
> Makes sense for me. I assume that some chromium results need to get updated. Sadly there is a server error and I can't see which tests. Please fix that before landing. r=me

Thanks for your review. I'm dealing with some internal higher priority things. Once I finish them, will fix tests and land this patch.

[Rob Buis]

Hi Leo,

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 120537 [details] [details])
> > Makes sense for me. I assume that some chromium results need to get updated. Sadly there is a server error and I can't see which tests. Please fix that before landing. r=me
> 
> Thanks for your review. I'm dealing with some internal higher priority things. Once I finish them, will fix tests and land this patch.

I am looking at fixing this bug right now as it overlaps with bug 82775, are you ok with that?
Cheers,

Rob.

[Leo Yang]

(In reply to comment #9)
> Hi Leo,
> 
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (From update of attachment 120537 [details] [details] [details])
> > > Makes sense for me. I assume that some chromium results need to get updated. Sadly there is a server error and I can't see which tests. Please fix that before landing. r=me
> > 
> > Thanks for your review. I'm dealing with some internal higher priority things. Once I finish them, will fix tests and land this patch.
> 
> I am looking at fixing this bug right now as it overlaps with bug 82775, are you ok with that?
> Cheers,
> 
> Rob.

Hi Rob, go ahead please.

[Rob Buis]

Created attachment 138571 [details]
Patch

[Rob Buis]

*** Bug 82775 has been marked as a duplicate of this bug. ***

[Dirk Schulze]

Comment on attachment 138571 [details]
Patch

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

r=me. With one change.

> Source/WebCore/ChangeLog:13
> +        Create RenderSVGHiddenContainer for each <symbol>, which allows creation of child renderers too. This fixes
> +        both bugs since the bbox can be calculated at all times now and gradients in <symbol> are always created.
> +        This does allocate more memory but in general <symbol> is not used a lot and they tend to be small subtrees.

Can you add Leo as original author please? "Patch by Leo Yang". Thanks.

[Dirk Schulze]

Comment on attachment 138571 [details]
Patch

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

r=me. With one change.

> Source/WebCore/ChangeLog:13
> +        Create RenderSVGHiddenContainer for each <symbol>, which allows creation of child renderers too. This fixes
> +        both bugs since the bbox can be calculated at all times now and gradients in <symbol> are always created.
> +        This does allocate more memory but in general <symbol> is not used a lot and they tend to be small subtrees.

Can you add Leo as original author please? "Patch by Leo Yang". Thanks.

[Stephen Chenney]

Comment on attachment 138571 [details]
Patch

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

> LayoutTests/ChangeLog:13
> +

Please create a new bug to rebaseline the Chromium expectations, and make me the owner. Then, add all of the re-baselined tests to platform/chromium/test_expectations.txt with the new bug number in the SVG section (search for SVG caps). Submit that change to chromium test expectations along with this patch. Help out our sherrifs.

[Rob Buis]

(In reply to comment #15)
> (From update of attachment 138571 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138571&action=review
> 
> > LayoutTests/ChangeLog:13
> > +
> 
> Please create a new bug to rebaseline the Chromium expectations, and make me the owner. Then, add all of the re-baselined tests to platform/chromium/test_expectations.txt with the new bug number in the SVG section (search for SVG caps). Submit that change to chromium test expectations along with this patch. Help out our sherrifs.

Sure I created bug 84731 for this. Will try to land the patch soon and then will update test_expectations.txt.
Cheers,

Rob.

[Rob Buis]

Landed in r115085.

[Leo Yang]

Thank all of you for the help.