LayoutNode: Skip non-DOM elements, refactor to propagate dirty only to its parent

Fix an issue with caching not always being disabled with attribute present.

Include/RmlUi/Core/Element.h

@@ -619,7 +619,7 @@ public:
 	const ComputedValues& GetComputedValues() const;
 
 protected:
-	void Update(float dp_ratio, Vector2f vp_dimensions);
+	void Update(float dp_ratio, Vector2f vp_dimensions, bool is_dom_element);
 	void Render();
 
 	/// Updates definition, computed values, and runs OnPropertyChange on this element.

Source/Core/Context.cpp

@@ -223,7 +223,7 @@ bool Context::Update()
 	root->dirty_definition = false;
 	root->dirty_child_definitions = false;
 
-	root->Update(density_independent_pixel_ratio, Vector2f(dimensions));
+	root->Update(density_independent_pixel_ratio, Vector2f(dimensions), true);
 
 	for (int i = 0; i < root->GetNumChildren(); ++i)
 	{

Source/Core/Element.cpp

@@ -139,7 +139,7 @@ Element::~Element()
 	ElementMetaPool::element_meta_pool->pool.DestroyAndDeallocate(meta);
 }
 
-void Element::Update(float dp_ratio, Vector2f vp_dimensions)
+void Element::Update(float dp_ratio, Vector2f vp_dimensions, bool is_dom_element)
 {
 #ifdef RMLUI_TRACY_PROFILING
 	auto name = GetAddress(false, false);
@@ -167,8 +167,11 @@ void Element::Update(float dp_ratio, Vector2f vp_dimensions)
 
 	meta->effects.InstanceEffects();
 
-	for (size_t i = 0; i < children.size(); i++)
-		children[i]->Update(dp_ratio, vp_dimensions);
+	for (int i = 0; i < (int)children.size(); i++)
+	{
+		const bool child_is_dom_element = (is_dom_element && i < (int)children.size() - num_non_dom_children);
+		children[i]->Update(dp_ratio, vp_dimensions, child_is_dom_element);
+	}
 
 	if (!animations.empty() && IsVisible(true))
 	{
@@ -176,8 +179,13 @@ void Element::Update(float dp_ratio, Vector2f vp_dimensions)
 			ctx->RequestNextUpdate(0);
 	}
 
-	LayoutNode* layout_node = GetLayoutNode();
-	layout_node->DirtyUpToClosestLayoutBoundary();
+	// TODO: Don't think we can always skip non-DOM elements. E.g. for some input elements, we at least need to dirty
+	// the input element itself to format its contents again. This may even change itself in such a way as to need to
+	// format the layout it partakes in too. Regardless, we should at least make sure that enabling and disabling
+	// scrollbars during layouting does not cause dirtying their element. There should also be a symmetry to commiting
+	// the layout.
+	if (is_dom_element)
+		GetLayoutNode()->PropagateDirtyToParent();
 }
 
 void Element::UpdateProperties(const float dp_ratio, const Vector2f vp_dimensions)

Source/Core/ElementDocument.cpp

@@ -511,7 +511,7 @@ void ElementDocument::UpdateDocument()
 {
 	const float dp_ratio = (context ? context->GetDensityIndependentPixelRatio() : 1.0f);
 	const Vector2f vp_dimensions = (context ? Vector2f(context->GetDimensions()) : Vector2f(1.0f));
-	Update(dp_ratio, vp_dimensions);
+	Update(dp_ratio, vp_dimensions, true);
 	UpdateLayout();
 	UpdatePosition();
 }
@@ -520,7 +520,7 @@ void ElementDocument::UpdatePropertiesForDebug()
 {
 	const float dp_ratio = (context ? context->GetDensityIndependentPixelRatio() : 1.0f);
 	const Vector2f vp_dimensions = (context ? Vector2f(context->GetDimensions()) : Vector2f(1.0f));
-	Update(dp_ratio, vp_dimensions);
+	Update(dp_ratio, vp_dimensions, true);
 }
 
 void ElementDocument::UpdateLayout()
@@ -538,55 +538,58 @@ void ElementDocument::UpdateLayout()
 		if (debug_logging)
 			Log::Message(Log::LT_INFO, "UpdateLayout start: %s", GetAddress().c_str());
 
-		bool force_full_document_layout = false;
+		bool force_full_document_layout = !allow_layout_cache;
 		bool any_layout_updates = false;
 
-		ElementUtilities::BreadthFirstSearch(this, [&](Element* element) {
-			if (element->GetDisplay() == Style::Display::None)
-				return ElementUtilities::CallbackControlFlow::SkipChildren;
+		if (!force_full_document_layout)
+		{
+			ElementUtilities::BreadthFirstSearch(this, [&](Element* element) {
+				if (element->GetDisplay() == Style::Display::None)
+					return ElementUtilities::CallbackControlFlow::SkipChildren;
 
-			LayoutNode* layout_node = element->GetLayoutNode();
-			if (!layout_node->IsDirty())
-				return ElementUtilities::CallbackControlFlow::Continue;
+				LayoutNode* layout_node = element->GetLayoutNode();
+				if (!layout_node->IsDirty())
+					return ElementUtilities::CallbackControlFlow::Continue;
 
-			if (!layout_node->IsLayoutBoundary())
-			{
-				// Normally the dirty layout should have propagated to the closest layout boundary during
-				// Element::Update(), which then invokes formatting on that boundary element, and which again clears the
-				// dirty layout on this element. This didn't happen here, which may be caused by modifying the layout
-				// during layouting itself. For now, we simply skip this element and keep going. The element should
-				// normally be properly layed-out on the next context update.
-				// TODO: Might want to investigate this further.
-				if (debug_logging)
-					Log::Message(Log::LT_INFO, "Dirty layout on non-layout boundary element: %s", element->GetAddress().c_str());
-				return ElementUtilities::CallbackControlFlow::Continue;
-			}
+				if (!layout_node->IsLayoutBoundary())
+				{
+					// Normally the dirty layout should have propagated to the closest layout boundary during
+					// Element::Update(), which then invokes formatting on that boundary element, and which again clears the
+					// dirty layout on this element. This didn't happen here, which may be caused by modifying the layout
+					// during layouting itself. For now, we simply skip this element and keep going. The element should
+					// normally be properly layed-out on the next context update.
+					// TODO: Might want to investigate this further.
+					if (debug_logging)
+						Log::Message(Log::LT_INFO, "Dirty layout on non-layout boundary element: %s", element->GetAddress().c_str());
+					return ElementUtilities::CallbackControlFlow::Continue;
+				}
 
-			any_layout_updates = true;
+				any_layout_updates = true;
 
-			const Optional<CommittedLayout>& committed_layout = layout_node->GetCommittedLayout();
-			if (!committed_layout)
-			{
-				if (element != this && debug_logging)
-					Log::Message(Log::LT_INFO, "Forcing full layout update due to missing committed layout on element: %s",
-						element->GetAddress().c_str());
-				force_full_document_layout = true;
-				// TODO: When is the committed layout dirtied?
-				return ElementUtilities::CallbackControlFlow::Break;
-			}
+				const Optional<CommittedLayout>& committed_layout = layout_node->GetCommittedLayout();
+				if (!committed_layout)
+				{
+					if (element != this && debug_logging)
+						Log::Message(Log::LT_INFO, "Forcing full layout update due to missing committed layout on element: %s",
+							element->GetAddress().c_str());
+					force_full_document_layout = true;
+					// TODO: When is the committed layout dirtied?
+					return ElementUtilities::CallbackControlFlow::Break;
+				}
 
-			if (element != this && debug_logging)
-				Log::Message(Log::LT_INFO, "Doing partial layout update on element: %s", element->GetAddress().c_str());
+				if (element != this && debug_logging)
+					Log::Message(Log::LT_INFO, "Doing partial layout update on element: %s", element->GetAddress().c_str());
 
-			// TODO: In some cases, we need to check if size changed, such that we need to do a layout update in its parent.
-			LayoutEngine::FormatElement(element, committed_layout->containing_block_size,
-				committed_layout->absolutely_positioning_containing_block_size, allow_layout_cache);
+				// TODO: In some cases, we need to check if size changed, such that we need to do a layout update in its parent.
+				LayoutEngine::FormatElement(element, committed_layout->containing_block_size,
+					committed_layout->absolutely_positioning_containing_block_size, allow_layout_cache);
 
-			// TODO: A bit ugly
-			element->UpdateRelativeOffsetFromInsetConstraints();
+				// TODO: A bit ugly
+				element->UpdateRelativeOffsetFromInsetConstraints();
 
-			return ElementUtilities::CallbackControlFlow::SkipChildren;
-		});
+				return ElementUtilities::CallbackControlFlow::SkipChildren;
+			});
+		}
 
 		if (force_full_document_layout)
 		{

Source/Core/ElementScroll.cpp

@@ -259,7 +259,7 @@ void ElementScroll::UpdateScrollElementProperties(Element* scroll_element)
 
 	const float dp_ratio = (context ? context->GetDensityIndependentPixelRatio() : 1.0f);
 	const Vector2f vp_dimensions = (context ? Vector2f(context->GetDimensions()) : Vector2f(1.0f));
-	scroll_element->Update(dp_ratio, vp_dimensions);
+	scroll_element->Update(dp_ratio, vp_dimensions, false);
 }
 
 ElementScroll::Scrollbar::Scrollbar() {}

Source/Core/Layout/LayoutNode.cpp

@@ -34,36 +34,42 @@
 
 namespace Rml {
 
-void LayoutNode::DirtyUpToClosestLayoutBoundary()
+void LayoutNode::PropagateDirtyToParent()
 {
-	if (!IsSelfDirty())
-		return;
+	auto DirtyParentNode = [](Element* element) {
+		if (Element* parent = element->GetParentNode())
+			parent->GetLayoutNode()->SetDirty(DirtyLayoutType::Child);
+	};
 
-	// We may be able to skip formatting in ancestor elements if this is a layout boundary, in some scenarios. Consider
-	// some scenarios for illustration:
-	//
-	// 1. Absolute element. `display: block` to `display: none`. This does not need a new layout. Same with margin and
-	//    size, only the current element need to be reformatted, not ancestors.
-	// 2. Absolute element. `display: none` to `display: block`. This *does* need to be layed out, since we don't know
-	//    our static position or containing block. We could in principle ignore static position in some situations where
-	//    it is not used, and could in principle find our containing block. But it is tricky.
-	// 3. Flex container contents changed. If (and only if) it results in a new layed out size, its parent needs to be
-	//    reformatted again. If so, it should be able to reuse the flex container's layout cache.
-	//
-	// Currently, we don't have all of this information here. So skip this for now.
-	// - TODO: This information could be provided as part of DirtyLayout.
-	// - TODO: Consider if some of this logic should be moved to the layout engine.
-	//
-	// ```
-	//     if (IsLayoutBoundary()) return;
-	// ```
+	if (IsSelfDirty())
+	{
+		// We may be able to skip formatting in ancestor elements if this is a layout boundary, in some scenarios. Consider
+		// some scenarios for illustration:
+		//
+		// 1. Absolute element. `display: block` to `display: none`. This does not need a new layout. Same with margin and
+		//    size, only the current element need to be reformatted, not ancestors.
+		// 2. Absolute element. `display: none` to `display: block`. This *does* need to be layed out, since we don't know
+		//    our static position or containing block. We could in principle ignore static position in some situations where
+		//    it is not used, and could in principle find our containing block. But it is tricky.
+		// 3. Flex container contents changed. If (and only if) it results in a new layed out size, its parent needs to be
+		//    reformatted again. If so, it should be able to reuse the flex container's layout cache.
+		//
+		// Currently, we don't have all of this information here. So skip this for now.
+		// - TODO: This information could be provided as part of DirtyLayout.
+		// - TODO: Consider if some of this logic should be moved to the layout engine.
+		//
+		// ```
+		//     if (IsLayoutBoundary()) return;
+		// ```
 
-	for (Element* parent = element->GetParentNode(); parent; parent = parent->GetParentNode())
+		DirtyParentNode(element);
+		return;
+	}
+
+	if (IsChildDirty() && !IsLayoutBoundary())
 	{
-		LayoutNode* parent_node = parent->GetLayoutNode();
-		parent_node->SetDirty(DirtyLayoutType::Child);
-		if (parent_node->IsLayoutBoundary())
-			break;
+		DirtyParentNode(element);
+		return;
 	}
 }
 
@@ -111,7 +117,7 @@ bool LayoutNode::IsLayoutBoundary() const
 	using namespace Style;
 	auto& computed = element->GetComputedValues();
 
-	// TODO: Should this be moved into DirtyUpToClosestLayoutBoundary() instead? It's not really a layout boundary, or
+	// TODO: Should this be moved into PropagateDirtyToParent() instead? It's not really a layout boundary, or
 	// maybe it's okay?
 	if (computed.display() == Display::None)
 		return true;

Source/Core/Layout/LayoutNode.h

@@ -77,7 +77,7 @@ public:
 	LayoutNode(Element* element) : element(element) {}
 
 	// Assumes valid computed values.
-	void DirtyUpToClosestLayoutBoundary();
+	void PropagateDirtyToParent();
 
 	void ClearDirty();
 	void SetDirty(DirtyLayoutType dirty_type);