730658 - use element does not display properly when animated with set

Status: RESOLVED FIXED

(Core :: SVG, defect)

Comment 1

[Robert Longson [:longsonr]]

The large shape in the URL should appear and disappear but once it has disappeared it never comes back.

Comment 2

[Robert Longson [:longsonr]]

Attachment: patch with test

The ClearAnimValue changes are because layout is notified of the length change with the mIsAnimated boolean set to true (this is set to false after the notification)

The HasValidDimensions methods are subtly different in many cases as some attributes default to 100% i.e. valid and others default to invalid if not present. We'll be able to use the HasValidDimensions method to fix other bugs e.g. foreignObject not working in patterns later.

We then pick up the valid/invalid transitions not by recloning but by not painting if we're invalid. This is much faster.

Comment 3

[Robert Longson [:longsonr]]

Comment on attachment 600751 [details] [diff] [review]
patch with test

Seems to have some reftest failures on Try. Investigating...

Comment 4

[Mozilla RelEng Bot]

Try run for b03bee233e2b is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b03bee233e2b
Results (out of 81 total builds):
    exception: 1
    success: 59
    warnings: 21
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b03bee233e2b

Comment 5

[Robert Longson [:longsonr]]

Attachment: updated patch to pass try

Changed logic on outer svg elements, fixed dodgy reftest and added a new one to catch what the fixed reftest discovered.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Robert Longson from comment #2)
> The ClearAnimValue changes are because layout is notified of the length
> change with the mIsAnimated boolean set to true (this is set to false after
> the notification)

I don't yet understand the significance of this.  I think what we do right now is:
 - Set animVal = baseVal
 - (That will send notifications, which will make things re-read the animated value, which
    happens to match base value)
 - Turn off "mIsAnimated"

Why/where does this fail?


>diff --git a/layout/svg/base/src/nsSVGUtils.cpp b/layout/svg/base/src/nsSVGUtils.cpp
>@@ -996,16 +996,22 @@ nsSVGUtils::PaintFrameWithEffects(nsSVGR
>   nsISVGChildFrame *svgChildFrame = do_QueryFrame(aFrame);
>   if (!svgChildFrame)
>     return;
[...]
>+  nsIContent* content = aFrame->GetContent();
>+  if (content->IsElement() &&
>+      !static_cast<nsSVGElement*>(content)->HasValidDimensions()) {
>+    return;
>+  }

Why do we need the IsElement() check there?

Also: this static_cast'ing scares me a little...  Is it possible that we might be painting some HTML content with SVG effects here?  Maybe not given that we've already passed a do_QueryFrame to nsISVGChildFrame...

In any case, I think we should add an assertion (or an 'if' check), to assert that the static_cast is valid -- perhaps something like:
 NS_ABORT_IF_FALSE(content->IsSVG(), "SVG frame should be backed by SVG content");

>@@ -1388,23 +1394,26 @@ nsSVGUtils::GetBBox(nsIFrame *aFrame, PR
>     if (metrics) {
>       while (aFrame->GetType() != nsGkAtoms::svgTextFrame) {
>         aFrame = aFrame->GetParent();
>       }
>       svg = do_QueryFrame(aFrame);
>     }
>+    nsSVGElement *element = static_cast<nsSVGElement*>(aFrame->GetContent());
>+    if (!element->HasValidDimensions()) {
>+      return bbox;
>+    }

Same for this static_cast.

Comment 7

[Robert Longson [:longsonr]]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> 
> I don't yet understand the significance of this.  I think what we do right
> now is:
>  - Set animVal = baseVal
>  - (That will send notifications, which will make things re-read the
> animated value, which
>     happens to match base value)
>  - Turn off "mIsAnimated"

The old code looks like this...

void
nsSVGLength2::SetAnimValueInSpecifiedUnits(float aValue,
                                           nsSVGElement* aSVGElement)
{
  mAnimVal = aValue;
  mIsAnimated = true;
  aSVGElement->DidAnimateLength(mAttrEnum);
}

void
nsSVGLength2::SMILLength::ClearAnimValue()
{
  if (mVal->mIsAnimated) {
    mVal->SetAnimValueInSpecifiedUnits(mVal->mBaseVal, mSVGElement);
    mVal->mIsAnimated = false;
  }  
}

DidAnimateLength calls AttributeChanged in layout. At the point of that call mIsAnimated is true. It's set to false only when ClearAnimValue returns. So if frame code in AttributeChanged needs to know whether the attribute is animated it gets the wrong information. It's rare to need to know this only filters and use have length attributes that work differently when they are set to when they are not. <use width="0"> for instance should not display even when it points to a <g> element that does not have a width attribute but <use width="x"> displays exactly the same for all x > 0 when it points to a <g>.

> 
> Why/where does this fail?
> 

It fails if you animate a <use> element that has no width/height using <set> to/from 0 as you need to know whether the <use> has an explicitly set width/height.

> 
> Why do we need the IsElement() check there?

It's called on text nodes by nsSVGGlyphFrame. They aren't elements.

> 
> Also: this static_cast'ing scares me a little...  Is it possible that we
> might be painting some HTML content with SVG effects here?  Maybe not given
> that we've already passed a do_QueryFrame to nsISVGChildFrame...

No, painting html content occurs in nsSVGIntegrationUtils::PaintFramesWithEffects

> 
> In any case, I think we should add an assertion (or an 'if' check), to
> assert that the static_cast is valid -- perhaps something like:
>  NS_ABORT_IF_FALSE(content->IsSVG(), "SVG frame should be backed by SVG
> content");

The content must be SVG or the nsISVGChildFrame check would fail. Only SVG frames implement that interface.

> 
> >@@ -1388,23 +1394,26 @@ nsSVGUtils::GetBBox(nsIFrame *aFrame, PR
> >     if (metrics) {
> >       while (aFrame->GetType() != nsGkAtoms::svgTextFrame) {
> >         aFrame = aFrame->GetParent();
> >       }
> >       svg = do_QueryFrame(aFrame);
> >     }
> >+    nsSVGElement *element = static_cast<nsSVGElement*>(aFrame->GetContent());
> >+    if (!element->HasValidDimensions()) {
> >+      return bbox;
> >+    }
> 
> Same for this static_cast.

We have the same nsISVGChildFrame check here already that proves we have an SVG frame and therefore SVG content.

Comment 8

[Robert Longson [:longsonr]]

I'll add the IsSVG checks as there's no harm in them.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 600787 [details] [diff] [review]
updated patch to pass try

(In reply to Robert Longson from comment #7)
> It fails if you animate a <use> element that has no width/height using <set>
> to/from 0 as you need to know whether the <use> has an explicitly set
> width/height.

Ah! Gotcha, so it's because IsExplicitlySet() will still return true after we've cleared the animated value, while we're doing notifications -- that's wrong, yeah.  Good.

> The content must be SVG or the nsISVGChildFrame check would fail. Only SVG
> frames implement that interface.

Cool, that's what it looked like.  Nice to have that explicitly noted/asserted before going ahead with a static_cast, though, both for safety & readability.  (Sounds like you're adding some IsSVG checks per comment 8 -- thanks!)


Also, one more nit:
>+++ b/layout/svg/base/src/nsSVGImageFrame.cpp
>@@ -315,18 +315,16 @@ nsSVGImageFrame::PaintSVG(nsSVGRenderSta
>   imgElem->GetAnimatedLengthValues(&x, &y, &width, &height, nsnull);
>-  if (width <= 0 || height <= 0)
>-    return NS_OK;

It looks like this chunk is here because presumably the new HasValidDimensions check in nsSVGUtils will keep us from getting here unless width & height are positive. I only know that from reading the patch -- it's not clear that they'd be already-bounds-checked from just skimming this function. For readability's sake, could you stick an assertion here, to make that assumption explicit?

r=me with that and IsSVG checks before the new static_casts in nsSVGUtils.  Thanks for fixing this!

Comment 10

[Daniel Holbert [:dholbert]]

> It looks like this chunk is here
(s/chunk/code-removal/)

Comment 11

[Robert Longson [:longsonr]]

pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/67a7d35ac972

Comment 12

[Robert Longson [:longsonr]]

And backed out. https://hg.mozilla.org/integration/mozilla-inbound/rev/372ab918a050

Looks like an intermittent reftest failure in anim-use-length-01.svg. Investigating.

Comment 13

[Robert Longson [:longsonr]]

Attachment: updated

Changes from previous version:

a) added HasValidDimensions implementation for paths that checks that the path is not empty.
b) added code in nsSVGInnerSVGFrame::AttributeChanged to check whether the canvasTM is singular and push though a transform changed notification if it is.

It's b that caught us last time, an existing bug: If you have an inner <svg> element with a zero width/height and you make it non-zero its children won't display properly as it will retain the singular mCanvasTM calculated by the frame when the width/height was zero. You can do this without using <use> and see the bug.

<use> used avoid this by recreating the frames and their cached mCanvasTM members when we changed from valid to invalid.

With this patch though, we keep the frames and just choose not to paint them when they have a zero width/height. This is much faster as we're doing less work but we need to make sure if we've cached something that the cache represents reality.

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 601613 [details] [diff] [review]
updated

The changes in comment 13 sound good.

Two other things though:

(1) From diffing your landed patch vs. this patch, it looks like some reftest changes were dropped. In particular, the following reftest changes are missing in this patch-version w.r.t. the previous one:
  - new file reftests/svg/rect-04.svg is now missing
  - a tweak to reftests/svg/smil/anim-feImage-preserveAspectRatio-01.svg is missing
  - new file reftests/svg/smil/anim-use-length-02.svg is now missing

I'm guessing this was unintentional, because the patch still adds lines for the new files in reftest.list.


(2) A nit on one thing you addressed from my previous review feedback:

>diff --git a/layout/svg/base/src/nsSVGImageFrame.cpp b/layout/svg/base/src/nsSVGImageFrame.cpp
>--- a/layout/svg/base/src/nsSVGImageFrame.cpp
>+++ b/layout/svg/base/src/nsSVGImageFrame.cpp
>@@ -315,18 +315,18 @@ nsSVGImageFrame::PaintSVG(nsSVGRenderSta
>   nsSVGImageElement *imgElem = static_cast<nsSVGImageElement*>(mContent);
>   imgElem->GetAnimatedLengthValues(&x, &y, &width, &height, nsnull);
>-  if (width <= 0 || height <= 0)
>-    return NS_OK;
>+  NS_ASSERTION(width >= 0 && height >= 0,
>+               "Should only be painting things with valid width/height");

Both checks in this assertion should be ">", not ">=", I think.  (If either width or height were 0, there'd be nothing to paint, and we wouldn't get here.)

r=me with those addressed.  Thanks!

Comment 15

[Robert Longson [:longsonr]]

(In reply to Daniel Holbert [:dholbert] from comment #14)
> 
> I'm guessing this was unintentional, because the patch still adds lines for
> the new files in reftest.list.

I left the reftests landed and just backed out the code changes.

> Both checks in this assertion should be ">", not ">=", I think.  (If either
> width or height were 0, there'd be nothing to paint, and we wouldn't get
> here.)

Done.

> 
> r=me with those addressed.  Thanks!

Comment 16

[Daniel Holbert [:dholbert]]

Ah, makes sense. Thanks!

Comment 17

[Robert Longson [:longsonr]]

pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/42cf37973665

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/42cf37973665