Closed Bug 730658 Opened 8 years ago Closed 8 years ago

use element does not display properly when animated with set

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: longsonr, Assigned: longsonr)

References

(Depends on 1 open bug, )

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
The large shape in the URL should appear and disappear but once it has disappeared it never comes back.
Attached patch patch with test (obsolete) — Splinter Review
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.
Assignee: nobody → longsonr
Attachment #600751 - Flags: review?(dholbert)
Comment on attachment 600751 [details] [diff] [review]
patch with test

Seems to have some reftest failures on Try. Investigating...
Attachment #600751 - Flags: review?(dholbert)
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/longsonr@gmail.com-b03bee233e2b
Attached patch updated patch to pass try (obsolete) — Splinter Review
Changed logic on outer svg elements, fixed dodgy reftest and added a new one to catch what the fixed reftest discovered.
Attachment #600751 - Attachment is obsolete: true
Attachment #600787 - Flags: review?(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.
(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.
I'll add the IsSVG checks as there's no harm in them.
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!
Attachment #600787 - Flags: review?(dholbert) → review+
> It looks like this chunk is here
(s/chunk/code-removal/)
Summary: Changing width/height on use misses updates → Use does not display properly when animated with set
Summary: Use does not display properly when animated with set → use element does not display properly when animated with set
Flags: in-testsuite+
Target Milestone: --- → mozilla13
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.
Target Milestone: mozilla13 → ---
Flags: in-testsuite+ → in-testsuite?
Attached patch updatedSplinter Review
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.
Attachment #600787 - Attachment is obsolete: true
Attachment #601613 - Flags: review?(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!
Attachment #601613 - Flags: review?(dholbert) → review+
(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!
Ah, makes sense. Thanks!
Flags: in-testsuite? → in-testsuite+
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/42cf37973665
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 734920
You need to log in before you can comment on or make changes to this bug.