Boris Chiou [:boris] Assignee
	Description • 8 years ago

According to the spec:
  For the keyword all, or if one of the identifiers listed is a shorthand property, implementations must start transitions for any of its longhand sub-properties that are animatable (or, for all, all animatable properties), using the duration, delay, and timing function at the index corresponding to the shorthand. 

Now, the servo transition_property, i.e. animated_properties::TransitionProperty, fails to parse shorthand properties; In other words, it only parses "all" or longhand properties [1]. We have to fix it.

[1] http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/servo/components/style/properties/helpers/animated_properties.mako.rs#59,69-80

	Boris Chiou [:boris] Assignee
	Comment 1 • 8 years ago

Two intermittent failures in test_transitions.html are caused by this bug [1]. test_transitions.html get the computed style at an  approximate time range of 2s, 4s, 6s, 8s, to check the transitions work properly. However, if one transition property is shorthand, the parser fails, and there is no transition, so the computed style of "margin-left" is always a fixed value. Therefore, we may have UNEXPECTED_PASS/UNEXPECTED_FAIL while checking the fixed computed value with the computed value within an approximate time range of 8s. (We assume the transitions end at 8s.)

[1] http://searchfox.org/mozilla-central/rev/c4fdb67bca7889e59af9dd99c604651a49c4aafa/layout/style/test/test_transitions.html#295-296

Assign to me, and I think I may need to fix this before enabling transitions to avoid the intermittent failures.

	Boris Chiou [:boris] Assignee
	Comment 2 • 8 years ago

Promote to P1. test_transitions.html is disabled temporarily (two shorthand intermittent failures) in bug 1341372 until we fix this bug.

	Hiroyuki Ikezoe (:hiro)
	Comment 6 • 8 years ago
mozreview-review

Comment on attachment 8859117 [details]
Bug 1353628 - Update mochitest expectations.

https://reviewboard.mozilla.org/r/131158/#review133694

	Boris Chiou [:boris] Assignee
	Comment 7 • 8 years ago

Oops, Just notice that if we uses shorthands in Servo, it will try to convert these shorthands into AnimatedProperty, and then panic. I will add an extra patch to avoid this (for Servo). For now, these patches work on stylo.

	Emilio Cobos Álvarez (:emilio)
	Comment 8 • 8 years ago
mozreview-review

Comment on attachment 8859115 [details]
Bug 1353628 - Part 1: Add shorthand properties into TransitionProperty.

https://reviewboard.mozilla.org/r/131154/#review133696

Looks good, but want to take another look after comments are addressed.

Thanks!

::: servo/components/style/properties/helpers/animated_properties.mako.rs:96
(Diff revision 1)
>              % for prop in data.longhands:
>                  % if prop.animatable:
>                      "${prop.name}" => Ok(TransitionProperty::${prop.camel_case}),
>                  % endif
>              % endfor
> +            % for prop in data.shorthands:

I think we should keep the `All` case separate and use `shorthands_except_all()` here, to be consistent.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:185
(Diff revision 1)
>              % for prop in data.longhands:
>                  % if prop.animatable:
>                      TransitionProperty::${prop.camel_case} => dest.write_str("${prop.name}"),
>                  % endif
>              % endfor
> +            % for prop in data.shorthands:

Ditto.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:247
(Diff revision 1)
>                  % if prop.animatable:
>                      TransitionProperty::${prop.camel_case}
>                          => PropertyDeclarationId::Longhand(LonghandId::${prop.camel_case}),
>                  % endif
>              % endfor
> -            TransitionProperty::All => panic!(),
> +            % for prop in data.shorthands:

Ditto, though you can just use a wildcard here:

```
_ => panic!()
```

::: servo/components/style/properties/helpers/animated_properties.mako.rs:346
(Diff revision 1)
>                              old_style.get_${prop.style_struct.ident.strip("_")}().clone_${prop.ident}(),
>                              new_style.get_${prop.style_struct.ident.strip("_")}().clone_${prop.ident}())
>                      }
>                  % endif
>              % endfor
> +            % for prop in data.shorthands:

instead, just use a wildcard:

```
other => panic!("Can't use TransitionProperty::{:?} here", other),
```

::: servo/components/style/properties/helpers/animated_properties.mako.rs:505
(Diff revision 1)
>                          AnimationValue::${prop.camel_case}(
>                              computed_values.get_${prop.style_struct.ident.strip("_")}().clone_${prop.ident}())
>                      }
>                  % endif
>              % endfor
> +            % for prop in data.shorthands:

ditto

::: servo/components/style/properties/properties.mako.rs:260
(Diff revision 1)
>              % for prop in data.longhands:
>                  % if prop.animatable:
>                      TransitionProperty::${prop.camel_case} => self.insert(LonghandId::${prop.camel_case}),
>                  % endif
>              % endfor
> -            TransitionProperty::All => unreachable!("Tried to set TransitionProperty::All in a PropertyBitfield"),
> +            % for prop in data.shorthands:

ditto

::: servo/components/style/properties/properties.mako.rs:276
(Diff revision 1)
>              % for prop in data.longhands:
>                  % if prop.animatable:
>                      TransitionProperty::${prop.camel_case} => self.contains(LonghandId::${prop.camel_case}),
>                  % endif
>              % endfor
> -            TransitionProperty::All => unreachable!("Tried to get TransitionProperty::All in a PropertyBitfield"),
> +            % for prop in data.shorthands:

ditto

	Emilio Cobos Álvarez (:emilio)
	Comment 9 • 8 years ago
mozreview-review

Comment on attachment 8859116 [details]
Bug 1353628 - Part 2: Add unit tests of parsing transition properties.

https://reviewboard.mozilla.org/r/131156/#review133700

	Emilio Cobos Álvarez (:emilio)
	Comment 14 • 8 years ago
mozreview-review

Comment on attachment 8859115 [details]
Bug 1353628 - Part 1: Add shorthand properties into TransitionProperty.

https://reviewboard.mozilla.org/r/131154/#review134246

::: servo/components/style/properties/helpers/animated_properties.mako.rs:66
(Diff revision 2)
> +        ${prop.camel_case},
> +    % endfor
>  }
>  
>  impl TransitionProperty {
> -    /// Iterates over each property that is not `All`.
> +    /// Iterates over each property that is longhand.

nit: "Iterates over each longhand property"?

::: servo/components/style/properties/helpers/animated_properties.mako.rs:249
(Diff revision 2)
>                  % if prop.animatable:
>                      TransitionProperty::${prop.camel_case}
>                          => PropertyDeclarationId::Longhand(LonghandId::${prop.camel_case}),
>                  % endif
>              % endfor
>              TransitionProperty::All => panic!(),

Remove the `All` line, since it's covered by the wildcard. Ideally finding a decent message for this panic would be nice too.

::: servo/components/style/properties/properties.mako.rs:260
(Diff revision 2)
>              % for prop in data.longhands:
>                  % if prop.animatable:
>                      TransitionProperty::${prop.camel_case} => self.insert(LonghandId::${prop.camel_case}),
>                  % endif
>              % endfor
>              TransitionProperty::All => unreachable!("Tried to set TransitionProperty::All in a PropertyBitfield"),

Remove the `All` line.

::: servo/components/style/properties/properties.mako.rs:274
(Diff revision 2)
>              % for prop in data.longhands:
>                  % if prop.animatable:
>                      TransitionProperty::${prop.camel_case} => self.contains(LonghandId::${prop.camel_case}),
>                  % endif
>              % endfor
>              TransitionProperty::All => unreachable!("Tried to get TransitionProperty::All in a PropertyBitfield"),

ditto, this is also covered by the wildcard below.

	Emilio Cobos Álvarez (:emilio)
	Comment 15 • 8 years ago
mozreview-review

Comment on attachment 8859435 [details]
Bug 1353628 - Part 3: Create PropertyAnimation for shorthands.

https://reviewboard.mozilla.org/r/131470/#review134248

	Pulsebot
	Comment 18 • 8 years ago

Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c6798f51e462
Update mochitest expectations. r=hiro

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 19 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/c6798f51e462