Properly remove transitions when the transition property is removed from the element.

Include/Rocket/Core/Element.h

@@ -649,21 +649,24 @@ private:
 	void DirtyTransformState(bool perspective_changed, bool transform_changed, bool parent_transform_changed);
 	void UpdateTransformState();
 
-	// Start an animation, replacing any existing animations of the same property name. If start_value is null, the element's current value is used.
+	/// Start an animation, replacing any existing animations of the same property name. If start_value is null, the element's current value is used.
 	ElementAnimationList::iterator StartAnimation(const String & property_name, const Property * start_value, int num_iterations, bool alternate_direction, float delay, bool origin_is_animation_property);
 
-	// Add a key to an animation, extending its duration. If target_value is null, the element's current value is used.
+	/// Add a key to an animation, extending its duration. If target_value is null, the element's current value is used.
 	bool AddAnimationKeyTime(const String & property_name, const Property * target_value, float time, Tween tween);
 
 	/// Start a transition of the given property on this element.
 	/// If an animation exists for the property, the call will be ignored. If a transition exists for this property, it will be replaced.
-	/// @return True if the transition was added.
+	/// @return True if the transition was added or replaced.
 	bool StartTransition(const Transition& transition, const Property& start_value, const Property& target_value);
-	void RemoveTransitions();
-	void RemoveTransition(const String& property_name);
 
-	void DirtyAnimation();
+	/// Removes all transitions that are no longer part of the element's 'transition' property.
+	void UpdateTransition();
+
+	/// Starts new animations and removes animations no longer part of the element's 'animation' property.
 	void UpdateAnimation();
+
+	/// Advances the animations (including transitions) forward in time.
 	void AdvanceAnimations();
 
 	// Original tag this element came from.
@@ -758,6 +761,7 @@ private:
 
 	ElementAnimationList animations;
 	bool dirty_animation;
+	bool dirty_transition;
 
 	ElementMeta* element_meta;
 

Source/Core/Element.cpp

@@ -121,7 +121,7 @@ struct alignas(ElementMeta) ElementMetaChunk
 
 /// Constructs a new libRocket element.
 Element::Element(const String& tag) : tag(tag), relative_offset_base(0, 0), relative_offset_position(0, 0), absolute_offset(0, 0), scroll_offset(0, 0), content_offset(0, 0), content_box(0, 0), 
-transform_state(), transform_state_perspective_dirty(true), transform_state_transform_dirty(true), transform_state_parent_transform_dirty(true), dirty_animation(false)
+transform_state(), transform_state_perspective_dirty(true), transform_state_transform_dirty(true), transform_state_parent_transform_dirty(true), dirty_animation(false), dirty_transition(false)
 {
 	ROCKET_ASSERT(tag == ToLower(tag));
 	parent = NULL;
@@ -213,8 +213,8 @@ void Element::Update(float dp_ratio)
 	UpdateStructure();
 
 	style->UpdateDefinition();
-	scroll->Update();
 
+	UpdateTransition();
 	UpdateAnimation();
 	AdvanceAnimations();
 
@@ -228,7 +228,7 @@ void Element::Update(float dp_ratio)
 			document_values = &doc->GetComputedValues();
 
 		// Compute values and clear dirty properties
-		auto dirty_properties = style->ComputeValues(element_meta->computed_values, parent_values, document_values, computed_values_are_default_initialized, dp_ratio);
+		DirtyPropertyList dirty_properties = style->ComputeValues(element_meta->computed_values, parent_values, document_values, computed_values_are_default_initialized, dp_ratio);
 
 		computed_values_are_default_initialized = false;
 
@@ -255,6 +255,8 @@ void Element::Update(float dp_ratio)
 
 	UpdateTransformState();
 
+	scroll->Update();
+
 	for (size_t i = 0; i < children.size(); i++)
 		children[i]->Update(dp_ratio);
 }
@@ -1920,7 +1922,12 @@ void Element::OnPropertyChange(const PropertyNameList& changed_properties)
 	// Check for `animation' changes
 	if (all_dirty || changed_properties.find(ANIMATION) != changed_properties.end())
 	{
-		DirtyAnimation();
+		dirty_animation = true;
+	}
+	// Check for `transition' changes
+	if (all_dirty || changed_properties.find(TRANSITION) != changed_properties.end())
+	{
+		dirty_transition = true;
 	}
 }
 
@@ -2429,44 +2436,58 @@ bool Element::StartTransition(const Transition & transition, const Property& sta
 	return result;
 }
 
-void Element::RemoveTransitions()
+void Element::UpdateTransition()
 {
-	// We only touch the animations that originate from the 'transition' property.
-	auto it_remove = std::remove_if(animations.begin(), animations.end(),
-		[](const ElementAnimation & animation) { return animation.IsTransition(); }
-	);
-
-	// We can decide what to do with ended animations here. Now, we remove the properties modified by the animation.
-	for (auto it = it_remove; it != animations.end(); ++it)
-		RemoveProperty(it->GetPropertyName());
+	if(dirty_transition)
+	{
+		dirty_transition = false;
 
-	animations.erase(it_remove, animations.end());
-}
+		// Remove all transitions that are no longer in our local list
+		const TransitionList& keep_transitions = GetComputedValues().transition;
 
-void Element::RemoveTransition(const String& property_name)
-{
-	// We only touch the animations that originate from the 'transition' property.
-	auto it_remove = std::remove_if(animations.begin(), animations.end(),
-		[&property_name](const ElementAnimation & animation) { return animation.IsTransition() && animation.GetPropertyName() == property_name; }
-	);
+		if (keep_transitions.all)
+			return;
 
-	// We can decide what to do with ended animations here. Now, we remove the properties modified by the animation.
-	for (auto it = it_remove; it != animations.end(); ++it)
-		RemoveProperty(it->GetPropertyName());
+		auto it_remove = animations.end();
 
-	animations.erase(it_remove, animations.end());
-}
+		if (keep_transitions.none)
+		{
+			// All transitions should be removed, but only touch the animations that originate from the 'transition' property.
+			it_remove = std::remove_if(animations.begin(), animations.end(),
+				[](const ElementAnimation& animation) -> bool { return animation.IsTransition(); }
+			);
+		}
+		else
+		{
+			// Only remove the transitions that are not in our keep list.
+			const auto& transitions = keep_transitions.transitions;
+
+			it_remove = std::remove_if(animations.begin(), animations.end(),
+				[&transitions](const ElementAnimation& animation) -> bool {
+					if (!animation.IsTransition())
+						return false;
+					auto it = std::find_if(transitions.begin(), transitions.end(), 
+						[&animation](const Transition& transition) { return animation.GetPropertyName() == transition.name; }
+					);
+					return it == transitions.end();
+				}
+			);
+		}
 
+		// We can decide what to do with ended animations here, just removing them seems to be the CSS approach.
+		for (auto it = it_remove; it != animations.end(); ++it)
+			RemoveProperty(it->GetPropertyName());
 
-void Element::DirtyAnimation()
-{
-	dirty_animation = true;
+		animations.erase(it_remove, animations.end());
+	}
 }
 
 void Element::UpdateAnimation()
 {
 	if (dirty_animation)
 	{
+		dirty_animation = false;
+
 		const AnimationList& animation_list = element_meta->computed_values.animation;
 		bool element_has_animations = (!animation_list.empty() || !animations.empty());
 		StyleSheet* stylesheet = nullptr;
@@ -2480,7 +2501,7 @@ void Element::UpdateAnimation()
 					[](const ElementAnimation & animation) { return animation.GetOrigin() == ElementAnimationOrigin::Animation; }
 				);
 
-				// We can decide what to do with ended animations here. Now, we remove the properties modified by the animation.
+				// We can decide what to do with ended animations here, should be consistent with removal of transitions.
 				for (auto it = it_remove; it != animations.end(); ++it)
 					RemoveProperty(it->GetPropertyName());
 
@@ -2488,9 +2509,9 @@ void Element::UpdateAnimation()
 			}
 
 			// Start animations
-			for (auto& animation : animation_list)
+			for (const auto& animation : animation_list)
 			{
-				Keyframes* keyframes_ptr = stylesheet->GetKeyframes(animation.name);
+				const Keyframes* keyframes_ptr = stylesheet->GetKeyframes(animation.name);
 				if (keyframes_ptr && keyframes_ptr->blocks.size() >= 1 && !animation.paused)
 				{
 					auto& property_names = keyframes_ptr->property_names;
@@ -2519,8 +2540,6 @@ void Element::UpdateAnimation()
 				}
 			}
 		}
-
-		dirty_animation = false;
 	}
 }
 

Source/Core/ElementStyle.cpp

@@ -77,7 +77,7 @@ const ElementDefinition* ElementStyle::GetDefinition()
 
 
 // Returns one of this element's properties.
-const Property* ElementStyle::GetLocalProperty(const String& name, PropertyDictionary* local_properties, ElementDefinition* definition, const PseudoClassList& pseudo_classes)
+const Property* ElementStyle::GetLocalProperty(const String& name, const PropertyDictionary* local_properties, const ElementDefinition* definition, const PseudoClassList& pseudo_classes)
 {
 	// Check for overriding local properties.
 	if (local_properties != NULL)
@@ -95,7 +95,7 @@ const Property* ElementStyle::GetLocalProperty(const String& name, PropertyDicti
 }
 
 // Returns one of this element's properties.
-const Property* ElementStyle::GetProperty(const String& name, Element* element, PropertyDictionary* local_properties, ElementDefinition* definition, const PseudoClassList& pseudo_classes)
+const Property* ElementStyle::GetProperty(const String& name, Element* element, const PropertyDictionary* local_properties, const ElementDefinition* definition, const PseudoClassList& pseudo_classes)
 {
 	const Property* local_property = GetLocalProperty(name, local_properties, definition, pseudo_classes);
 	if (local_property != NULL)
@@ -126,13 +126,15 @@ const Property* ElementStyle::GetProperty(const String& name, Element* element,
 
 // Apply transition to relevant properties if a transition is defined on element.
 // Properties that are part of a transition are removed from the properties list.
-void ElementStyle::TransitionPropertyChanges(Element* element, PropertyNameList& properties, PropertyDictionary* local_properties, ElementDefinition* old_definition, ElementDefinition* new_definition,
+void ElementStyle::TransitionPropertyChanges(Element* element, PropertyNameList& properties, const PropertyDictionary* local_properties, const ElementDefinition* old_definition, const ElementDefinition* new_definition,
 	const PseudoClassList& pseudo_classes_before, const PseudoClassList& pseudo_classes_after)
 {
 	ROCKET_ASSERT(element);
 	if (!old_definition || !new_definition || properties.empty())
 		return;
 
+	// We get the local property instead of the computed value here, because we want to intercept property changes even before the computed values are ready.
+	// Now that we have the concept of computed values, we may want do this operation directly on them instead.
 	if (const Property* transition_property = GetLocalProperty(TRANSITION, local_properties, new_definition, pseudo_classes_after))
 	{
 		auto transition_list = transition_property->Get<TransitionList>();
@@ -148,14 +150,8 @@ void ElementStyle::TransitionPropertyChanges(Element* element, PropertyNameList&
 				return transition_added;
 			};
 
-			// TODO: Right now we are removing all previous transitions. This ensures that previsouly transitioned properties
-			// that are no longer in the transition list are properly removed. However, this resets transitions that are quickly
-			// changed, instead of doing a time-compression of the values. We should only remove those that are no longer part
-			// of our transitions.
-
 			if (transition_list.all)
 			{
-				element->RemoveTransitions();
 				Transition transition = transition_list.transitions[0];
 				for (auto it = properties.begin(); it != properties.end(); )
 				{
@@ -170,7 +166,6 @@ void ElementStyle::TransitionPropertyChanges(Element* element, PropertyNameList&
 			{
 				for (auto& transition : transition_list.transitions)
 				{
-					element->RemoveTransition(transition.name);
 					auto it = properties.find(transition.name);
 					if (it != properties.end())
 					{

Source/Core/ElementStyle.h

@@ -195,9 +195,9 @@ private:
 	// Sets a list of our potentially inherited properties as dirtied by an ancestor.
 	void DirtyInheritedProperties(const PropertyNameList& properties);
 
-	static const Property* GetLocalProperty(const String & name, PropertyDictionary * local_properties, ElementDefinition * definition, const PseudoClassList & pseudo_classes);
-	static const Property* GetProperty(const String & name, Element * element, PropertyDictionary * local_properties, ElementDefinition * definition, const PseudoClassList & pseudo_classes);
-	static void TransitionPropertyChanges(Element * element, PropertyNameList & properties, PropertyDictionary * local_properties, ElementDefinition * old_definition, ElementDefinition * new_definition,
+	static const Property* GetLocalProperty(const String & name, const PropertyDictionary * local_properties, const ElementDefinition * definition, const PseudoClassList & pseudo_classes);
+	static const Property* GetProperty(const String & name, Element * element, const PropertyDictionary * local_properties, const ElementDefinition * definition, const PseudoClassList & pseudo_classes);
+	static void TransitionPropertyChanges(Element * element, PropertyNameList & properties, const PropertyDictionary * local_properties, const ElementDefinition * old_definition, const ElementDefinition * new_definition,
 		const PseudoClassList & pseudo_classes_before, const PseudoClassList & pseudo_classes_after);
 
 	// Element these properties belong to