Bug 72796 - [SVG] getBBox on a <g> within symbol returns 0 but with defs it works correctly
Summary: [SVG] getBBox on a <g> within symbol returns 0 but with defs it works correctly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Rob Buis
URL:
Keywords:
: 82775 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-11-19 03:32 PST by Raks
Modified: 2012-04-24 19:13 PDT (History)
9 users (show)

See Also:


Attachments
Patch (5.88 KB, patch)
2011-12-18 22:59 PST, Leo Yang
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch v2 (79.71 KB, patch)
2011-12-26 01:13 PST, Leo Yang
no flags Details | Formatted Diff | Diff
Patch (67.38 KB, patch)
2012-04-24 08:39 PDT, Rob Buis
krit: review+
krit: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Raks 2011-11-19 03:32:47 PST
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>
Comment 1 Leo Yang 2011-12-18 22:59:06 PST
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.
Comment 2 WebKit Review Bot 2011-12-20 15:26:34 PST
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
Comment 3 Nikolas Zimmermann 2011-12-21 12:33:54 PST
(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?
Comment 4 Leo Yang 2011-12-21 18:59:12 PST
(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.
Comment 5 Leo Yang 2011-12-26 01:13:22 PST
Created attachment 120537 [details]
Patch v2

Also adding re-baselined test results for mac and qt platform.
Comment 6 WebKit Review Bot 2011-12-26 12:03:29 PST
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
Comment 7 Dirk Schulze 2012-04-19 20:19:54 PDT
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
Comment 8 Leo Yang 2012-04-23 19:44:30 PDT
(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.
Comment 9 Rob Buis 2012-04-23 20:05:06 PDT
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.
Comment 10 Leo Yang 2012-04-23 20:13:43 PDT
(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.
Comment 11 Rob Buis 2012-04-24 08:39:01 PDT
Created attachment 138571 [details]
Patch
Comment 12 Rob Buis 2012-04-24 08:45:52 PDT
*** Bug 82775 has been marked as a duplicate of this bug. ***
Comment 13 Dirk Schulze 2012-04-24 08:47:59 PDT
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.
Comment 14 Dirk Schulze 2012-04-24 08:48:20 PDT
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.
Comment 15 Stephen Chenney 2012-04-24 09:01:17 PDT
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.
Comment 16 Rob Buis 2012-04-24 09:07:54 PDT
(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.
Comment 17 Rob Buis 2012-04-24 11:32:59 PDT
Landed in r115085.
Comment 18 Leo Yang 2012-04-24 19:13:49 PDT
Thank all of you for the help.