Closed Bug 1361639 Opened 3 years ago Closed 2 years ago

getBBox returns wrong x,y for use-element

Categories

(Core :: SVG, defect, P3)

55 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: ulima.ums, Assigned: u459114)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

Attached file minimal_example.htm
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170502030211

Steps to reproduce:

Use attachment or do the following:
Use svg:

<svg id="SvgjsSvg1002" width="1500" height="750" xmlns="http://www.w3.org/2000/svg" version="1.1" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 1500 750">
 <defs id="SvgjsDefs1003"></defs>
 <symbol id="SvgjsSymbol1009">
  <rect id="SvgjsRect1010" width="100" height="100" fill="#cccccc" x="10" y="10"></rect>
 </symbol>
 <use id="SvgjsUse1011" xlink:href="#SvgjsSymbol1009" x="100" y="100"></use>
</svg>

Execute in console: `document.getElementById('SvgjsUse1011').getBBox()`


Actual results:

It returns {x:10,y:10 ...}


Expected results:

It should return {x:110, y:110 ...}

There was a similar bug un chrome which is fixed already:
https://bugs.chromium.org/p/chromium/issues/detail?id=512081

There also is a nice list with test cases in the specs which also cover this usecase:
https://svgwg.org/svg2-draft/single-page.html#coords-BoundingBoxes
Component: Untriaged → SVG
Product: Firefox → Core
This is a breaking change in SVG 2 from SVG 1.1. In the SVG 1.1 version of the specification what we return is correct.
My understanding of a bounding box is, that it fits the shape in current user space.
If I get the bbox of a use element and add it as rectangle to the parent element it should fully enclose the use element because they are in the same user space.

So even according to SVG 1.1 I think that the above should be returned.
Any other interpretation kinda renders the bbox useless (correct me if iam wrong but I cant see a usecase for a use-bbox defined like this).
In SVG 1.1 the <use> x,y became a transform and bounding boxes are returned within the transformed co-ordinate space.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch 1361639.txtSplinter Review
Assignee: nobody → longsonr
Assignee: longsonr → nobody
The patch fixes bounding boxes but breaks clipping, masking and filtering of use elements. https://treeherder.mozilla.org/#/jobs?repo=try&revision=e69fccf119e94f1a96e43a7cef5e10a09c56681c&selectedJob=111401090
Perhaps bug 537623 fixed the wrong extra translation?
Blocks: 537623
Priority: -- → P3
Assignee: nobody → cku
Blocks: svg-enhance
relative bug on chomium:
https://bugs.chromium.org/p/chromium/issues/detail?id=512081
Attachment #8902084 - Attachment is obsolete: true
Attachment #8902231 - Flags: review?(longsonr)
Attachment #8902510 - Flags: review?(longsonr)
Attachment #8902511 - Flags: review?(longsonr)
Comment on attachment 8902510 [details]
Bug 1361639 - Part 2. Add test cases of calling getBBox on use element in test_bbox.xhtml.

https://reviewboard.mozilla.org/r/174088/#review179596

r+ assuming issues are fixed.

::: dom/svg/test/bbox-helper.svg:2
(Diff revision 2)
>  <?xml version="1.0"?>
> -<svg xmlns="http://www.w3.org/2000/svg">
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">

This change is not necessary (see below)

::: dom/svg/test/bbox-helper.svg:38
(Diff revision 2)
>      <circle cx="100" cy="100" r="5"/>
>      <g/>
>      <circle cx="100" cy="100" r="5"/>
>      <g/>
>    </g>
> +  <use x="100" y="100" id="use_v" xlink:href="#v"/>

just use href rather than xlink:href
Attachment #8902510 - Flags: review?(longsonr) → review+
Comment on attachment 8902511 [details]
Bug 1361639 - Part 3. Remove unused EffectOffsets::offsetToBoundingBoxInDevPx.

https://reviewboard.mozilla.org/r/174090/#review179600
Attachment #8902511 - Flags: review?(longsonr) → review+
Comment on attachment 8902231 [details]
Bug 1361639 - Part 1. Depend on input flag, return bbox of an <use> element in different coordinate system.

https://reviewboard.mozilla.org/r/173752/#review179602

r+ assuming nits are fixed.

::: dom/svg/SVGTransformableElement.cpp:192
(Diff revision 5)
>      return nullptr;
>    }
>  
>    if (!NS_SVGNewGetBBoxEnabled()) {
> -    return NS_NewSVGRect(this, ToRect(nsSVGUtils::GetBBox(frame)));
> +    return NS_NewSVGRect(this, ToRect(nsSVGUtils::GetBBox(frame,
> +                                      nsSVGUtils::eBBoxIncludeFillGeometry |

Don't think you need to (or should) pass nsSVGUtils::eBBoxIncludeFillGeometry here.

::: layout/svg/nsSVGUtils.h:397
(Diff revision 5)
>      eBBoxIncludeFillGeometry   = 1 << 1,
>      eBBoxIncludeStroke         = 1 << 2,
>      eBBoxIncludeStrokeGeometry = 1 << 3,
>      eBBoxIncludeMarkers        = 1 << 4,
>      eBBoxIncludeClipped        = 1 << 5,
> +    // This flag takes effect on if getBBox calls on outer-<svg>.

This flag takes effect on outer-<svg> elements.

(Although given the name of the flag I don't think this comment adds much so I'd be tempted not to make this change at all).

::: layout/svg/nsSVGUtils.h:408
(Diff revision 5)
>      eForGetClientRects         = 1 << 7,
>      // If the given frame is an HTML element, only include the region of the
>      // given frame, instead of all continuations of it, while computing bbox if
>      // this flag is set.
>      eIncludeOnlyCurrentFrameForNonSVGElement = 1 << 8,
> +    // This flag takes effect only if getBBox calls on <use>.

This flag is only has an effect when the target is a <use> element.

::: layout/svg/nsSVGUtils.h:411
(Diff revision 5)
>      // this flag is set.
>      eIncludeOnlyCurrentFrameForNonSVGElement = 1 << 8,
> +    // This flag takes effect only if getBBox calls on <use>.
> +    // getBBox returns the bounds of the elements children in user space if
> +    // this flag is set; Otherwise, getBBox returns the union bounds in
> +    // coordinate system formed by <use> element.

the coordinate system formed by the <use> element.
Attachment #8902231 - Flags: review?(longsonr) → review+
Comment on attachment 8902231 [details]
Bug 1361639 - Part 1. Depend on input flag, return bbox of an <use> element in different coordinate system.

https://reviewboard.mozilla.org/r/173752/#review179602

> Don't think you need to (or should) pass nsSVGUtils::eBBoxIncludeFillGeometry here.

nsSVGUtils::GetBBox calls and pass aFlags to SVGGeometryFrame::GetBBoxContribution. In SVGGeometryFrame::GetBBoxContribution, we do need nsSVGUtils::eBBoxIncludeFillGeometry flag to inlcude fill geometry[1].

http://searchfox.org/mozilla-central/source/layout/svg/SVGGeometryFrame.cpp#498
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/24bcdb71be27
Part 1. Depend on input flag, return bbox of an <use> element in different coordinate system. r=longsonr+218550
https://hg.mozilla.org/integration/autoland/rev/14ca951b7d37
Part 2. Add test cases of calling getBBox on use element in test_bbox.xhtml. r=longsonr+218550
https://hg.mozilla.org/integration/autoland/rev/f73863428c5c
Part 3. Remove unused EffectOffsets::offsetToBoundingBoxInDevPx. r=longsonr+218550
Depends on: 1395870
Depends on: 1395873
You need to log in before you can comment on or make changes to this bug.