Closed Bug 1357295 Opened 3 years ago Closed 3 years ago

stylo: Allow negative CSS property values in SMIL

Categories

(Core :: CSS Parsing and Computation, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: birtles, Assigned: hiro)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

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.
Priority: -- → P2
For reference, jryans added SVG mode for parsing length in bug 1329088.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
See Also: → 1364279
Comment on attachment 8867050 [details]
Bug 1357295 - Allow all numeric values for SMIL.

https://reviewboard.mozilla.org/r/138662/#review141896
Attachment #8867050 - Flags: review?(bbirtles) → review+
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
Attachment #8867047 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8867048 [details]
Bug 1357295 - Make ParsingMode bitflags.

https://reviewboard.mozilla.org/r/138658/#review141966
Attachment #8867048 - Flags: review?(emilio+bugs) → review+
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
Attachment #8867049 - Flags: review?(emilio+bugs) → review+
(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
Depends on: 1369588
You need to log in before you can comment on or make changes to this bug.