Closed Bug 556441 Opened 14 years ago Closed 10 years ago

SVG/SMIL: Interpolation fails for "stroke-width" when animating between unitless values & values with units

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: dholbert, Assigned: longsonr, Mentored)

References

Details

(Whiteboard: [lang=c++])

Attachments

(2 files, 1 obsolete file)

Attached image testcase 1
See attached testcase.  Ideally, all four rects in the grid should animate smoothly, but currently, just the upper-left and lower-right entries do.

This is because nsStyleAnimation::ComputeValue produces Values with type eUnit_Coord when given px-values, vs. type eUnit_Float for unitless values*.  And it can't interpolate (or add, or do distance-computation) between values of different types.

*NOTE: One unimportant exception to the above rule -- the special value "0" proces a Value of type eUnit_Coord, even though it's unitless, since unitless-0 is a valid length.  (Not particularly important, just clarifying for completeness.)
Attachment #436388 - Attachment is patch: false
Attachment #436388 - Attachment mime type: text/plain → image/svg+xml
Presumably this requires adding a property flag to the CSS properties where numbers mean pixels, and then making nsStyleAnimation treat the two the same for only those properties (I'd suggest converting numbers to pixels in that case).
(The relevant code is in layout/style/nsCSSProps.h , layout/style/nsCSSPropList.h , layout/style/nsStyleAnimation.cpp .)
Whiteboard: [mentor=dbaron]
Whiteboard: [mentor=dbaron] → [mentor=dbaron][lang=c++]
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → maxli
Attachment #8351236 - Flags: review?(dbaron)
Shouldn't the flag also be set for stoke-dashoffset, and word-spacing?
Comment on attachment 8351236 [details] [diff] [review]
Patch

The following properties should have CSS_PROPERTY_NUMBERS_ARE_PIXELS in
addition to stroke-width:
  stroke-dashoffset
  stroke-dasharray

(I don't see why comment 4 suggests word-spacing, though.)

nsStyleAnimation.cpp:

  I think the approach you've taken here is less than ideal.  I think
  it would be better to ensure that nsStyleAnimation::Value always
  normalizes to either eUnit_Coord or eUnit_Float when numbers are
  pixels.  In particular, you can do this in ExtractComputedValue.
  (Note that ExtractComputedValue *already* handles stroke-dasharray in
  the eStyleAnimType_Custom case; stroke-width and stroke-dashoffset
  would need to be handled in the eStyleAnimType_Coord case.  Given the
  comment in that code, it's probably better to handle stroke-width and
  stroke-dashoffset by normalizing to eUnit_Float as well, rather than
  my original suggestion of normalizing to eUnit_Coord.)

  So I think you can get rid of all the changes you currently have in
  nsStyleAnimation.cpp and replace them with a change to the
  eStyleAnimType_Coord case in ExtractComputedValue (specific to the
  flag being set).

Then you also should have an automated test (preferably a testharness.js
test running as a mochitest) that fails without the patch and passes
with it; ideally testing all of stroke-width, stroke-dasharray, and
stroke-dashoffset.


This is good progress here, but I'd like to see another revision of the
patch.

And sorry for the (holiday-related) delay getting back to you.
Attachment #8351236 - Flags: review?(dbaron) → review-
Mentor: dbaron
Whiteboard: [mentor=dbaron][lang=c++] → [lang=c++]
Attached patch interpolate.txtSplinter Review
Been nothing happening in 5 months so I'll take a crack at this. Like so...
Assignee: maxli → longsonr
Attachment #8351236 - Attachment is obsolete: true
Attachment #8447683 - Flags: review?(dbaron)
Comment on attachment 8447683 [details] [diff] [review]
interpolate.txt

You'll have to merge with the renaming of nsStyleAnimation.cpp to StyleAnimationValue.cpp, but that should be straightforward.

>-    case eStyleAnimType_Coord:
>-      return StyleCoordToValue(*static_cast<const nsStyleCoord*>(
>-        StyleDataAtOffset(styleStruct, ssOffset)), aComputedValue);
>+    case eStyleAnimType_Coord: {
>+      const nsStyleCoord& coord = *static_cast<const nsStyleCoord*>(
>+        StyleDataAtOffset(styleStruct, ssOffset));
>+      if (nsCSSProps::PropHasFlags(aProperty, CSS_PROPERTY_NUMBERS_ARE_PIXELS) &&
>+          coord.GetUnit() == eStyleUnit_Coord) {
>+        aComputedValue.SetFloatValue(nsPresContext::
>+          AppUnitsToFloatCSSPixels(coord.GetCoordValue()));
>+        return true;
>+      }
>+      return StyleCoordToValue(coord, aComputedValue);
>+    }


Could you add a comment inside the if, similar to the comment in the eCSSProperty_stroke_dasharray: case (except perhaps beginning with "For SVG properties where number means the same thing as length, we...")

r=dbaron with that, if you've tested that the new automated tests fail without the patch (or at least some of them fail) and pass with it.
Attachment #8447683 - Flags: review?(dbaron) → review+
https://hg.mozilla.org/mozilla-central/rev/2edbce6ee196
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Blocks: 1218257
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: