Closed Bug 1357357 Opened 8 years ago Closed 8 years ago

stylo: Make the parser of transition-property match the spec

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: boris, Assigned: boris)

References

Details

Attachments

(3 files, 2 obsolete files)

Bug 1357357 - Part 1: Add one FFI to set unsupported transition property.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1357357 - Part 2: Update mochitest expectations and enable two mochitests.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Servo PR, #16614
41 bytes, text/x-github-pull-request
Details | Review
According to the spec, about transition-property: If one of the identifiers listed is not a recognized property name or is not an animatable property, the implementation _must still start transitions_ on the animatable properties in the list using the duration, delay, and timing function at their respective indices in the lists for transition-duration, transition-delay, and transition-timing-function. In other words, unrecognized or non-animatable properties must be kept in the list to preserve the matching of indices. Now, if we have something like this: transition: 2s margin-left, 4s transition-duration; The parser returns Err, and it is not correct. We should still start a transition for margin-left.
Blocks: stylo, stylo-css-transitions
Priority: -- → P3
This bug also need to store the non-animatable or unrecognized transition-property, so getComputedStyle() returns the correct string of transition-property, so we can also fix some errors in test_transitions_computed_value_combinations.html [1]. [1] http://searchfox.org/mozilla-central/rev/7aa21f3b531ddee90a353215bd86e97d6974e25b/layout/style/test/test_transitions_computed_value_combinations.html#55-58
Assignee: nobody → boris.chiou
Status: NEW → ASSIGNED
Priority: P3 → P1
Comment on attachment 8861331 [details] Bug 1357357 - Part 2: Update mochitest expectations and enable two mochitests. https://reviewboard.mozilla.org/r/133300/#review136120
Attachment #8861331 - Flags: review?(hikezoe) → review+
Comment on attachment 8861328 [details] Bug 1357357 - Part 1: Remove Copy trait from TransitionProperty. https://reviewboard.mozilla.org/r/133294/#review136144 r=me with that, thanks! ::: servo/ports/geckolib/glue.rs:2082 (Diff revision 1) > > for (index, &(ref declaration, _)) in animatable.enumerate() { > unsafe { > let property = TransitionProperty::from_declaration(declaration).unwrap(); > (*keyframe).mPropertyValues.set_len((index + 1) as u32); > - (*keyframe).mPropertyValues[index].mProperty = property.into(); > + (*keyframe).mPropertyValues[index].mProperty = property.clone().into(); Do you need the `clone()` here? Presumably you can change the trait implementation to be something like: ``` impl<'a> From<&'a TransitionProperty> for ... ``` Then remove that clone.
Attachment #8861328 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8861329 [details] Bug 1357357 - Part 1: Add one FFI to set unsupported transition property. https://reviewboard.mozilla.org/r/133296/#review136148 Looks reasonable, but want to take another look once comments are addressed, thanks! ::: layout/style/ServoBindings.cpp:629 (Diff revision 1) > + nsCSSProps::LookupProperty(nsDependentAtomString(aAtom), > + CSSEnabledState::eForAllContent); > + if (id == eCSSProperty_UNKNOWN || id == eCSSPropertyExtra_variable) { > + aTransition->SetUnknownProperty(id, aAtom); > + } else { > + aTransition->SetProperty(id); So, to be clear, when this happens we need to eventually fix it so the property is not unknown on stylo, right? Maybe it's worth to add a warning or something? Or is it fine for non-animatable properties too? ::: layout/style/nsStyleStruct.cpp:3250 (Diff revision 1) > CSSEnabledState::eForAllContent) == > aProperty, > "property and property string should match"); > MOZ_ASSERT(aProperty == eCSSProperty_UNKNOWN || > aProperty == eCSSPropertyExtra_variable, > "should be either unknown or custom property"); This function can now call SetUnkownProperty(aProperty, NS_Atomize(aPropertyString)), and remove the second assertion. ::: servo/components/style/properties/gecko.mako.rs:2150 (Diff revision 1) > + if property == eCSSProperty_UNKNOWN || property == eCSSPropertyExtra_variable { > + let atom = self.gecko.mTransitions[index].mUnknownProperty.raw(); > + debug_assert!(!atom.is_null()); > + TransitionProperty::Unsupported(atom.into()) > + } else if property == eCSSPropertyExtra_no_properties { > + TransitionProperty::Unsupported(Atom::from("none")) nit: use `atom!("none")`, so you get it at compile time instead of going through `NS_Atomize`. ::: servo/components/style/properties/helpers/animated_properties.mako.rs:111 (Diff revision 1) > % endfor > other => { > - println!("[Boris] TransitionProperty::parse [{:?}]",other); > + use std::ascii::AsciiExt; > + use std::borrow::Cow; > + if CSSWideKeyword::from_ident(&Cow::from(other)).is_some() || > + other.eq_ignore_ascii_case("none") { This won't compile with the update of cssparser, so you need to move the `try!(input.expect_ident())` to its own variable, and use it here. Also, it's probably slightly more efficient to add another branch for `"none" => Err(())`, but no big deal with that. ::: servo/components/style/properties/helpers/animated_properties.mako.rs:217 (Diff revision 1) > % endfor > % for prop in data.shorthands_except_all(): > TransitionProperty::${prop.camel_case} => dest.write_str("${prop.name}"), > % endfor > + #[cfg(feature = "gecko")] > + TransitionProperty::Unsupported(ref atom) => atom.with_str(|s| dest.write_str(s)), You need to use `serialize_identifier` here.
Attachment #8861329 - Flags: review?(emilio+bugs)
Comment on attachment 8861330 [details] Bug 1357357 - Part 3: Update unit tests for transition-property. https://reviewboard.mozilla.org/r/133298/#review136158
Attachment #8861330 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8861329 [details] Bug 1357357 - Part 1: Add one FFI to set unsupported transition property. https://reviewboard.mozilla.org/r/133296/#review136148 > So, to be clear, when this happens we need to eventually fix it so the property is not unknown on stylo, right? Maybe it's worth to add a warning or something? Or is it fine for non-animatable properties too? We put not only _non-animatable_ longhand property but also _unknown_ and _custom_ property into TransitionProperty::Unsupported(), so if we want to convert TransitionProperty into nsCSSPropertyID, we need to use the string (which we store in TransitionProperty::Unsupported) to look up its nsCSSPropertyID (including non-animatable property), so we need to handle both cases: 1. Unknown or custom property => (```eCSSPropertyExtra_variable``` or ```eCSSProperty_UNKNOWN```) 2. non-animatable property => (```eCSSProperty_xxxxx```) This is what I want, and I think it is fine.
Comment on attachment 8861329 [details] Bug 1357357 - Part 1: Add one FFI to set unsupported transition property. https://reviewboard.mozilla.org/r/133296/#review136726 Looks great, thanks! :)
Attachment #8861329 - Flags: review?(emilio+bugs) → review+
Attachment #8861328 - Attachment is obsolete: true
Attachment #8861330 - Attachment is obsolete: true
Attached file Servo PR, #16614 —
See Also: → 1328499
Summary: stylo: Need to start a transition even if one of the property is non-animatable or unrecognized property → stylo: Make the parser of transition-property match the spec
Pushed by bchiou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d5c6dd33eede Part 1: Add one FFI to set unsupported transition property. r=emilio https://hg.mozilla.org/integration/autoland/rev/9aae2ba9a06f Part 2: Update mochitest expectations and enable two mochitests. r=hiro
https://hg.mozilla.org/mozilla-central/rev/d5c6dd33eede https://hg.mozilla.org/mozilla-central/rev/9aae2ba9a06f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: