The default bug view has changed. See this FAQ.

SVG SMIL: Don't use HasAttr to test for presence of animated attributes

RESOLVED FIXED in mozilla13

Status

()

Core
SVG
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

Trunk
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

7 years ago
As noted by Robert Longson in bug 606399 comment 1, we shouldn't be relying on HasAttr to test for the presence of animated attributes inside content or layout.

The problem is that whilst an attribute may not be specified in markup or via a DOM call, it can still nevertheless have a value if it is animated. If we rely on HasAttr we'll end up ignoring animated values.

I've done a quick grep of svg/content and it seems we use HasAttr on animated attributes in the following cases:

* <image>: xlink:href (not sure if this is actually a problem--perhaps only if we initially have no attr, then <set> one, and then bind to the tree?)
* <marker>: viewBox
* <path>: pathLength
* <rect>: rx,ry (bug 606399)
* <use>: width, height, and viewBox on target content
* filters: kernelUnitLength, values, targetX, targetY, divisor, bias, limitingConeAngle, xlink:href

In layout it seems we have the following:

* filters: filter effects region (x, y, width, height), filterRes
* gradients: gradientUnits, gradientTransform, spreadMethod
* <svg>: viewBox
* patterns: patternUnits, patternContentUnits, patternTransform, viewBox, preserveAspectRatio, x, y, width, height (bug 544809)

All of these are potential bugs although some are not currently manifest since, for example, we don't animate number lists yet (bug 589439).
Brian, can you take these bugs?

Comment 2

7 years ago
viewBox has an IsValid member so that should be easy to convert. Other attribute types may need changing to add an IsValid member as they don't yet have one.
(Assignee)

Updated

7 years ago
Assignee: nobody → birtles
Status: NEW → ASSIGNED

Comment 3

6 years ago
pathLength is fixed by bug 643419

Updated

6 years ago
Depends on: 643419

Comment 4

6 years ago
652832 removes the <use> HasAttr calls
Depends on: 652832

Comment 5

6 years ago
bug 617623 removes filters and image so we're left with

<marker>: viewBox
gradients
<svg>: viewBox

Updated

6 years ago
Depends on: 617623

Comment 6

6 years ago
bug 687830 fixes marker viewBox. Content is now done, only layout left and that's just

gradients
<svg>: viewBox

Updated

6 years ago
Depends on: 687830

Comment 7

6 years ago
bug 689546 gets rid of the <svg> viewBox so only gradients are left now.
Depends on: 689546
(Assignee)

Comment 8

5 years ago
Created attachment 600328 [details] [diff] [review]
WIP patch v1

Fix use of HasAttr in gradients. I want to have another look over this before I put it up for review.
(Assignee)

Comment 9

5 years ago
Created attachment 601198 [details] [diff] [review]
Patch v1a

This patch removes HasAttr using the approach we applied to patterns.

I may post another patch to fix some const-correctness with the underlying code so we can make all these getters const but I've yet to check if it's going to be worthwhile.
Attachment #601198 - Flags: review?(longsonr)
(Assignee)

Updated

5 years ago
Attachment #600328 - Attachment is obsolete: true

Comment 10

5 years ago
Comment on attachment 601198 [details] [diff] [review]
Patch v1a

>diff --git a/layout/reftests/svg/smil/anim-gradient-attr-presence-01-ref.svg b/layout/reftests/svg/smil/anim-gradient-attr-presence-01-ref.svg
>new file mode 100644
>--- /dev/null
>+++ b/layout/reftests/svg/smil/anim-gradient-attr-presence-01-ref.svg
>@@ -0,0 +1,126 @@

<!--
     Any copyright is dedicated to the Public Domain.
     http://creativecommons.org/publicdomain/zero/1.0/
-->

would be nice on the testcases assuming you're happy to do that.

>+<svg xmlns="http://www.w3.org/2000/svg"
>+  xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 200 900">
>+  <!-- 1. gradientUnits -->
>   *aStopColor   = stopFrame->GetStyleSVGReset()->mStopColor;
>   *aStopOpacity = stopFrame->GetStyleSVGReset()->mStopOpacity;
> }
> 
>+PRUint16
>+nsSVGGradientFrame::GetEnumValue(PRUint32 aIndex, nsIContent *aDefault)
>+{
>+  nsSVGEnum& thisEnum =
>+    static_cast<nsSVGGradientElement *>(mContent)->mEnumAttributes[aIndex];

I think you could use a const reference couldn't you?

>+SVGAnimatedTransformList*
>+nsSVGGradientFrame::GetGradientTransformList(nsIContent* aDefault)

Return a const SVGAnimatedTransformList*

>+nsSVGGradientFrame::GetLinearGradientWithLength(PRUint32 aIndex,
>+  nsSVGLinearGradientElement* aDefault)
> {

>+nsSVGGradientFrame::GetRadialGradientWithLength(PRUint32 aIndex,
>+  nsSVGRadialGradientElement* aDefault)
>+{

Same implementation isn't it? Can't we have GetGradientWithLength in the base class?

>-
>-  if (!(gradient = GetRadialGradientWithAttr(nsGkAtoms::fx, nsnull)))
>+  if (!GetRadialGradientWithLength(nsSVGRadialGradientElement::FX, nsnull))
>     fx = cx;  // if fx isn't set, we must use cx
>   else
>-    fx = GradientLookupAttribute(nsGkAtoms::fx, nsSVGRadialGradientElement::FX, gradient);
>+    fx = GetLengthValue(nsSVGRadialGradientElement::FX);

Better if GetLengthValue took a default so we had something like this...

fx = GetLengthValue(nsSVGRadialGradientElement::FX, cx);

This could be a follow up.

r=longsonr with comments addressed or follow ups raised.
Attachment #601198 - Flags: review?(longsonr) → review+

Comment 11

5 years ago
Try run for 12c124460adc is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=12c124460adc
Results (out of 219 total builds):
    exception: 2
    success: 176
    warnings: 27
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bbirtles@mozilla.com-12c124460adc
(Assignee)

Comment 12

5 years ago
Created attachment 601549 [details] [diff] [review]
Patch v1b, r=longsonr

Thanks for the review Robert!

(In reply to Robert Longson from comment #10)
> <!--
>      Any copyright is dedicated to the Public Domain.
>      http://creativecommons.org/publicdomain/zero/1.0/
> -->
> 
> would be nice on the testcases assuming you're happy to do that.

Fixed.

> >+PRUint16
> >+nsSVGGradientFrame::GetEnumValue(PRUint32 aIndex, nsIContent *aDefault)
> >+{
> >+  nsSVGEnum& thisEnum =
> >+    static_cast<nsSVGGradientElement *>(mContent)->mEnumAttributes[aIndex];
> 
> I think you could use a const reference couldn't you?

Fixed.

> >+SVGAnimatedTransformList*
> >+nsSVGGradientFrame::GetGradientTransformList(nsIContent* aDefault)
> 
> Return a const SVGAnimatedTransformList*

Fixed.

> >+nsSVGGradientFrame::GetLinearGradientWithLength(PRUint32 aIndex,
> >+  nsSVGLinearGradientElement* aDefault)
> > {
> 
> >+nsSVGGradientFrame::GetRadialGradientWithLength(PRUint32 aIndex,
> >+  nsSVGRadialGradientElement* aDefault)
> >+{
> 
> Same implementation isn't it? Can't we have GetGradientWithLength in the
> base class?

It's not quite the same, the last line differs, but I had a look at what we could do to remove redundancy.

The current arrangement allows us to traverse linear and radial gradient frames and return the typed element without any casting of the frames, or checking of type parameters. The only casting of the mContent member happens within the subclasses and so should be safe.

Most of the options for factoring out common code would mean we'd end up adding back some casting or switching based on GetType() both of which I'd rather avoid. I looked at templatising GetGradientWithLength but, of course, that doesn't play well with virtual overrides.

As it stands, it's three lines, two of which are the same, repeated twice. I think it's ok?

> >-  if (!(gradient = GetRadialGradientWithAttr(nsGkAtoms::fx, nsnull)))
> >+  if (!GetRadialGradientWithLength(nsSVGRadialGradientElement::FX, nsnull))
> >     fx = cx;  // if fx isn't set, we must use cx
> >   else
> >-    fx = GradientLookupAttribute(nsGkAtoms::fx, nsSVGRadialGradientElement::FX, gradient);
> >+    fx = GetLengthValue(nsSVGRadialGradientElement::FX);
> 
> Better if GetLengthValue took a default so we had something like this...
> 
> fx = GetLengthValue(nsSVGRadialGradientElement::FX, cx);

Good idea, fixed.

Thanks again Robert!

Just waiting for another pass through the Try server (https://tbpl.mozilla.org/?tree=Try&rev=741fc0125151). The previous run had failures on OSX for the added test case.
Attachment #601198 - Attachment is obsolete: true
Attachment #601549 - Flags: review+

Comment 13

5 years ago
(In reply to Brian Birtles (:birtles) from comment #12)
> 
> As it stands, it's three lines, two of which are the same, repeated twice. I
> think it's ok?

Sure.
(Assignee)

Comment 14

5 years ago
Still can't get the reftest to pass on OSX. I keep getting anti-aliasing differences on the border of the gradientTransform test case. All other platforms seem fine though.

Comment 15

5 years ago
You could mask out the problem areas using extra lines or clip regions.
(Assignee)

Comment 16

5 years ago
I think I've finally appeased try server.

Landed on m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df0d1eb248dd
https://hg.mozilla.org/mozilla-central/rev/df0d1eb248dd
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.