Fix structural data views (data-for) not working with data-model on the body element, see #790

The issue being that the data model is not available on the body element while its XML is being parsed, until it is attached to the context's root element. The normal data views are instantiated only after attaching the document in this case. However, this did not work for structural data views, they could only be attached during XML parsing.

This commit addresses the issue by allowing structural data views also to be attached while attaching the document to the context root.

An alternative fix could be to make the data model available during XML parsing, as the context is known at that point. Both approaches could supplement each other.

It seems the previously reported issue with data-model not working with body templates was the same underlying issue as addressed here. A test has been added, and this issue is considered fixed as well.

Include/RmlUi/Core/ElementUtilities.h

@@ -138,11 +138,6 @@ public:
 	/// Attributes such as 'data-' are used to create the views and controllers.
 	/// @return True if a data view or controller was constructed.
 	static bool ApplyDataViewsControllers(Element* element);
-
-	/// Creates data views that use a raw inner xml content string to construct child elements.
-	/// Right now, this only applies to the 'data-for' view.
-	/// @return True if a data view was constructed.
-	static bool ApplyStructuralDataViews(Element* element, const String& inner_rml);
 };
 
 } // namespace Rml

Include/RmlUi/Core/Factory.h

@@ -203,7 +203,7 @@ public:
 	static void RegisterDataControllerInstancer(DataControllerInstancer* instancer, const String& type_name);
 
 	/// Instance the data view with the given type name.
-	static DataViewPtr InstanceDataView(const String& type_name, Element* element, bool is_structural_view);
+	static DataViewPtr InstanceDataView(const String& type_name, Element* element);
 
 	/// Instance the data controller with the given type name.
 	static DataControllerPtr InstanceDataController(const String& type_name, Element* element);
@@ -212,7 +212,7 @@ public:
 	static bool IsStructuralDataView(const String& type_name);
 
 	/// Returns the list of element attribute names with an associated structural data view instancer.
-	static const StringList& GetStructuralDataViewAttributeNames();
+	static const SmallUnorderedSet<String>& GetStructuralDataViewAttributeNames();
 
 private:
 	Factory();

Source/Core/DataView.h

@@ -70,10 +70,10 @@ public:
 	// Initialize the data view.
 	// @param[in] model The data model the view will be attached to.
 	// @param[in] element The element which spawned the view.
-	// @param[in] expression The value of the element's 'data-' attribute which spawned the view (see above).
-	// @param[in] modifier_or_inner_rml The modifier for the given view type (see above), or the inner rml contents for structural data views.
+	// @param[in] expression The value of the element's 'data-' attribute which spawned the view (see class documentation).
+	// @param[in] modifier The modifier for the given view type (see class documentation).
 	// @return True on success.
-	virtual bool Initialize(DataModel& model, Element* element, const String& expression, const String& modifier_or_inner_rml) = 0;
+	virtual bool Initialize(DataModel& model, Element* element, const String& expression, const String& modifier) = 0;
 
 	// Update the data view.
 	// Returns true if the update resulted in a document change.

Source/Core/DataViewDefault.cpp

@@ -52,7 +52,7 @@ DataViewCommon::DataViewCommon(Element* element, String override_modifier, int s
 
 bool DataViewCommon::Initialize(DataModel& model, Element* element, const String& expression_str, const String& in_modifier)
 {
-	// The modifier can be overriden in the constructor
+	// The modifier can be overridden in the constructor
 	if (modifier.empty())
 		modifier = in_modifier;
 
@@ -454,10 +454,8 @@ String DataViewText::BuildText() const
 
 DataViewFor::DataViewFor(Element* element) : DataView(element, 0) {}
 
-bool DataViewFor::Initialize(DataModel& model, Element* element, const String& in_expression, const String& in_rml_content)
+bool DataViewFor::Initialize(DataModel& model, Element* element, const String& in_expression, const String& /*modifier*/)
 {
-	rml_contents = in_rml_content;
-
 	StringList iterator_container_pair;
 	StringUtilities::ExpandString(iterator_container_pair, in_expression, ':');
 
@@ -503,15 +501,21 @@ bool DataViewFor::Initialize(DataModel& model, Element* element, const String& i
 
 	element->SetProperty(PropertyId::Display, Property(Style::Display::None));
 
-	// Copy over the attributes, but remove the 'data-for' which would otherwise recreate the data-for loop on all constructed children recursively.
-	attributes = element->GetAttributes();
-	for (auto it = attributes.begin(); it != attributes.end(); ++it)
+	// Copy over the attributes, but remove the 'data-for' which would otherwise recreate the data-for loop on all
+	// constructed children recursively. There is also no need for the 'rmlui-inner-rml' attribute if that is present.
+	const ElementAttributes& element_attributes = element->GetAttributes();
+	constexpr size_t num_data_for_attributes = 1;
+	if (element_attributes.size() < num_data_for_attributes)
 	{
-		if (it->first == "data-for")
-		{
-			attributes.erase(it);
-			break;
-		}
+		Log::Message(Log::LT_WARNING, "Expected attributes for data-for view missing from element: %s", element->GetAddress().c_str());
+		return false;
+	}
+	attributes.reserve(element_attributes.size() - num_data_for_attributes);
+	for (const auto& attribute : element->GetAttributes())
+	{
+		if (attribute.first == "data-for" || attribute.first == "rmlui-inner-rml")
+			continue;
+		attributes.emplace(attribute.first, attribute.second);
 	}
 
 	return true;
@@ -547,7 +551,8 @@ bool DataViewFor::Update(DataModel& model)
 			Element* new_element = element->GetParentNode()->InsertBefore(std::move(new_element_ptr), element);
 			elements.push_back(new_element);
 
-			elements[i]->SetInnerRML(rml_contents);
+			const String* rml_contents = RMLContents();
+			elements[i]->SetInnerRML(rml_contents ? *rml_contents : "");
 
 			RMLUI_ASSERT(i < (int)elements.size());
 		}
@@ -576,6 +581,24 @@ void DataViewFor::Release()
 	delete this;
 }
 
+const String* DataViewFor::RMLContents() const
+{
+	if (Element* element = GetElement())
+	{
+		if (Variant* attribute = element->GetAttribute("rmlui-inner-rml"))
+		{
+			if (attribute->GetType() == Variant::STRING)
+				return &attribute->GetReference<String>();
+		}
+		Log::Message(Log::LT_WARNING, "Missing or invalid RML contents in data-for view on element %s", element->GetAddress().c_str());
+	}
+	else
+	{
+		Log::Message(Log::LT_WARNING, "Invalid element in data-for view");
+	}
+	return nullptr;
+}
+
 DataViewAlias::DataViewAlias(Element* element) : DataView(element, 0) {}
 
 StringList DataViewAlias::GetVariableNameList() const

Source/Core/DataViewDefault.h

@@ -154,7 +154,7 @@ class DataViewFor final : public DataView {
 public:
 	DataViewFor(Element* element);
 
-	bool Initialize(DataModel& model, Element* element, const String& expression, const String& inner_rml) override;
+	bool Initialize(DataModel& model, Element* element, const String& expression, const String& modifier) override;
 
 	bool Update(DataModel& model) override;
 
@@ -164,10 +164,11 @@ protected:
 	void Release() override;
 
 private:
+	const String* RMLContents() const;
+
 	DataAddress container_address;
 	String iterator_name;
 	String iterator_index_name;
-	String rml_contents;
 	ElementAttributes attributes;
 
 	ElementList elements;

Source/Core/ElementUtilities.cpp

@@ -407,7 +407,7 @@ bool ElementUtilities::ApplyTransform(Element& element)
 	return true;
 }
 
-static bool ApplyDataViewsControllersInternal(Element* element, const bool construct_structural_view, const String& structural_view_inner_rml)
+static bool ApplyDataViewsControllersInternal(Element* element)
 {
 	RMLUI_ASSERT(element);
 	bool result = false;
@@ -417,7 +417,7 @@ static bool ApplyDataViewsControllersInternal(Element* element, const bool const
 	{
 		struct ViewControllerInitializer {
 			String type;
-			String modifier_or_inner_rml;
+			String modifier;
 			String expression;
 			DataViewPtr view;
 			DataControllerPtr controller;
@@ -442,46 +442,39 @@ static bool ApplyDataViewsControllersInternal(Element* element, const bool const
 			{
 				const size_t type_end = name.find('-', data_str_length);
 				const size_t type_size = (type_end == String::npos ? String::npos : type_end - data_str_length);
-				String type_name = name.substr(data_str_length, type_size);
+				const String type_name = name.substr(data_str_length, type_size);
 
 				ViewControllerInitializer initializer;
 
-				// Structural data views are applied in a separate step from the normal views and controllers.
-				if (construct_structural_view)
-				{
-					if (DataViewPtr view = Factory::InstanceDataView(type_name, element, true))
-					{
-						initializer.modifier_or_inner_rml = structural_view_inner_rml;
-						initializer.view = std::move(view);
-					}
-				}
-				else
-				{
-					if (Factory::IsStructuralDataView(type_name))
-					{
-						// Structural data views should cancel all other non-structural data views and controllers. Exit now.
-						// Eg. in elements with a 'data-for' attribute, the data views should be constructed on the generated
-						// children elements and not on the current element generating the 'for' view.
-						return false;
-					}
+				const size_t modifier_offset = data_str_length + type_name.size() + 1;
+				if (modifier_offset < name.size())
+					initializer.modifier = name.substr(modifier_offset);
 
-					const size_t modifier_offset = data_str_length + type_name.size() + 1;
-					if (modifier_offset < name.size())
-						initializer.modifier_or_inner_rml = name.substr(modifier_offset);
+				if (DataViewPtr view = Factory::InstanceDataView(type_name, element))
+					initializer.view = std::move(view);
 
-					if (DataViewPtr view = Factory::InstanceDataView(type_name, element, false))
-						initializer.view = std::move(view);
-
-					if (DataControllerPtr controller = Factory::InstanceDataController(type_name, element))
-						initializer.controller = std::move(controller);
-				}
+				if (DataControllerPtr controller = Factory::InstanceDataController(type_name, element))
+					initializer.controller = std::move(controller);
 
 				if (initializer)
 				{
-					initializer.type = std::move(type_name);
+					initializer.type = type_name;
 					initializer.expression = attribute.second.Get<String>();
 
-					initializer_list.push_back(std::move(initializer));
+					if (Factory::IsStructuralDataView(type_name))
+					{
+						// Structural data views should cancel all other non-structural data views and controllers.
+						// Clear all other views, and only initialize this one.
+						// E.g. in elements with a 'data-for' attribute, the data views should be constructed on the
+						// generated children elements and not on the current element generating the 'for' view.
+						initializer_list.clear();
+						initializer_list.push_back(std::move(initializer));
+						break;
+					}
+					else
+					{
+						initializer_list.push_back(std::move(initializer));
+					}
 				}
 			}
 		}
@@ -494,7 +487,7 @@ static bool ApplyDataViewsControllersInternal(Element* element, const bool const
 
 			if (view)
 			{
-				if (view->Initialize(*data_model, element, initializer.expression, initializer.modifier_or_inner_rml))
+				if (view->Initialize(*data_model, element, initializer.expression, initializer.modifier))
 				{
 					data_model->AddView(std::move(view));
 					result = true;
@@ -506,7 +499,7 @@ static bool ApplyDataViewsControllersInternal(Element* element, const bool const
 
 			if (controller)
 			{
-				if (controller->Initialize(*data_model, element, initializer.expression, initializer.modifier_or_inner_rml))
+				if (controller->Initialize(*data_model, element, initializer.expression, initializer.modifier))
 				{
 					data_model->AddController(std::move(controller));
 					result = true;
@@ -523,12 +516,7 @@ static bool ApplyDataViewsControllersInternal(Element* element, const bool const
 
 bool ElementUtilities::ApplyDataViewsControllers(Element* element)
 {
-	return ApplyDataViewsControllersInternal(element, false, String());
-}
-
-bool ElementUtilities::ApplyStructuralDataViews(Element* element, const String& inner_rml)
-{
-	return ApplyDataViewsControllersInternal(element, true, inner_rml);
+	return ApplyDataViewsControllersInternal(element);
 }
 
 } // namespace Rml

Source/Core/Factory.cpp

@@ -163,8 +163,7 @@ struct FactoryData {
 	UnorderedMap<String, FontEffectInstancer*> font_effect_instancers;
 	UnorderedMap<String, DataViewInstancer*> data_view_instancers;
 	UnorderedMap<String, DataControllerInstancer*> data_controller_instancers;
-	SmallUnorderedMap<String, DataViewInstancer*> structural_data_view_instancers;
-	StringList structural_data_view_attribute_names;
+	SmallUnorderedSet<String> structural_data_view_attribute_names;
 };
 
 static ControlledLifetimeResource<FactoryData> factory_data;
@@ -585,20 +584,14 @@ EventListener* Factory::InstanceEventListener(const String& value, Element* elem
 
 void Factory::RegisterDataViewInstancer(DataViewInstancer* instancer, const String& name, bool is_structural_view)
 {
-	bool inserted = false;
-	if (is_structural_view)
-	{
-		inserted = factory_data->structural_data_view_instancers.emplace(name, instancer).second;
-		if (inserted)
-			factory_data->structural_data_view_attribute_names.push_back(String("data-") + name);
-	}
-	else
-	{
-		inserted = factory_data->data_view_instancers.emplace(name, instancer).second;
-	}
-
+	const bool inserted = factory_data->data_view_instancers.emplace(name, instancer).second;
 	if (!inserted)
+	{
 		Log::Message(Log::LT_WARNING, "Could not register data view instancer '%s'. The given name is already registered.", name.c_str());
+		return;
+	}
+	if (is_structural_view)
+		factory_data->structural_data_view_attribute_names.emplace("data-" + name);
 }
 
 void Factory::RegisterDataControllerInstancer(DataControllerInstancer* instancer, const String& name)
@@ -608,39 +601,30 @@ void Factory::RegisterDataControllerInstancer(DataControllerInstancer* instancer
 		Log::Message(Log::LT_WARNING, "Could not register data controller instancer '%s'. The given name is already registered.", name.c_str());
 }
 
-DataViewPtr Factory::InstanceDataView(const String& type_name, Element* element, bool is_structural_view)
+DataViewPtr Factory::InstanceDataView(const String& type_name, Element* element)
 {
 	RMLUI_ASSERT(element);
-
-	if (is_structural_view)
-	{
-		auto it = factory_data->structural_data_view_instancers.find(type_name);
-		if (it != factory_data->structural_data_view_instancers.end())
-			return it->second->InstanceView(element);
-	}
-	else
-	{
-		auto it = factory_data->data_view_instancers.find(type_name);
-		if (it != factory_data->data_view_instancers.end())
-			return it->second->InstanceView(element);
-	}
+	const auto it = factory_data->data_view_instancers.find(type_name);
+	if (it != factory_data->data_view_instancers.end())
+		return it->second->InstanceView(element);
 	return nullptr;
 }
 
 DataControllerPtr Factory::InstanceDataController(const String& type_name, Element* element)
 {
-	auto it = factory_data->data_controller_instancers.find(type_name);
+	const auto it = factory_data->data_controller_instancers.find(type_name);
 	if (it != factory_data->data_controller_instancers.end())
 		return it->second->InstanceController(element);
-	return DataControllerPtr();
+	return nullptr;
 }
 
 bool Factory::IsStructuralDataView(const String& type_name)
 {
-	return factory_data->structural_data_view_instancers.find(type_name) != factory_data->structural_data_view_instancers.end();
+	const String attribute = "data-" + type_name;
+	return factory_data->structural_data_view_attribute_names.find(attribute) != factory_data->structural_data_view_attribute_names.end();
 }
 
-const StringList& Factory::GetStructuralDataViewAttributeNames()
+const SmallUnorderedSet<String>& Factory::GetStructuralDataViewAttributeNames()
 {
 	return factory_data->structural_data_view_attribute_names;
 }

Source/Core/XMLNodeHandlerDefault.cpp

@@ -77,9 +77,9 @@ bool XMLNodeHandlerDefault::ElementData(XMLParser* parser, const String& data, X
 
 	if (type == XMLDataType::InnerXML)
 	{
-		// Structural data views use the raw inner xml contents of the node, submit them now.
-		if (ElementUtilities::ApplyStructuralDataViews(parent, data))
-			return true;
+		// Structural data views use the raw inner xml contents of the node, store them as an attribute to be processed by the data view.
+		parent->SetAttribute("rmlui-inner-rml", data);
+		return true;
 	}
 
 	// Parse the text into the element

Tests/Source/UnitTests/DataBinding.cpp

@@ -38,7 +38,7 @@ using namespace Rml;
 
 namespace {
 
-static const String document_rml = R"(
+static const String data_binding_rml = R"(
 <rml>
 <head>
 	<title>Test</title>
@@ -564,7 +564,7 @@ TEST_CASE("data_binding")
 
 	REQUIRE(InitializeDataBindings(context));
 
-	ElementDocument* document = context->LoadDocumentFromMemory(document_rml);
+	ElementDocument* document = context->LoadDocumentFromMemory(data_binding_rml);
 	REQUIRE(document);
 	document->Show();
 
@@ -710,3 +710,84 @@ TEST_CASE("data_binding.set_enum")
 	document->Close();
 	TestsShell::ShutdownShell();
 }
+
+static const String data_model_on_body_rml = R"(
+<rml>
+<head>
+	<title>Test</title>
+	<link type="text/rcss" href="/assets/rml.rcss"/>
+	<link type="text/rcss" href="/assets/invader.rcss"/>
+	<link type="text/template" href="/assets/window.rml"/>
+	<style>
+		body {
+			width: 500px;
+			height: 400px;
+			background: #ccc;
+			color: #333;
+		}
+	</style>
+</head>
+<body BODY_ATTRIBUTE>
+	<div DIV_ATTRIBUTE>
+		<div id="simple">{{ simple }}</div>
+		<div id="s2_val">{{ s2.val }}</div>
+		<div id="array_size">{{ arrays.a.size }}</div>
+		<div id="array_empty" data-attr-empty="arrays.a.size == 0"></div>
+		<div data-for="value, i : arrays.a">{{ i }}: {{ value }}</div>
+	</div>
+</body>
+</rml>
+)";
+
+TEST_CASE("data_binding.data_model_on_body")
+{
+	Context* context = TestsShell::GetContext();
+	REQUIRE(context);
+
+	REQUIRE(InitializeDataBindings(context));
+
+	const String data_model_attribute = R"( data-model="basics")";
+	const String template_attribute = R"( template="window")";
+
+	String document_rml;
+	SUBCASE("data_model_on_div")
+	{
+		document_rml = StringUtilities::Replace(data_model_on_body_rml, "BODY_ATTRIBUTE", "");
+		document_rml = StringUtilities::Replace(document_rml, "DIV_ATTRIBUTE", data_model_attribute);
+	}
+	SUBCASE("data_model_on_body")
+	{
+		document_rml = StringUtilities::Replace(data_model_on_body_rml, "BODY_ATTRIBUTE", data_model_attribute);
+		document_rml = StringUtilities::Replace(document_rml, "DIV_ATTRIBUTE", "");
+	}
+	SUBCASE("data_model_on_body_with_template")
+	{
+		document_rml = StringUtilities::Replace(data_model_on_body_rml, "BODY_ATTRIBUTE", data_model_attribute + template_attribute);
+		document_rml = StringUtilities::Replace(document_rml, "DIV_ATTRIBUTE", "");
+	}
+
+	ElementDocument* document = context->LoadDocumentFromMemory(document_rml);
+	REQUIRE(document);
+	document->Show();
+
+	TestsShell::RenderLoop();
+
+	CHECK(document->GetElementById("simple")->GetInnerRML() == Rml::ToString(int(globals.simple)));
+	CHECK(document->GetElementById("s2_val")->GetInnerRML() == globals.s2.val);
+
+	const Vector<int> array = Arrays{}.a;
+	CHECK(document->GetElementById("array_size")->GetInnerRML() == Rml::ToString(array.size()));
+	CHECK(document->GetElementById("array_empty")->GetAttribute<bool>("empty", true) == array.empty());
+
+	Element* element = document->GetElementById("array_empty")->GetNextSibling();
+	size_t i = 0;
+	for (; i < array.size() && element; ++i)
+	{
+		CHECK(element->GetInnerRML() == Rml::CreateString("%zu: %d", i, array[i]));
+		element = element->GetNextSibling();
+	}
+	CHECK(i == array.size());
+
+	document->Close();
+	TestsShell::ShutdownShell();
+}