Attach event listeners upon attribute changes

Include/RmlUi/Core/ElementUtilities.h

@@ -87,10 +87,6 @@ public:
 	/// @return The string width, in pixels.
 	static int GetStringWidth(Element* element, const String& string, Character prior_character = Character::Null);
 
-	/// Bind and instance all event attributes on the given element onto the element
-	/// @param element Element to bind events on
-	static void BindEventAttributes(Element* element);
-
 	/// Generates the clipping region for an element.
 	/// @param[out] clip_origin The origin, in context coordinates, of the origin of the element's clipping window.
 	/// @param[out] clip_dimensions The size, in context coordinates, of the element's clipping window.

Source/Core/Context.cpp

@@ -270,8 +270,6 @@ ElementDocument* Context::LoadDocument(Stream* stream)
 	
 	root->AppendChild(std::move(element));
 
-	ElementUtilities::BindEventAttributes(document);
-
 	// The 'load' event is fired before updating the document, because the user might
 	// need to initalize things before running an update. The drawback is that computed
 	// values and layouting are not performed yet, resulting in default values when

Source/Core/Element.cpp

@@ -73,6 +73,7 @@ static constexpr int ChildNotifyLevels = 2;
 struct ElementMeta
 {
 	ElementMeta(Element* el) : event_dispatcher(el), style(el), background_border(el), decoration(el), scroll(el) {}
+	SmallUnorderedMap<EventId, EventListener*> attribute_event_listeners;
 	EventDispatcher event_dispatcher;
 	ElementStyle style;
 	ElementBackgroundBorder background_border;
@@ -1200,14 +1201,14 @@ void Element::Click()
 }
 
 // Adds an event listener
-void Element::AddEventListener(const String& event, EventListener* listener, bool in_capture_phase)
+void Element::AddEventListener(const String& event, EventListener* listener, const bool in_capture_phase)
 {
-	EventId id = EventSpecificationInterface::GetIdOrInsert(event);
+	const EventId id = EventSpecificationInterface::GetIdOrInsert(event);
 	meta->event_dispatcher.AttachEvent(id, listener, in_capture_phase);
 }
 
 // Adds an event listener
-void Element::AddEventListener(EventId id, EventListener* listener, bool in_capture_phase)
+void Element::AddEventListener(const EventId id, EventListener* listener, const bool in_capture_phase)
 {
 	meta->event_dispatcher.AttachEvent(id, listener, in_capture_phase);
 }
@@ -1659,48 +1660,66 @@ void Element::OnStyleSheetChange()
 // Called when attributes on the element are changed.
 void Element::OnAttributeChange(const ElementAttributes& changed_attributes)
 {
-	auto it = changed_attributes.find("id");
-	if (it != changed_attributes.end())
-	{
-		id = it->second.Get<String>();
-		meta->style.DirtyDefinition();
-	}
-
-	it = changed_attributes.find("class");
-	if (it != changed_attributes.end())
-	{
-		meta->style.SetClassNames(it->second.Get<String>());
-	}
-
-	if (changed_attributes.count("colspan") || changed_attributes.count("rowspan"))
+	for (const auto& element_attribute : changed_attributes)
 	{
-		if (meta->computed_values.display == Style::Display::TableCell)
-			DirtyLayout();
-	}
-
-	if (changed_attributes.count("span"))
-	{
-		if (meta->computed_values.display == Style::Display::TableColumn || meta->computed_values.display == Style::Display::TableColumnGroup)
+		const auto& attribute = element_attribute.first;
+		const auto& value = element_attribute.second;
+		if (attribute == "id")
+		{
+			id = value.Get<String>();
+			meta->style.DirtyDefinition();
+		}
+		else if (attribute == "class")
+		{
+			meta->style.SetClassNames(value.Get<String>());
+		}
+		else if (((attribute == "colspan" || attribute == "rowspan") && meta->computed_values.display == Style::Display::TableCell)
+			|| (attribute == "span" && (meta->computed_values.display == Style::Display::TableColumn || meta->computed_values.display == Style::Display::TableColumnGroup)))
+		{
 			DirtyLayout();
-	}
-
-	it = changed_attributes.find("style");
-	if (it != changed_attributes.end())
-	{
-		if (it->second.GetType() == Variant::STRING)
+		}
+		else if (attribute.size() > 2 && attribute[0] == 'o' && attribute[1] == 'n')
 		{
-			PropertyDictionary properties;
-			StyleSheetParser parser;
-			parser.ParseProperties(properties, it->second.GetReference<String>());
+			static constexpr bool IN_CAPTURE_PHASE = false;
+
+			auto& attribute_event_listeners = meta->attribute_event_listeners;
+			auto& event_dispatcher = meta->event_dispatcher;
+			const auto event_id = EventSpecificationInterface::GetIdOrInsert(attribute.substr(2));
+			const auto remove_event_listener_if_exists = [&attribute_event_listeners, &event_dispatcher, event_id]()
+			{
+				const auto listener_it = attribute_event_listeners.find(event_id);
+				if (listener_it != attribute_event_listeners.cend())
+				{
+					event_dispatcher.DetachEvent(event_id, listener_it->second, IN_CAPTURE_PHASE);
+					attribute_event_listeners.erase(listener_it);
+				}
+			};
 
-			for (const auto& name_value : properties.GetProperties())
+			if (value.GetType() == Variant::Type::STRING)
 			{
-				meta->style.SetProperty(name_value.first, name_value.second);
+				remove_event_listener_if_exists();
+
+				const auto value_as_string = value.Get<String>();
+				auto insertion_result = attribute_event_listeners.emplace(event_id, Factory::InstanceEventListener(value_as_string, this));
+				if (auto* listener = insertion_result.first->second)
+					event_dispatcher.AttachEvent(event_id, listener, IN_CAPTURE_PHASE);
 			}
+			else if (value.GetType() == Variant::Type::NONE)
+				remove_event_listener_if_exists();
 		}
-		else if (it->second.GetType() != Variant::NONE)
+		else if (attribute == "style")
 		{
-			Log::Message(Log::LT_WARNING, "Invalid 'style' attribute, string type required. In element: %s", GetAddress().c_str());
+			if (value.GetType() == Variant::STRING)
+			{
+				PropertyDictionary properties;
+				StyleSheetParser parser;
+				parser.ParseProperties(properties, value.GetReference<String>());
+
+				for (const auto& name_value : properties.GetProperties())
+					meta->style.SetProperty(name_value.first, name_value.second);
+			}
+			else if (value.GetType() != Variant::NONE)
+				Log::Message(Log::LT_WARNING, "Invalid 'style' attribute, string type required. In element: %s", GetAddress().c_str());
 		}
 	}
 }

Source/Core/ElementUtilities.cpp

@@ -163,20 +163,6 @@ int ElementUtilities::GetStringWidth(Element* element, const String& string, Cha
 	return GetFontEngineInterface()->GetStringWidth(font_face_handle, string, prior_character);
 }
 
-void ElementUtilities::BindEventAttributes(Element* element)
-{
-	// Check for and instance the on* events
-	for (const auto& pair: element->GetAttributes())
-	{
-		if (pair.first.size() > 2 && pair.first[0] == 'o' && pair.first[1] == 'n')
-		{
-			EventListener* listener = Factory::InstanceEventListener(pair.second.Get<String>(), element);
-			if (listener)
-				element->AddEventListener(pair.first.substr(2), listener, false);
-		}
-	}
-}
-	
 // Generates the clipping region for an element.
 bool ElementUtilities::GetClippingRegion(Vector2i& clip_origin, Vector2i& clip_dimensions, Element* element)
 {

Source/Core/EventDispatcher.cpp

@@ -33,7 +33,7 @@
 #include "../../Include/RmlUi/Core/Factory.h"
 #include "EventSpecification.h"
 #include <algorithm>
-#include <limits.h>
+#include <limits>
 
 namespace Rml {
 
@@ -50,8 +50,8 @@ struct CompareIdPhase {
 
 
 EventDispatcher::EventDispatcher(Element* _element)
+	: element(_element)
 {
-	element = _element;
 }
 
 EventDispatcher::~EventDispatcher()
@@ -61,36 +61,27 @@ EventDispatcher::~EventDispatcher()
 		event.listener->OnDetach(element);
 }
 
-void EventDispatcher::AttachEvent(EventId id, EventListener* listener, bool in_capture_phase)
+void EventDispatcher::AttachEvent(const EventId id, EventListener* listener, const bool in_capture_phase)
 {
-	EventListenerEntry entry(id, listener, in_capture_phase);
+	const EventListenerEntry entry(id, listener, in_capture_phase);
 
 	// The entries are sorted by (id,phase). Find the bounds of this sort, then find the entry.
-	auto range = std::equal_range(listeners.begin(), listeners.end(), entry, CompareIdPhase());
-	auto it = std::find(range.first, range.second, entry);
-
-	if(it == range.second)
+	const auto range = std::equal_range(listeners.cbegin(), listeners.cend(), entry, CompareIdPhase());
+	const auto matching_entry_it = std::find(range.first, range.second, entry);
+	if (matching_entry_it == range.second)
 	{
-		// No existing entry found, add it to the end of the (id, phase) range
-		listeners.emplace(it, entry);
+		listeners.emplace(range.second, entry);
 		listener->OnAttach(element);
 	}
 }
 
 
-void EventDispatcher::DetachEvent(EventId id, EventListener* listener, bool in_capture_phase)
+void EventDispatcher::DetachEvent(const EventId id, EventListener* listener, const bool in_capture_phase)
 {
-	EventListenerEntry entry(id, listener, in_capture_phase);
-	
-	// The entries are sorted by (id,phase). Find the bounds of this sort, then find the entry.
-	// We could also just do a linear search over all the entries, which might be faster for low number of entries.
-	auto range = std::equal_range(listeners.begin(), listeners.end(), entry, CompareIdPhase());
-	auto it = std::find(range.first, range.second, entry);
-
-	if (it != range.second)
+	const auto listenerIt = std::find(listeners.cbegin(), listeners.cend(), EventListenerEntry(id, listener, in_capture_phase));
+	if (listenerIt != listeners.cend())
 	{
-		// We found our listener, remove it
-		listeners.erase(it);
+		listeners.erase(listenerIt);
 		listener->OnDetach(element);
 	}
 }
@@ -178,7 +169,7 @@ bool EventDispatcher::DispatchEvent(Element* target_element, const EventId id, c
 	if (!event)
 		return false;
 
-	int previous_sort_value = INT_MAX;
+	auto previous_sort_value = std::numeric_limits<int>::max();
 
 	// Process the event in each listener.
 	for (const auto& listener_desc : listeners)

Source/Core/EventDispatcher.h

@@ -39,7 +39,8 @@ class EventListener;
 struct CollectedListener;
 
 struct EventListenerEntry {
-	EventListenerEntry(EventId id, EventListener* listener, bool in_capture_phase) : id(id), in_capture_phase(in_capture_phase), listener(listener) {}
+	EventListenerEntry(const EventId id, EventListener* listener, const bool in_capture_phase) : id(id), in_capture_phase(in_capture_phase), listener(listener) {}
+
 	EventId id;
 	bool in_capture_phase;
 	EventListener* listener;
@@ -63,15 +64,15 @@ public:
 	/// Destructor
 	~EventDispatcher();
 
-	/// Attaches a new listener to the specified event name
-	/// @param[in] type Type of the event to attach to
-	/// @param[in] event_listener The event listener to be notified when the event fires
-	/// @param[in] in_capture_phase Should the listener be notified in the capture phase
+	/// Attaches a new listener to the specified event name.
+	/// @param[in] type Type of the event to attach to.
+	/// @param[in] event_listener The event listener to be notified when the event fires.
+	/// @param[in] in_capture_phase Should the listener be notified in the capture phase.
 	void AttachEvent(EventId id, EventListener* event_listener, bool in_capture_phase);
 
 	/// Detaches a listener from the specified event name
 	/// @param[in] type Type of the event to attach to
-	/// @para[in]m event_listener The event listener to be notified when the event fires
+	/// @param[in] event_listener The event listener to be notified when the event fires
 	/// @param[in] in_capture_phase Should the listener be notified in the capture phase
 	void DetachEvent(EventId id, EventListener* listener, bool in_capture_phase);
 

Source/Core/Factory.cpp

@@ -359,23 +359,16 @@ ElementInstancer* Factory::GetElementInstancer(const String& tag)
 // Instances a single element.
 ElementPtr Factory::InstanceElement(Element* parent, const String& instancer_name, const String& tag, const XMLAttributes& attributes)
 {
-	ElementInstancer* instancer = GetElementInstancer(instancer_name);
-
-	if (instancer)
+	if (ElementInstancer* instancer = GetElementInstancer(instancer_name))
 	{
-		ElementPtr element = instancer->InstanceElement(parent, tag, attributes);		
-
-		// Process the generic attributes and bind any events
-		if (element)
+		if (ElementPtr element = instancer->InstanceElement(parent, tag, attributes))
 		{
 			element->SetInstancer(instancer);
 			element->SetAttributes(attributes);
-			ElementUtilities::BindEventAttributes(element.get());
 
 			PluginRegistry::NotifyElementCreate(element.get());
+			return element;
 		}
-
-		return element;
 	}
 
 	return nullptr;