Closed Bug 556404 Opened 10 years ago Closed 10 years ago

SMIL Interpolation doesn't work on mapped attributes when units are left off

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a4

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(3 files, 2 obsolete files)

STR:
 1. Load attached testcase

EXPECTED RESULT:
 All rects should smoothly animate their stroke-width to be larger (i.e. the green border should grow smoothly, for all rects)

ACTUAL RESULT:
 The first row of rects don't interpolate smoothly -- instead, they use discrete-mode animation, snapping back and forth between stroke-width=0 and stroke-width=20.

The only difference between the two rows are the presence vs. absence of the "px" units.  So I'm pretty sure this is because we're barfing on the unitless values when parsing them -- i.e. the CSS parser isn't in SVG mode.

The parsing in this case is done via nsStyleAnimation. The obvious fix would be to add a boolean flag |aUseSVGModeParser| to nsStyleAnimation::ComputeValue (and its helpers StyleWithDeclarationAdded & BuildStyleRule).  nsSMILCSSValueType would pass in PR_TRUE for that flag, and any other callers (for CSS Transitions) would pass in PR_FALSE.
Attached image static testcase with unitless values (obsolete) —
Here's a test-case with no animations, just to demonstrate that static unitless values do work, for both the inline style attribute (the first rect) and the mapped |stroke-width| attribute (the second rect).
Ok, there are actually a few distinct bugs here, and it's not quite as I initially thought it was. Better explanation coming in a few minutes.
A few things:

 (A) The "stroke-width" property is kind of a special case, because the CSS parser already allows unitless values for it. (It's parsed with VARIANT_HLPN, with N = [unitless] numbers).  The currently-posted testcase is actually broken because nsStyleAnimation can't interpolate between [unitless] number values and length values.  In the attached testcase, the special value "0" is treated as a length (eStyleUnit_Coord / eUnit_Coord), as is "20px", but "20" is treated as a floating-point number. (eStyleUnit_Factor / eUnit_Float)  And nsStyleAnimation doesn't know how to interpolate between Coord values and Float values, which is why we fail.  This is a distinct issue from what I described as being "pretty sure" about in comment 0, and it needs a separate bug.

 (B) "font-size" is actually a better example of what I was originally talking about in comment 0.  font-size normally *requires* a unit when specified as CSS (even in the inline style attribute for SVG), but it doesn't require a unit when specified via the mapped 'font-size' attribute.  So, unitless 'font-size' values are supposed to work for attributeType="XML", but NOT for attributeType="auto"/"CSS".
Summary: SMIL Interpolation doesn't work on CSS Properties & mapped attributes when units are left off → SMIL Interpolation doesn't work on mapped attributes when units are left off
Attachment #436332 - Attachment description: testcase 1 → testcase 1 [different bug, ignore for now]
Attachment #436334 - Attachment is obsolete: true
This testcase animates "font-size" with attributeType="CSS".  This renders correctly in today's nightly -- the bottom-right entry (px->px) animates, while none of the rest do (since they use invalid values for the font-size CSS property).
This testcase uses attributeType="XML", and it currently renders the same as testcase 2 (only the bottom-right px->px item animates), which is *incorrect*.  All four items should animate here, since unitless values are allowed for the font-size mapped attribute.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
(In reply to comment #3)
>  (A) The "stroke-width" property is kind of a special case, because the CSS
[...]
> This is a distinct issue from what I described as being "pretty
> sure" about in comment 0, and it needs a separate bug.

Filed bug 556441 on that issue.
Attached patch fix v1Splinter Review
This fix...
 - Adds a boolean flag "aUseSVGMode" to nsStyleAnimation::ComputeValue (and its helpers)
 - Adds the same flag in nsSMILCSSValueType::ValueFromString (and its helper)
 - Adds a ValueFromString implementation in nsSMILCSSMappedAttribute, so it can differ from its inherited method (from parent nsSMILCSSProperty) and pass in PR_TRUE to aUseSVGMode.

This also:
 - Removes two now-obsolete comments about multiple presShells in nsStyleAnimation. (obsolete now that bug 534226 has landed)

Requesting r=dbaron on the nsStyleAnimation changes, and r=roc on the SMIL changes.
Attachment #436492 - Flags: review?(dbaron)
Attachment #436492 - Flags: superreview?(roc)
Attachment #436492 - Flags: superreview?(roc) → review?(roc)
Attachment #436332 - Attachment is obsolete: true
Comment on attachment 436492 [details] [diff] [review]
fix v1

r=dbaron on the nsStyleAnimation changes
Attachment #436492 - Flags: review?(dbaron) → review+
http://hg.mozilla.org/mozilla-central/rev/008e74cf781b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Target Milestone: --- → mozilla1.9.3a4
You need to log in before you can comment on or make changes to this bug.