Cleanup element style and definition.

Include/Rocket/Core/Types.h

@@ -132,7 +132,7 @@ using SmallUnorderedSet = chobo::flat_set< T >;
 // Container types for some common lists
 typedef std::vector< Element* > ElementList;
 typedef std::vector< ElementAnimation > ElementAnimationList;
-typedef SmallOrderedSet< String > PseudoClassList; /* Ordered reason: See ElementDefinition */
+typedef SmallUnorderedSet< String > PseudoClassList;
 typedef SmallUnorderedSet< String > AttributeNameList;
 typedef SmallOrderedSet< PropertyId > PropertyNameList;
 typedef UnorderedMap< PropertyId, Property > PropertyMap;
@@ -148,15 +148,6 @@ struct TransitionList;
 using DecoratorList = std::vector<std::shared_ptr<Decorator>>;
 using AnimationList = std::vector<Animation>;
 
-// Pseudo class properties
-// Defines for the optimised version of the pseudo-class properties (note the difference from the
-// PseudoClassPropertyMap defined in StyleSheetNode.h ... bit clumsy). Here the properties are stored as a list
-// of definitions against each property name in specificity-order, along with the pseudo-class requirements for each
-// one. This makes it much more straight-forward to query at run-time.
-typedef std::pair< PseudoClassList, Property > PseudoClassProperty;
-typedef std::vector< PseudoClassProperty > PseudoClassPropertyList;
-typedef SmallUnorderedMap< PropertyId, PseudoClassPropertyList > PseudoClassPropertyDictionary;
-
 }
 }
 

Source/Core/Element.cpp

@@ -1801,7 +1801,9 @@ void Element::OnPropertyChange(const PropertyNameList& changed_properties)
 		if (all_dirty || 
 			changed_properties.find(PropertyId::Display) != changed_properties.end())
 		{
-			// TODO: Is this really necessary. It has problems, because any change to definitions as a result of this will only be reflected on the next frame.
+			// Due to structural pseudo-classes, this may change the element definition in siblings and parent.
+			// However, the definitions will only be changed on the next update loop which may result in jarring behavior for one @frame.
+			// A possible workaround is to add the parent to a list of elements that need to be updated again.
 			if (parent != NULL)
 				parent->DirtyStructure();
 		}
@@ -1839,7 +1841,7 @@ void Element::OnPropertyChange(const PropertyNameList& changed_properties)
 		changed_properties.find(PropertyId::Top) != changed_properties.end() ||
 		changed_properties.find(PropertyId::Bottom) != changed_properties.end())
 	{
-		// TODO: This should happen during/after layout, as the containing box is not properly defined yet
+		// TODO: This should happen during/after layout, as the containing box is not properly defined yet. Off-by-one @frame issue.
 		UpdateOffset();
 		DirtyOffset();
 	}

Source/Core/ElementDefinition.cpp

@@ -28,27 +28,21 @@
 #include "precompiled.h"
 #include "ElementDefinition.h"
 #include "StyleSheetNode.h"
-#include "../../Include/Rocket/Core/Log.h"
 
 namespace Rocket {
 namespace Core {
 
-ElementDefinition::ElementDefinition()
+ElementDefinition::ElementDefinition(const std::vector< const StyleSheetNode* >& style_sheet_nodes)
 {
+	// Initialises the element definition from the list of style sheet nodes.
+	for (size_t i = 0; i < style_sheet_nodes.size(); ++i)
+		properties.Merge(style_sheet_nodes[i]->GetProperties());
 }
 
 ElementDefinition::~ElementDefinition()
 {
 }
 
-// Initialises the element definition from a list of style sheet nodes.
-void ElementDefinition::Initialise(const std::vector< const StyleSheetNode* >& style_sheet_nodes)
-{
-	// Merge the default (non-pseudo-class) properties.
-	for (size_t i = 0; i < style_sheet_nodes.size(); ++i)
-		properties.Merge(style_sheet_nodes[i]->GetProperties());
-}
-
 // Returns a specific property from the element definition's base properties.
 const Property* ElementDefinition::GetProperty(PropertyId id) const
 {

Source/Core/ElementDefinition.h

@@ -38,25 +38,17 @@ class StyleSheetNode;
 class ElementDefinitionIterator;
 
 /**
+	ElementDefinition provides an element's applicable properties from its stylesheet.
+
 	@author Peter Curry
  */
 
 class ElementDefinition : public ReferenceCountable
 {
 public:
-	enum PseudoClassVolatility
-	{
-		STABLE,					// pseudo-class has no volatility
-		FONT_VOLATILE,			// pseudo-class may impact on font effects
-		STRUCTURE_VOLATILE		// pseudo-class may impact on definitions of child elements
-	};
-
-	ElementDefinition();
+	ElementDefinition(const std::vector< const StyleSheetNode* >& style_sheet_nodes);
 	virtual ~ElementDefinition();
 
-	/// Initialises the element definition from a list of style sheet nodes.
-	void Initialise(const std::vector< const StyleSheetNode* >& style_sheet_nodes);
-
 	/// Returns a specific property from the element definition's base properties.
 	/// @param[in] name The name of the property to return.
 	/// @param[in] pseudo_classes The pseudo-classes currently active on the calling element.

Source/Core/ElementStyle.cpp

@@ -71,10 +71,10 @@ const ElementDefinition* ElementStyle::GetDefinition() const
 }
 
 // Returns one of this element's properties.
-const Property* ElementStyle::GetLocalProperty(PropertyId id, const PropertyDictionary& local_properties, const ElementDefinition* definition)
+const Property* ElementStyle::GetLocalProperty(PropertyId id, const PropertyDictionary& inline_properties, const ElementDefinition* definition)
 {
 	// Check for overriding local properties.
-	const Property* property = local_properties.GetProperty(id);
+	const Property* property = inline_properties.GetProperty(id);
 	if (property)
 		return property;
 
@@ -86,9 +86,9 @@ const Property* ElementStyle::GetLocalProperty(PropertyId id, const PropertyDict
 }
 
 // Returns one of this element's properties.
-const Property* ElementStyle::GetProperty(PropertyId id, const Element* element, const PropertyDictionary& local_properties, const ElementDefinition* definition)
+const Property* ElementStyle::GetProperty(PropertyId id, const Element* element, const PropertyDictionary& inline_properties, const ElementDefinition* definition)
 {
-	const Property* local_property = GetLocalProperty(id, local_properties, definition);
+	const Property* local_property = GetLocalProperty(id, inline_properties, definition);
 	if (local_property)
 		return local_property;
 
@@ -117,13 +117,13 @@ const Property* ElementStyle::GetProperty(PropertyId id, const 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, const PropertyDictionary& local_properties, const ElementDefinition* old_definition, const ElementDefinition* new_definition)
+void ElementStyle::TransitionPropertyChanges(Element* element, PropertyNameList& properties, const PropertyDictionary& inline_properties, const ElementDefinition* old_definition, const ElementDefinition* new_definition)
 {
 	ROCKET_ASSERT(element);
 	if (!old_definition || !new_definition || properties.empty())
 		return;
 
-	if (const Property* transition_property = GetLocalProperty(PropertyId::Transition, local_properties, new_definition))
+	if (const Property* transition_property = GetLocalProperty(PropertyId::Transition, inline_properties, new_definition))
 	{
 		auto transition_list = transition_property->Get<TransitionList>();
 
@@ -133,7 +133,7 @@ void ElementStyle::TransitionPropertyChanges(Element* element, PropertyNameList&
 
 			auto add_transition = [&](const Transition& transition) {
 				bool transition_added = false;
-				const Property* start_value = GetProperty(transition.id, element, local_properties, old_definition);
+				const Property* start_value = GetProperty(transition.id, element, inline_properties, old_definition);
 				const Property* target_value = GetProperty(transition.id, element, empty_properties, new_definition);
 				if (start_value && target_value && (*start_value != *target_value))
 					transition_added = element->StartTransition(transition, *start_value, *target_value);
@@ -203,7 +203,7 @@ void ElementStyle::UpdateDefinition()
 			if (new_definition)
 				new_definition->GetDefinedProperties(properties);
 
-			TransitionPropertyChanges(element, properties, local_properties, definition, new_definition);
+			TransitionPropertyChanges(element, properties, inline_properties, definition, new_definition);
 
 			if (definition)
 				definition->RemoveReference();
@@ -318,7 +318,7 @@ bool ElementStyle::SetProperty(PropertyId id, const Property& property)
 	if (!new_property.definition)
 		return false;
 
-	local_properties.SetProperty(id, new_property);
+	inline_properties.SetProperty(id, new_property);
 	DirtyProperty(id);
 
 	return true;
@@ -327,10 +327,10 @@ bool ElementStyle::SetProperty(PropertyId id, const Property& property)
 // Removes a local property override on the element.
 void ElementStyle::RemoveProperty(PropertyId id)
 {
-	int size_before = local_properties.GetNumProperties();
-	local_properties.RemoveProperty(id);
+	int size_before = inline_properties.GetNumProperties();
+	inline_properties.RemoveProperty(id);
 
-	if(local_properties.GetNumProperties() != size_before)
+	if(inline_properties.GetNumProperties() != size_before)
 		DirtyProperty(id);
 }
 
@@ -339,18 +339,18 @@ void ElementStyle::RemoveProperty(PropertyId id)
 // Returns one of this element's properties.
 const Property* ElementStyle::GetProperty(PropertyId id) const
 {
-	return GetProperty(id, element, local_properties, definition);
+	return GetProperty(id, element, inline_properties, definition);
 }
 
 // Returns one of this element's properties.
 const Property* ElementStyle::GetLocalProperty(PropertyId id) const
 {
-	return GetLocalProperty(id, local_properties, definition);
+	return GetLocalProperty(id, inline_properties, definition);
 }
 
 const PropertyMap& ElementStyle::GetLocalStyleProperties() const
 {
-	return local_properties.GetProperties();
+	return inline_properties.GetProperties();
 }
 
 // Returns the active style sheet for this element. This may be NULL.
@@ -481,22 +481,18 @@ PropertiesIterator ElementStyle::Iterate() const {
 	static_assert(__cplusplus >= 201402L, "C++14 or higher required, see comment.");
 #endif
 
-	const PropertyMap& property_map = local_properties.GetProperties();
+	const PropertyMap& property_map = inline_properties.GetProperties();
 	auto it_style_begin = property_map.begin();
 	auto it_style_end = property_map.end();
 
-	PseudoClassPropertyDictionary::const_iterator it_pseudo{}, it_pseudo_end{};
-	PropertyMap::const_iterator it_base{}, it_base_end{};
+	PropertyMap::const_iterator it_definition{}, it_definition_end{};
 	if (definition)
 	{
-		static const PseudoClassPropertyDictionary pseudo;
-		const PropertyMap& base = definition->GetProperties().GetProperties();
-		it_pseudo = pseudo.begin();
-		it_pseudo_end = pseudo.end();
-		it_base = base.begin();
-		it_base_end = base.end();
+		const PropertyMap& definition_properties = definition->GetProperties().GetProperties();
+		it_definition = definition_properties.begin();
+		it_definition_end = definition_properties.end();
 	}
-	return PropertiesIterator(pseudo_classes, it_style_begin, it_style_end, it_pseudo, it_pseudo_end, it_base, it_base_end);
+	return PropertiesIterator(pseudo_classes, it_style_begin, it_style_end, it_definition, it_definition_end);
 }
 
 // Sets a single property as dirty.

Source/Core/ElementStyle.h

@@ -142,9 +142,9 @@ private:
 	// Sets a list of our potentially inherited properties as dirtied by an ancestor.
 	void DirtyInheritedProperties(const PropertyNameList& properties);
 
-	static const Property* GetLocalProperty(PropertyId id, const PropertyDictionary & local_properties, const ElementDefinition * definition);
-	static const Property* GetProperty(PropertyId id, const Element * element, const PropertyDictionary & local_properties, const ElementDefinition * definition);
-	static void TransitionPropertyChanges(Element * element, PropertyNameList & properties, const PropertyDictionary & local_properties, const ElementDefinition * old_definition, const ElementDefinition * new_definition);
+	static const Property* GetLocalProperty(PropertyId id, const PropertyDictionary & inline_properties, const ElementDefinition * definition);
+	static const Property* GetProperty(PropertyId id, const Element * element, const PropertyDictionary & inline_properties, const ElementDefinition * definition);
+	static void TransitionPropertyChanges(Element * element, PropertyNameList & properties, const PropertyDictionary & inline_properties, const ElementDefinition * old_definition, const ElementDefinition * new_definition);
 
 	// Element these properties belong to
 	Element* element;
@@ -155,8 +155,8 @@ private:
 	PseudoClassList pseudo_classes;
 
 	// Any properties that have been overridden in this element.
-	PropertyDictionary local_properties;
-	// The definition of this element; if this is NULL one will be fetched from the element's style.
+	PropertyDictionary inline_properties;
+	// The definition of this element, provides applicable properties from the stylesheet.
 	ElementDefinition* definition;
 	// Set if a new element definition should be fetched from the style.
 	bool definition_dirty;

Source/Core/PropertiesIterator.h

@@ -42,26 +42,20 @@ class PropertiesIterator {
 public:
 	using ValueType = std::pair<PropertyId, const Property&>;
 	using PropertyIt = PropertyMap::const_iterator;
-	using PseudoIt = PseudoClassPropertyDictionary::const_iterator;
 
-	PropertiesIterator(const PseudoClassList& element_pseudo_classes, PropertyIt it_style, PropertyIt it_style_end, PseudoIt it_pseudo, PseudoIt it_pseudo_end, PropertyIt it_base, PropertyIt it_base_end)
-		: element_pseudo_classes(&element_pseudo_classes), it_style(it_style), it_style_end(it_style_end), it_pseudo(it_pseudo), it_pseudo_end(it_pseudo_end), it_base(it_base), it_base_end(it_base_end)
+	PropertiesIterator(const PseudoClassList& element_pseudo_classes, PropertyIt it_style, PropertyIt it_style_end, PropertyIt it_definition, PropertyIt it_definition_end)
+		: element_pseudo_classes(&element_pseudo_classes), it_style(it_style), it_style_end(it_style_end), it_definition(it_definition), it_definition_end(it_definition_end)
 	{
-		if (this->element_pseudo_classes->empty())
-			this->it_pseudo = this->it_pseudo_end;
 		ProceedToNextValid();
 	}
 
 	PropertiesIterator& operator++() {
 		if (it_style != it_style_end)
-			// First, we iterate over the local properties
+			// We iterate over the local style properties first
 			++it_style;
-		else if (it_pseudo != it_pseudo_end)
-			// Then, we iterate over the properties given by the pseudo classes in the element's definition
-			++i_pseudo_class;
 		else
-			// Finally, we iterate over the base properties given by the element's definition
-			++it_base;
+			// .. and then over the properties given by the element's definition
+			++it_definition;
 		// If we reached the end of one of the iterator pairs, we need to continue iteration on the next pair.
 		ProceedToNextValid();
 		return *this;
@@ -71,9 +65,7 @@ public:
 	{
 		if (it_style != it_style_end)
 			return { it_style->first, it_style->second };
-		if (it_pseudo != it_pseudo_end)
-			return { it_pseudo->first, it_pseudo->second[i_pseudo_class].second };
-		return { it_base->first, it_base->second };
+		return { it_definition->first, it_definition->second };
 	}
 
 	bool AtEnd() const {
@@ -83,8 +75,7 @@ public:
 	// Return the list of pseudo classes which defines the current property, possibly null.
 	const PseudoClassList* GetPseudoClassList() const
 	{
-		if (it_style == it_style_end && it_pseudo != it_pseudo_end)
-			return &it_pseudo->second[i_pseudo_class].first;
+		// TODO: Not implemented
 		return nullptr;
 	}
 
@@ -92,9 +83,7 @@ private:
 	const PseudoClassList* element_pseudo_classes;
 	DirtyPropertyList iterated_properties;
 	PropertyIt it_style, it_style_end;
-	PseudoIt it_pseudo, it_pseudo_end;
-	PropertyIt it_base, it_base_end;
-	size_t i_pseudo_class = 0;
+	PropertyIt it_definition, it_definition_end;
 	bool at_end = false;
 
 	inline bool IsDirtyRemove(PropertyId id)
@@ -107,21 +96,17 @@ private:
 		return false;
 	}
 
-	inline void ProceedToNextValid() {
+	inline void ProceedToNextValid() 
+	{
 		for (; it_style != it_style_end; ++it_style)
 		{
 			if (IsDirtyRemove(it_style->first))
 				return;
 		}
 
-		// Iterate over all the pseudo classes and match the applicable rules
-		for (; it_pseudo != it_pseudo_end; ++it_pseudo)
-		{
-		}
-
-		for (; it_base != it_base_end; ++it_base)
+		for (; it_definition != it_definition_end; ++it_definition)
 		{
-			if (IsDirtyRemove(it_base->first))
+			if (IsDirtyRemove(it_definition->first))
 				return;
 		}
 

Source/Core/StyleSheet.cpp

@@ -274,16 +274,12 @@ ElementDefinition* StyleSheet::GetElementDefinition(const Element* element) cons
 
 	// Create the new definition and add it to our cache. One reference count is added, bringing the total to two; one
 	// for the element that requested it, and one for the cache.
-	ElementDefinition* new_definition = new ElementDefinition();
-	new_definition->Initialise(applicable_nodes);
-
-
+	ElementDefinition* new_definition = new ElementDefinition(applicable_nodes);
 
 	// Add to the node cache.
 	node_cache[seed] = new_definition;
 	new_definition->AddReference();
 
-	applicable_nodes.clear();
 	return new_definition;
 }