Closed Bug 1320036 Opened 3 years ago Closed 3 years ago

filter region computed incorrectly when targetting outer <svg> element in an HTML document

Categories

(Core :: SVG, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: heycam, Assigned: u459114)

References

Details

(Whiteboard: [webcompat])

Attachments

(4 files, 1 obsolete file)

Attached file test
From https://github.com/webcompat/web-bugs/issues/3771.

The attached test renders different in Chrome and Firefox.  I think the objectBoundingBox (0%, 0%, 75%, 100%) values that give the filter region are being interpreted differently since we are targetting an outer <svg> element (in an HTML context).

Firefox seems to be doing something based on the bounding box of the content of the <svg>, applying the filter's x/y/width/height attributes to it, then applying them in the HTML document's space relative to the <svg>'s box.  Or something.

In the test, hovering over each of the two subtests makes a second circle display in the bottom right of the <svg>.  You can see how it affects the filter region on the left test.

The presence of the border affects things too, though I'm not sure how.

CJ, I wonder if this is something you have time to look at?
Flags: needinfo?(cku)
Priority: -- → P2
Assignee: nobody → cku
Flags: needinfo?(cku)
http://searchfox.org/mozilla-central/source/layout/svg/nsFilterInstance.cpp#183

We compute bbox by union all visible svg objects(two circle, one is visible, one is display:none which will be discard while computing bbox) inside that <svg> element.
Before display the second circle, the size of bbox is evaluated as (100 * 75%, 100 * 100%) = (75, 100)
The size of bbox we expect is (200 * 75%, 200 * 100%) = (150, 200) 
That's why the circle in the left svg image be clipped.
In SVG 2.0
https://svgwg.org/svg2-draft/single-page.html#coords-BoundingBoxes
bounding box
The bounding box (or "bbox") of an element is the tightest fitting rectangle aligned with the axes of that element's user coordinate system that entirely encloses it and its descendants.

In SVG 1.1
https://svgwg.org/svg2-draft/single-page.html#coords-BoundingBoxes
The bounding box is the tightest fitting rectangle aligned with the axes of the applicable element's user coordinate system that entirely encloses the applicable element and its descendants.
(the term applicable element is the element to which the given effect applies)

In SVG 1.1
https://www.w3.org/TR/SVG11/single-page.html#types-__svg__SVGLocatable__getBBox
SVGRect getBBox()
Returns the tight bounding box in current user space (i.e., after application of the ‘transform’ attribute, if any) 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. 

There are two things here:
1. the definition of bounding box: no matter 1.1 or 2.0, the bounding box of an element contains both the element itself and its children.
2. getBBox: only returns the union of contained graphic elements.

This bug says we should not use the returning value of getBBox as the bounding box of the filtering element.
note: (discussion with heycam)
1. compare the behavior of bounding box computation in a pure svg file on all browsers
2. The computation of bounding box computation on an outer svg element in html matches all other html elements, such as div?
Attachment #8834870 - Flags: review?(cam)
Attachment #8834786 - Flags: review?(cam)
Attachment #8834818 - Flags: review?(cam)
With these patches, our behavior aligns with chrome and edge. Safari has problem on applying SVG filter, so its rendering result is different anyway.
You should get jwatt to review this as I'm not convinced that our implementation is incorrect.

You're applying the filter to an element that has a viewBox. The viewBox transform applies to your children so the co-ordinate system for the element is different to that of its children, hence the mismatch. Other UAs have longstanding bugs here.
Flags: needinfo?(jwatt)
One other thing. Does this patch make our behaviour with regard to outer (or root) svg elements that have a viewBox consistent or inconsistent with our behaviour regarding inner svg elements that have a viewBox. We probably need an inner svg element with a viewBox in a reftest to check this.
Flags: needinfo?(cku)
(In reply to Robert Longson from comment #20)
> One other thing. Does this patch make our behaviour with regard to outer (or
> root) svg elements that have a viewBox consistent or inconsistent with our
> behaviour regarding inner svg elements that have a viewBox. We probably need
> an inner svg element with a viewBox in a reftest to check this.

I do not change the behavior if the target element is an inner svg element. But it's reasonable to add a test case for it.
Flags: needinfo?(cku)
Attachment #8834870 - Flags: review?(cam) → review?(jwatt)
Attachment #8834786 - Flags: review?(cam) → review?(jwatt)
Attachment #8834818 - Flags: review?(cam) → review?(jwatt)
(In reply to Robert Longson from comment #19)
> You're applying the filter to an element that has a viewBox. The viewBox
> transform applies to your children so the co-ordinate system for the element
> is different to that of its children, hence the mismatch. Other UAs have
> longstanding bugs here.
Will look into it.
(In reply to Robert Longson from comment #19)
> You should get jwatt to review this as I'm not convinced that our
> implementation is incorrect.
Yes, we probably should have a conclusion first before moving into review stage.

outer svg element is a very special svg element, since it's the only svg element which has css box layout on it. If we treat it as an SVG element while computing bounding-box of the associated filter, our original behavior is correct; In another way, if we treat it as an HTML element, chrome and edge's behavior is more properly.
See Also: → 1287492
Attachment #8834870 - Attachment is obsolete: true
Attachment #8834870 - Flags: review?(jwatt)
Comment on attachment 8834818 [details]
Bug 1320036 - Part 2. Reftest.

https://reviewboard.mozilla.org/r/110650/#review113726

::: layout/reftests/svg/filters/filter-region-01.html:8
(Diff revision 7)
> +  #filtered {
> +    filter: url(#myFilter);
> +  }
> +
> +  #resources {
> +    visibility: hidden;

Can't this <svg> be put in the <head> to avoid having this style rule?

::: layout/reftests/svg/filters/filter-region-01.html:30
(Diff revision 7)
> +  <rect x="0" y="0" width="100%" height="100%" fill="red"/>
> +</svg>
> +
> +<svg id="filtered" style="position: fixed;" x="0" y ="0" width="200" height="200">
> +  <rect x="0" y="0" width="100" height="100" fill="lime"/>
> +  <rect style="display: none;" x="100" y="100" width="100" height="100" fill="lime"/>

Should this |display: none| rect not be removed?

::: layout/reftests/svg/filters/filter-region-02.html:8
(Diff revision 7)
> +  #filtered {
> +    filter: url(#myFilter);
> +  }
> +
> +  #resources {
> +    visibility: hidden;

Again, put the <svg> in the <head>?

::: layout/reftests/svg/filters/filter-region-02.html:26
(Diff revision 7)
> +
> +<svg style="position: fixed;" x="0" y ="0" width="400" height="200">
> +  <!-- This filtered inner element should be covered by the next outer svg element. -->
> +  <svg id="filtered" style="position: fixed;" x="0" y ="0" width="400" height="200">
> +    <rect x="0" y="0" width="100" height="100" fill="red"/>
> +    <rect style="display: none;" x="100" y="100" width="100" height="100" fill="red"/>

Again, what's the purpose of this |display: none| rect?
Comment on attachment 8834786 [details]
Bug 1320036 - Part 1. Correct objectBoundingBox region of a filter applied to an outer SVG element.

https://reviewboard.mozilla.org/r/110624/#review113742

::: layout/svg/nsSVGUtils.h:416
(Diff revision 9)
>     */
>    static gfxRect GetBBox(nsIFrame *aFrame,
>                           // If the default arg changes, update the handling for
>                           // ObjectBoundingBoxProperty() in the implementation.
> -                         uint32_t aFlags = eBBoxIncludeFillGeometry);
> +                         uint32_t aFlags = eBBoxIncludeFillGeometry,
> +                         bool aTreateOutterSVGElementAsHTMLElement = false);

Why do we need this argument? It looks to me like this should always be true. Does something break if it's always true?

::: layout/svg/nsSVGUtils.cpp:1103
(Diff revision 9)
>    }
>    gfxRect bbox;
>    nsISVGChildFrame *svg = do_QueryFrame(aFrame);
> -  if (svg || aFrame->IsSVGText()) {
> +  bool treatAsHTMLElement =
> +    aTreateOutterSVGElementAsHTMLElement &&
> +    nsLayoutUtils::HasCSSBoxLayout(aFrame);

I'm not sure why we have this HasCSSBoxLayout function? We already have a way of checking this (or the inverse, rather) using |frame->GetStateBits() & NS_FRAME_SVG_LAYOUT|. So I think this should just be:

-  if (svg || aFrame->IsSVGText()) {
+  if ((svg || aFrame->IsSVGText()) &&
+      (aFrame->GetStateBits() & NS_FRAME_SVG_LAYOUT)) {

and we should look at removing HasCSSBoxLayout.
Also note that HasCSSBoxLayout doesn't handle text correctly, unlike NS_FRAME_SVG_LAYOUT.

> Why do we need this argument? It looks to me like this should always
> be true. Does something break if it's always true?

To try and answer that question I pushed:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4a79c77cdae02896b79fff3ff2612bd0c2d3245
Flags: needinfo?(jwatt)
Comment on attachment 8834786 [details]
Bug 1320036 - Part 1. Correct objectBoundingBox region of a filter applied to an outer SVG element.

https://reviewboard.mozilla.org/r/110624/#review113742

> Why do we need this argument? It looks to me like this should always be true. Does something break if it's always true?

While calling from an outer svg::svg element SVGSVGElement::getBBox, should we return the union of contained visible graphic element or border-box of it?
If the answer is the later one, then, yes, we don't need this parameter.
All browser returns (0, 0, 100, 100) which means outer-svg::getBBox still return the tightest fitting rectangle that can contain all visible graphic elements.
spec seems pretty clear...

SVGRect getBBox()
    Returns the tight bounding box in current user space (i.e., after application of the ‘transform’ attribute, if any) on the geometry of all contained graphics elements...
Okay, shame that we need the "bbox" to be different for filters (or probably for everything that's not a JS call, but let's take that to a separate bug) when it comes to outer-<svg>. So can you try this:

Get rid of the bool parameter you've added to GetBBox and instead add an eUseFrameBoundsForOuterSVG value to the BBoxFlags enum with a comment like:

  // Normally a getBBox call on outer-<svg> should only return the
  // bounds of the elements children.  This flag will cause the
  // element's bounds to be returned instead.

Pass this flag to the flag you're passing from the nsFilterInstance constructor.

In nsSVGUtils::GetBBox use something like:

  nsISVGChildFrame* svg = do_QueryFrame(aFrame);
  if (aFrame->GetStateBits() & NS_FRAME_SVG_LAYOUT) ||
      aFrame->IsSVGText() ||
      // if we evaluate the following, |svg| can only be an outer-<svg> or null
      (svg && !(aFlags & eUseFrameBoundsForOuterSVG) {
Blocks: 1345946
Comment on attachment 8834786 [details]
Bug 1320036 - Part 1. Correct objectBoundingBox region of a filter applied to an outer SVG element.

r- for now.
Attachment #8834786 - Flags: review?(jwatt) → review-
Comment on attachment 8834818 [details]
Bug 1320036 - Part 2. Reftest.

https://reviewboard.mozilla.org/r/110650/#review120570

r- until comments addressed
Attachment #8834818 - Flags: review?(jwatt) → review-
Comment on attachment 8834818 [details]
Bug 1320036 - Part 2. Reftest.

https://reviewboard.mozilla.org/r/110650/#review121842

::: layout/reftests/svg/filters/filter-region-01.html:8
(Diff revision 7)
> +  #filtered {
> +    filter: url(#myFilter);
> +  }
> +
> +  #resources {
> +    visibility: hidden;

fixed

::: layout/reftests/svg/filters/filter-region-01.html:30
(Diff revision 7)
> +  <rect x="0" y="0" width="100%" height="100%" fill="red"/>
> +</svg>
> +
> +<svg id="filtered" style="position: fixed;" x="0" y ="0" width="200" height="200">
> +  <rect x="0" y="0" width="100" height="100" fill="lime"/>
> +  <rect style="display: none;" x="100" y="100" width="100" height="100" fill="lime"/>

fixed

::: layout/reftests/svg/filters/filter-region-02.html:8
(Diff revision 7)
> +  #filtered {
> +    filter: url(#myFilter);
> +  }
> +
> +  #resources {
> +    visibility: hidden;

fixed

::: layout/reftests/svg/filters/filter-region-02.html:26
(Diff revision 7)
> +
> +<svg style="position: fixed;" x="0" y ="0" width="400" height="200">
> +  <!-- This filtered inner element should be covered by the next outer svg element. -->
> +  <svg id="filtered" style="position: fixed;" x="0" y ="0" width="400" height="200">
> +    <rect x="0" y="0" width="100" height="100" fill="red"/>
> +    <rect style="display: none;" x="100" y="100" width="100" height="100" fill="red"/>

fixed
Attachment #8834786 - Flags: review- → review?(jwatt)
Attachment #8834818 - Flags: review- → review?(jwatt)
Comment on attachment 8834786 [details]
Bug 1320036 - Part 1. Correct objectBoundingBox region of a filter applied to an outer SVG element.

https://reviewboard.mozilla.org/r/110624/#review122428

::: layout/svg/nsSVGUtils.cpp:1101
(Diff revision 11)
>      aFrame = aFrame->GetParent();
>    }
>    gfxRect bbox;
>    nsISVGChildFrame *svg = do_QueryFrame(aFrame);
> -  if (svg || aFrame->IsSVGText()) {
> +  const bool hasSVGLayout = aFrame->GetStateBits() & NS_FRAME_SVG_LAYOUT;
> +  if ((hasSVGLayout || aFrame->IsSVGText()) ||

The parenthesis around the (hasSVGLayout || aFrame->IsSVGText()) are unnecessary. Please remove them for clarity.

::: layout/svg/nsSVGUtils.cpp:1102
(Diff revision 11)
>    }
>    gfxRect bbox;
>    nsISVGChildFrame *svg = do_QueryFrame(aFrame);
> -  if (svg || aFrame->IsSVGText()) {
> +  const bool hasSVGLayout = aFrame->GetStateBits() & NS_FRAME_SVG_LAYOUT;
> +  if ((hasSVGLayout || aFrame->IsSVGText()) ||
> +      (svg && !(aFlags & eUseFrameBoundsForOuterSVG))) {

Please add the following comment immediately before this line:

// if we evaluate the following, |svg| can only be an outer-<svg> or null

I know creating the 'hasSVGLayout' maybe makes the code a little bit clearer by giving a meaningful name to what it is we checked on the previous line, but people may still assume that outer-<svg> "has SVG layout". So I think this comment is important to make the code clear to anyone not familiar with the subtleties of NS_FRAME_SVG_LAYOUT.
Attachment #8834786 - Flags: review?(jwatt) → review+
Comment on attachment 8834818 [details]
Bug 1320036 - Part 2. Reftest.

https://reviewboard.mozilla.org/r/110650/#review122432

::: layout/reftests/svg/filters/filter-region-01.html:3
(Diff revision 9)
> +<!DOCTYPE html>
> +<body style="background-color: lime;">
> +<svg width="0" height="0">

I still think that resources should go in a <head> block, but I guess it doesn't really matter so feel free to leave this as it is.

::: layout/reftests/svg/filters/filter-region-01.html:13
(Diff revision 9)
> +  </filter>
> +</svg>
> +
> +<!-- This outer svg element should be totally covered by the next filtered outer svg element. -->
> +<svg style="position: fixed;" x="0" y ="0" width="100" height="100">
> +  <rect x="0" y="0" width="100%" height="100%" fill="red"/>

Can you offset the <rect> by {10,10} an increase the size of the <svg> to {120,120}?

::: layout/reftests/svg/filters/filter-region-01.html:17
(Diff revision 9)
> +<svg style="position: fixed;" x="0" y ="0" width="100" height="100">
> +  <rect x="0" y="0" width="100%" height="100%" fill="red"/>
> +</svg>
> +
> +<svg filter="url(#myFilter)" style="position: fixed;" x="0" y ="0" width="200" height="200">
> +  <rect x="0" y="0" width="100" height="100" fill="lime"/>

You'll then need to offset this by {10,10} too.

::: layout/reftests/svg/filters/filter-region-02.html:1
(Diff revision 9)
> +<!DOCTYPE html>

This file is closely linked with the previous file in that one sanity checks the other. Could you name this *-01b.html to indicate that the two are linked?
Attachment #8834818 - Flags: review?(jwatt) → review+
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9b6d00fa6435
Part 1. Correct objectBoundingBox region of a filter applied to an outer SVG element. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/eb2013d935b2
Part 2. Reftest. r=jwatt
https://hg.mozilla.org/mozilla-central/rev/9b6d00fa6435
https://hg.mozilla.org/mozilla-central/rev/eb2013d935b2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1348564
You need to log in before you can comment on or make changes to this bug.