Skip to content

Commit

Permalink
Auto merge of #16614 - BorisChiou:stylo/transition/transition_propert…
Browse files Browse the repository at this point in the history 
…y, r=emilio

stylo: Bug 1357357 - Make the parser of transition-property match the spec.

These are interdependent patches of Bug 1357357. We add one more arm, TransitionProperty::Unsupported, which stores the string of non-animatable, custom, or unrecognized property, so we can parse these kinds of properties and serialize them correctly. This is necessary because we need to start transitions even though some transition-properties are non-animatable, custom, or unrecognized.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix [Bug 1357357](https://bugzilla.mozilla.org/show_bug.cgi?id=1357357).
- [X] There are tests for these changes.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16614)
<!-- Reviewable:end -->
  • Loading branch information
bors-servo committed Apr 26, 2017
2 parents 4e70e10 + 02fc178 commit 8f1356d
Show file tree
Hide file tree
Showing 13 changed files with 139 additions and 56 deletions.
18 changes: 11 additions & 7 deletions components/style/animation.rs
View file
Expand Up @@ -272,9 +272,13 @@ impl PropertyAnimation {
let timing_function = box_style.transition_timing_function_mod(transition_index);
let duration = box_style.transition_duration_mod(transition_index);

if let TransitionProperty::Unsupported(_) = transition_property {
return result
}

if transition_property.is_shorthand() {
return transition_property.longhands().iter().filter_map(|transition_property| {
PropertyAnimation::from_transition_property(*transition_property,
PropertyAnimation::from_transition_property(transition_property,
timing_function,
duration,
old_style,
Expand All @@ -284,7 +288,7 @@ impl PropertyAnimation {

if transition_property != TransitionProperty::All {
if let Some(property_animation) =
PropertyAnimation::from_transition_property(transition_property,
PropertyAnimation::from_transition_property(&transition_property,
timing_function,
duration,
old_style,
Expand All @@ -296,7 +300,7 @@ impl PropertyAnimation {

TransitionProperty::each(|transition_property| {
if let Some(property_animation) =
PropertyAnimation::from_transition_property(transition_property,
PropertyAnimation::from_transition_property(&transition_property,
timing_function,
duration,
old_style,
Expand All @@ -308,15 +312,15 @@ impl PropertyAnimation {
result
}

fn from_transition_property(transition_property: TransitionProperty,
fn from_transition_property(transition_property: &TransitionProperty,
timing_function: TransitionTimingFunction,
duration: Time,
old_style: &ComputedValues,
new_style: &ComputedValues)
-> Option<PropertyAnimation> {
debug_assert!(!transition_property.is_shorthand() &&
transition_property != TransitionProperty::All);
let animated_property = AnimatedProperty::from_transition_property(&transition_property,
transition_property != &TransitionProperty::All);
let animated_property = AnimatedProperty::from_transition_property(transition_property,
old_style,
new_style);

Expand Down Expand Up @@ -702,7 +706,7 @@ pub fn update_style_for_animation(context: &SharedStyleContext,
for transition_property in &animation.properties_changed {
debug!("update_style_for_animation: scanning prop {:?} for animation \"{}\"",
transition_property, name);
match PropertyAnimation::from_transition_property(*transition_property,
match PropertyAnimation::from_transition_property(transition_property,
timing_function,
Time::from_seconds(relative_duration as f32),
&from_style,
Expand Down
1 change: 1 addition & 0 deletions components/style/build_gecko.rs
View file
Expand Up @@ -659,6 +659,7 @@ mod bindings {
"StyleBasicShape",
"StyleBasicShapeType",
"StyleShapeSource",
"StyleTransition",
"nsCSSFontFaceRule",
"nsCSSKeyword",
"nsCSSPropertyID",
Expand Down
2 changes: 1 addition & 1 deletion components/style/dom.rs
View file
Expand Up @@ -520,7 +520,7 @@ pub trait TElement : Eq + PartialEq + Debug + Hash + Sized + Copy + Clone +
/// Returns true if we need to update transitions for the specified property on this element.
#[cfg(feature = "gecko")]
fn needs_transitions_update_per_property(&self,
property: TransitionProperty,
property: &TransitionProperty,
combined_duration: f32,
before_change_style: &Arc<ComputedValues>,
after_change_style: &Arc<ComputedValues>,
Expand Down
20 changes: 11 additions & 9 deletions components/style/gecko/wrapper.rs
View file
Expand Up @@ -811,7 +811,7 @@ impl<'le> TElement for GeckoElement<'le> {
continue;
}

let mut property_check_helper = |property: TransitionProperty| -> bool {
let mut property_check_helper = |property: &TransitionProperty| -> bool {
if self.needs_transitions_update_per_property(property,
combined_duration,
before_change_style,
Expand All @@ -821,7 +821,9 @@ impl<'le> TElement for GeckoElement<'le> {
}

if let Some(set) = transitions_to_keep.as_mut() {
set.insert(property);
// The TransitionProperty here must be animatable, so cloning it is cheap
// because it is an integer-like enum.
set.insert(property.clone());
}
false
};
Expand All @@ -835,12 +837,12 @@ impl<'le> TElement for GeckoElement<'le> {
});
if is_shorthand {
let shorthand: TransitionProperty = property.into();
if shorthand.longhands().iter().any(|&p| property_check_helper(p)) {
if shorthand.longhands().iter().any(|p| property_check_helper(p)) {
return true;
}
} else {
if animated_properties::nscsspropertyid_is_animatable(property) &&
property_check_helper(property.into()) {
property_check_helper(&property.into()) {
return true;
}
}
Expand All @@ -855,7 +857,7 @@ impl<'le> TElement for GeckoElement<'le> {
}

fn needs_transitions_update_per_property(&self,
property: TransitionProperty,
property: &TransitionProperty,
combined_duration: f32,
before_change_style: &Arc<ComputedValues>,
after_change_style: &Arc<ComputedValues>,
Expand All @@ -869,17 +871,17 @@ impl<'le> TElement for GeckoElement<'le> {
return false;
}

if existing_transitions.contains_key(&property) {
if existing_transitions.contains_key(property) {
// If there is an existing transition, update only if the end value differs.
// If the end value has not changed, we should leave the currently running
// transition as-is since we don't want to interrupt its timing function.
let after_value =
Arc::new(AnimationValue::from_computed_values(&property, after_change_style));
return existing_transitions.get(&property).unwrap() != &after_value;
Arc::new(AnimationValue::from_computed_values(property, after_change_style));
return existing_transitions.get(property).unwrap() != &after_value;
}

combined_duration > 0.0f32 &&
AnimatedProperty::from_transition_property(&property,
AnimatedProperty::from_transition_property(property,
before_change_style,
after_change_style).does_animate()
}
Expand Down
6 changes: 6 additions & 0 deletions components/style/gecko_bindings/bindings.rs
View file
Expand Up @@ -39,6 +39,7 @@ use gecko_bindings::structs::SheetParsingMode;
use gecko_bindings::structs::StyleBasicShape;
use gecko_bindings::structs::StyleBasicShapeType;
use gecko_bindings::structs::StyleShapeSource;
use gecko_bindings::structs::StyleTransition;
use gecko_bindings::structs::nsCSSFontFaceRule;
use gecko_bindings::structs::nsCSSKeyword;
use gecko_bindings::structs::nsCSSPropertyID;
Expand Down Expand Up @@ -692,6 +693,11 @@ extern "C" {
aProperty: nsCSSPropertyID)
-> RawServoAnimationValueBorrowedOrNull;
}
extern "C" {
pub fn Gecko_StyleTransition_SetUnsupportedProperty(aTransition:
*mut StyleTransition,
aAtom: *mut nsIAtom);
}
extern "C" {
pub fn Gecko_Atomize(aString: *const ::std::os::raw::c_char, aLength: u32)
-> *mut nsIAtom;
Expand Down
2 changes: 1 addition & 1 deletion components/style/keyframes.rs
View file
Expand Up @@ -259,8 +259,8 @@ fn get_animated_properties(keyframes: &[Arc<Locked<Keyframe>>], guard: &SharedRw

if let Some(property) = TransitionProperty::from_declaration(declaration) {
if !seen.has_transition_property_bit(&property) {
ret.push(property);
seen.set_transition_property_bit(&property);
ret.push(property);
}
}
}
Expand Down
34 changes: 32 additions & 2 deletions components/style/properties/gecko.mako.rs
View file
Expand Up @@ -33,6 +33,7 @@ use gecko_bindings::bindings::Gecko_FontFamilyList_AppendNamed;
use gecko_bindings::bindings::Gecko_FontFamilyList_Clear;
use gecko_bindings::bindings::Gecko_SetCursorArrayLength;
use gecko_bindings::bindings::Gecko_SetCursorImage;
use gecko_bindings::bindings::Gecko_StyleTransition_SetUnsupportedProperty;
use gecko_bindings::bindings::Gecko_NewCSSShadowArray;
use gecko_bindings::bindings::Gecko_nsStyleFont_SetLang;
use gecko_bindings::bindings::Gecko_nsStyleFont_CopyLangFrom;
Expand All @@ -53,6 +54,7 @@ use gecko::values::convert_rgba_to_nscolor;
use gecko::values::GeckoStyleCoordConvertible;
use gecko::values::round_border_to_device_pixels;
use logical_geometry::WritingMode;
use properties::animated_properties::TransitionProperty;
use properties::longhands;
use properties::{Importance, LonghandId};
use properties::{PropertyDeclaration, PropertyDeclarationBlock, PropertyDeclarationId};
Expand Down Expand Up @@ -2149,7 +2151,12 @@ fn static_assert() {
unsafe { self.gecko.mTransitions.ensure_len(v.0.len()) };
self.gecko.mTransitionPropertyCount = v.0.len() as u32;
for (servo, gecko) in v.0.into_iter().zip(self.gecko.mTransitions.iter_mut()) {
gecko.mProperty = servo.into();
match servo {
TransitionProperty::Unsupported(ref atom) => unsafe {
Gecko_StyleTransition_SetUnsupportedProperty(gecko, atom.as_ptr())
},
_ => gecko.mProperty = (&servo).into(),
}
}
} else {
// In gecko |none| is represented by eCSSPropertyExtra_no_properties.
Expand All @@ -2172,21 +2179,44 @@ fn static_assert() {

pub fn transition_property_at(&self, index: usize)
-> longhands::transition_property::computed_value::SingleComputedValue {
self.gecko.mTransitions[index].mProperty.into()
use gecko_bindings::structs::nsCSSPropertyID::eCSSPropertyExtra_no_properties;
use gecko_bindings::structs::nsCSSPropertyID::eCSSPropertyExtra_variable;
use gecko_bindings::structs::nsCSSPropertyID::eCSSProperty_UNKNOWN;

let property = self.gecko.mTransitions[index].mProperty;
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 {
// Actually, we don't expect TransitionProperty::Unsupported also represents "none",
// but if the caller wants to convert it, it is fine. Please use it carefully.
TransitionProperty::Unsupported(atom!("none"))
} else {
property.into()
}
}

pub fn transition_nscsspropertyid_at(&self, index: usize) -> nsCSSPropertyID {
self.gecko.mTransitions[index].mProperty
}

pub fn copy_transition_property_from(&mut self, other: &Self) {
use gecko_bindings::structs::nsCSSPropertyID::eCSSPropertyExtra_variable;
use gecko_bindings::structs::nsCSSPropertyID::eCSSProperty_UNKNOWN;
unsafe { self.gecko.mTransitions.ensure_len(other.gecko.mTransitions.len()) };

let count = other.gecko.mTransitionPropertyCount;
self.gecko.mTransitionPropertyCount = count;

for (index, transition) in self.gecko.mTransitions.iter_mut().enumerate().take(count as usize) {
transition.mProperty = other.gecko.mTransitions[index].mProperty;
if transition.mProperty == eCSSProperty_UNKNOWN ||
transition.mProperty == eCSSPropertyExtra_variable {
let atom = other.gecko.mTransitions[index].mUnknownProperty.raw();
debug_assert!(!atom.is_null());
unsafe { Gecko_StyleTransition_SetUnsupportedProperty(transition, atom) };
}
}
}
${impl_transition_count('property', 'Property')}
Expand Down

0 comments on commit 8f1356d

Please sign in to comment.