SMIL allows parsing negative values that are normally out-of-range, e.g. negative opacity. This is because a negative value when added to the existing value might produce something in range. e.g. <animate attributeName="opacity" from="1" by="-1" dur="1s"/> In nsSMILCSSValueType.cpp we handle this by manually dropping the "-" sign from the string before parsing it, then making the return computed value negative. Bug 1355348 factors out a method, GetNonNegativePropValue, that does the first part but we can't easily make the computed value negative. We either need to provide a means to negate a Servo computed value, or (and this second option is probably beetter) just a helper on the Servo side that does SMIL-specific string handling. e.g. Servo_ParsePropertyForSMIL which, if need be, strips the "-" sign and then makes the value negative. Alternatively we could define a new computed/animation value type which is an unconstrained opacity value which we then clamp when we apply it. In fact, we'll probably need to do that anyway.

(In reply to Brian Birtles (:birtles) from comment #0) > Alternatively we could define a new computed/animation value type which is > an unconstrained opacity value which we then clamp when we apply it. In > fact, we'll probably need to do that anyway. For this part, in bug 1356941 I am going to implement a mechanism that we can store RGBA values that exceed 255 during interpolation (i.e. inside AnimationValue). I think we can re-use it for this bug.

For reference, jryans added SVG mode for parsing length in bug 1329088.

Comment on attachment 8867050 [details] Bug 1357295 - Allow all numeric values for SMIL. https://reviewboard.mozilla.org/r/138662/#review141896

Comment on attachment 8867050 [details] Bug 1357295 - Allow all numeric values for SMIL. https://reviewboard.mozilla.org/r/138662/#review141898 ::: dom/smil/nsSMILCSSValueType.cpp:481 (Diff revision 1) > // FIXME (bug 1357295): Handle negative values properly > #ifdef DEBUG > { > bool isNegative = false; > Unused << GetNonNegativePropValue(aString, aPropID, isNegative); > if (isNegative) { > NS_WARNING("stylo: Special negative value handling not yet supported" > " (bug 1357295)"); > } > } > #endif // DEBUG > We should drop this code too.

(In reply to Brian Birtles (:birtles) from comment #9) > Comment on attachment 8867050 [details] > Bug 1357295 - Allow all numeric values for SMIL. > > https://reviewboard.mozilla.org/r/138662/#review141898 > > ::: dom/smil/nsSMILCSSValueType.cpp:481 > (Diff revision 1) > > // FIXME (bug 1357295): Handle negative values properly > > #ifdef DEBUG > > { > > bool isNegative = false; > > Unused << GetNonNegativePropValue(aString, aPropID, isNegative); > > if (isNegative) { > > NS_WARNING("stylo: Special negative value handling not yet supported" > > " (bug 1357295)"); > > } > > } > > #endif // DEBUG > > > > We should drop this code too. Thanks!

Comment on attachment 8867047 [details] Bug 1357295 - Rename LengthParsingMode to ParsingMode and LengthParsingMode::SVG to PasingMode::AllowUnitlessLength. https://reviewboard.mozilla.org/r/138656/#review141962

Comment on attachment 8867048 [details] Bug 1357295 - Make ParsingMode bitflags. https://reviewboard.mozilla.org/r/138658/#review141966

Comment on attachment 8867049 [details] Bug 1357295 - Add PARSING_MODE_ALLOW_ALL_NUMERIC_VALUES to force to parse negative values. https://reviewboard.mozilla.org/r/138660/#review141968 ::: layout/style/ServoTypes.h:87 (Diff revision 1) > // In SVG, a coordinate or length value without a unit identifier (e.g., "25") > // is assumed to be in user units (px). > // https://www.w3.org/TR/SVG/coords.html#Units > AllowUnitlessLength = 1 << 0, > + // In SVG, out-of-range values are not treated as an error in parsing. > + // https://www.w3.org/TR/SVG/implnote.html#RangeClamping Is this for all SVG? Or only for SMIL animations? if it's for all SVG, don't we need to update nsSVGElement.cpp too[1]? If we do, that's the only current caller left with LengthParsingMode::SVG, so I wouldn't be so sure we need bitflags for it. If not, then this looks fine. http://searchfox.org/mozilla-central/rev/cd8c561106d804e26bc09389f18f361846d005eb/dom/svg/nsSVGElement.cpp#1240

(In reply to Emilio Cobos Álvarez [:emilio] from comment #14) > Comment on attachment 8867049 [details] > Bug 1357295 - Add PARSING_MODE_ALLOW_ALL_NUMERIC_VALUES to force to parse > negative values. > > https://reviewboard.mozilla.org/r/138660/#review141968 > > ::: layout/style/ServoTypes.h:87 > (Diff revision 1) > > // In SVG, a coordinate or length value without a unit identifier (e.g., "25") > > // is assumed to be in user units (px). > > // https://www.w3.org/TR/SVG/coords.html#Units > > AllowUnitlessLength = 1 << 0, > > + // In SVG, out-of-range values are not treated as an error in parsing. > > + // https://www.w3.org/TR/SVG/implnote.html#RangeClamping > > Is this for all SVG? Or only for SMIL animations? if it's for all SVG, don't > we need to update nsSVGElement.cpp too[1]? > > If we do, that's the only current caller left with LengthParsingMode::SVG, > so I wouldn't be so sure we need bitflags for it. > > If not, then this looks fine. > > http://searchfox.org/mozilla-central/rev/ > cd8c561106d804e26bc09389f18f361846d005eb/dom/svg/nsSVGElement.cpp#1240 According to what I heard from Brian, presentation attribute does not support this negative value. So this part should not be affected by the negative value. Anyway, thanks for paying attention!

Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0daef4212bdf Rename LengthParsingMode to ParsingMode and LengthParsingMode::SVG to PasingMode::AllowUnitlessLength. r=emilio https://hg.mozilla.org/integration/autoland/rev/488ac0d5200c Make ParsingMode bitflags. r=emilio https://hg.mozilla.org/integration/autoland/rev/18ed65b515a4 Add PARSING_MODE_ALLOW_ALL_NUMERIC_VALUES to force to parse negative values. r=emilio https://hg.mozilla.org/integration/autoland/rev/dd580b538f92 Allow all numeric values for SMIL. r=birtles

https://hg.mozilla.org/mozilla-central/rev/0daef4212bdf https://hg.mozilla.org/mozilla-central/rev/488ac0d5200c https://hg.mozilla.org/mozilla-central/rev/18ed65b515a4 https://hg.mozilla.org/mozilla-central/rev/dd580b538f92