stylo: Allow negative CSS property values in SMIL

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: birtles, Assigned: hiro)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Reporter)

Description

6 months ago
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.
(Assignee)

Comment 1

6 months ago
(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
(Assignee)

Comment 2

6 months ago
For reference, jryans added SVG mode for parsing length in bug 1329088.
(Assignee)

Updated

6 months ago
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
(Assignee)

Comment 3

6 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=29192c681e10b41cee07c868a3f534ec352cbfe5
(Assignee)

Updated

5 months ago
See Also: → bug 1364279
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 8

5 months ago
mozreview-review
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+
(Reporter)

Comment 9

5 months ago
mozreview-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.
(Assignee)

Comment 10

5 months ago
(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 hidden (mozreview-request)

Comment 12

5 months ago
mozreview-review
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 13

5 months ago
mozreview-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 14

5 months ago
mozreview-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+
(Assignee)

Comment 15

5 months ago
(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!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 20

5 months ago
https://github.com/servo/servo/pull/16851

Comment 21

5 months ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Reporter)

Updated

5 months ago
Depends on: 1369588
You need to log in before you can comment on or make changes to this bug.