Clear dirty state of hidden elements, and reset layout cache when context dimension changes

Source/Core/Context.cpp

@@ -41,6 +41,7 @@
 #include "../../Include/RmlUi/Core/SystemInterface.h"
 #include "DataModel.h"
 #include "EventDispatcher.h"
+#include "Layout/LayoutNode.h"
 #include "PluginRegistry.h"
 #include "ScrollController.h"
 #include "StreamFile.h"
@@ -163,6 +164,7 @@ void Context::SetDimensions(const Vector2i _dimensions)
 				document->DirtyVwAndVhProperties();
 				document->DirtyLayout();
 				document->DirtyPosition();
+				document->GetLayoutNode()->ClearCommittedLayout();
 				document->DispatchEvent(EventId::Resize, Dictionary());
 			}
 		}

Source/Core/Element.cpp

@@ -411,6 +411,7 @@ void Element::UpdateAbsoluteOffsetAndRenderBoxData()
 				offset_from_ancestors += ancestor->relative_offset_position;
 		}
 
+		// Absolute offset is defined by the border-box of the element.
 		const Vector2f relative_offset = relative_offset_base + relative_offset_position - GetBox().GetPosition(relative_offset_area);
 		absolute_offset = relative_offset + offset_from_ancestors;
 
@@ -3031,7 +3032,10 @@ void Element::ClampScrollOffset()
 void Element::CommitLayoutRecursive()
 {
 	if (GetDisplay() == Style::Display::None)
+	{
+		GetLayoutNode()->ClearDirty();
 		return;
+	}
 
 	OnLayout();
 

Source/Core/ElementDocument.cpp

@@ -532,10 +532,17 @@ void ElementDocument::UpdateLayout()
 		RMLUI_ZoneScoped;
 		RMLUI_ZoneText(source_url.c_str(), source_url.size());
 
+		constexpr bool debug_logging = false;
+		if (debug_logging)
+			Log::Message(Log::LT_INFO, "UpdateLayout start: %s", GetAddress().c_str());
+
 		bool force_full_document_layout = false;
 		bool any_layout_updates = false;
 
 		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;
@@ -548,7 +555,8 @@ void ElementDocument::UpdateLayout()
 				// 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.
-				Log::Message(Log::LT_INFO, "Dirty layout on non-layout boundary element: %s", element->GetAddress().c_str());
+				if (debug_logging)
+					Log::Message(Log::LT_INFO, "Dirty layout on non-layout boundary element: %s", element->GetAddress().c_str());
 				return ElementUtilities::CallbackControlFlow::Continue;
 			}
 
@@ -557,13 +565,15 @@ void ElementDocument::UpdateLayout()
 			const Optional<CommittedLayout>& committed_layout = layout_node->GetCommittedLayout();
 			if (!committed_layout)
 			{
-				if (element->GetOwnerDocument() != this)
-					Log::Message(Log::LT_INFO, "Forcing full layout update due to element: %s", element->GetAddress().c_str());
+				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->GetOwnerDocument() != this)
+			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.
@@ -587,12 +597,15 @@ void ElementDocument::UpdateLayout()
 			this->UpdateRelativeOffsetFromInsetConstraints();
 		}
 
-		if (!any_layout_updates)
+		if (!any_layout_updates && debug_logging)
 			Log::Message(Log::LT_INFO, "Didn't make any layout updates on dirty document: %s", GetAddress().c_str());
 
 		// Ignore dirtied layout during document formatting. Layouting must not require re-iteration.
 		// In particular, scrollbars being enabled may set the dirty flag, but this case is already handled within the layout engine.
 		layout_dirty = false;
+
+		if (debug_logging)
+			Log::Message(Log::LT_INFO, "Document layout: Clear dirty on %s", GetAddress().c_str());
 	}
 }
 
@@ -644,6 +657,7 @@ void ElementDocument::DirtyPosition()
 
 void ElementDocument::DirtyDocumentLayout()
 {
+	// Log::Message(Log::LT_INFO, "DirtyDocumentLayout: %s", GetAddress().c_str());
 	layout_dirty = true;
 }
 

Source/Core/Layout/LayoutNode.cpp

@@ -39,11 +39,24 @@ void LayoutNode::DirtyUpToClosestLayoutBoundary()
 	if (!IsSelfDirty())
 		return;
 
-	// Later on, we might expand the definition of layout boundaries from just absolutely positioned elements. For
-	// example, for flexible boxes, we can make them conditional, in the sense that its parent layout boundary needs to
-	// be layed-out again if (and only if) the flex container changes size after its next layout.
-	if (IsLayoutBoundary())
-		return;
+	// 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())
 	{
@@ -54,15 +67,56 @@ void LayoutNode::DirtyUpToClosestLayoutBoundary()
 	}
 }
 
+void LayoutNode::ClearDirty()
+{
+	// Log::Message(Log::LT_INFO, "ClearDirty (was Self %d  Child %d)  Element: %s", (dirty_flag & DirtyLayoutType::DOM) != DirtyLayoutType::None,
+	//	(dirty_flag & DirtyLayoutType::Child) != DirtyLayoutType::None, element->GetAddress().c_str());
+	dirty_flag = DirtyLayoutType::None;
+}
+
+void LayoutNode::SetDirty(DirtyLayoutType dirty_type)
+{
+	// Log::Message(Log::LT_INFO, "SetDirty. Self %d  Child %d  Element: %s", (dirty_type & DirtyLayoutType::DOM) != DirtyLayoutType::None,
+	//	(dirty_type & DirtyLayoutType::Child) != DirtyLayoutType::None, element->GetAddress().c_str());
+	dirty_flag = dirty_flag | dirty_type;
+}
+
+void LayoutNode::CommitLayout(Vector2f containing_block_size, Vector2f absolutely_positioning_containing_block_size, const Box* override_box,
+	Vector2f visible_overflow_size, Optional<float> baseline_of_last_line)
+{
+	// TODO: This is mixing slightly different concepts. Rather, it might be advantageous to separate what is the input
+	// to the layout of the current element, and what is the output. That way we can e.g. set the containing block size
+	// even if there is nothing to format (for example due to `display: none`), or if the element itself can be cached
+	// despite ancestor changes. This way we can resume the layout here, without formatting its ancestors, if it is
+	// dirtied in a non-parent-mutable way.
+	// - E.g. consider scenario 2 above with Absolute element `display: none` to `display: block`.
+	//
+	// Conversely, the output of the layout passed in here can later be used by when formatting ancestors, when the
+	// current element does not need a new layout by itself.
+	committed_layout.emplace(CommittedLayout{
+		containing_block_size,
+		absolutely_positioning_containing_block_size,
+		override_box ? Optional<Box>(*override_box) : Optional<Box>(),
+		visible_overflow_size,
+		baseline_of_last_line,
+	});
+	ClearDirty();
+}
+
 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
+	// maybe it's okay?
+	if (computed.display() == Display::None)
+		return true;
+
 	const FormattingContextType formatting_context = FormattingContext::GetFormattingContextType(element);
 	if (formatting_context == FormattingContextType::None)
 		return false;
 
-	using namespace Style;
-	auto& computed = element->GetComputedValues();
-
 	if (computed.position() == Position::Absolute || computed.position() == Position::Fixed)
 		return true;
 

Source/Core/Layout/LayoutNode.h

@@ -75,26 +75,22 @@ public:
 
 	LayoutNode(Element* element) : element(element) {}
 
+	// Assumes valid computed values.
 	void DirtyUpToClosestLayoutBoundary();
 
-	void ClearDirty() { dirty_flag = DirtyLayoutType::None; }
-	void SetDirty(DirtyLayoutType dirty_type) { dirty_flag = dirty_flag | dirty_type; }
+	void ClearDirty();
+	void SetDirty(DirtyLayoutType dirty_type);
 
+	bool IsChildDirty() const { return (dirty_flag & DirtyLayoutType::Child) != DirtyLayoutType::None; }
 	bool IsDirty() const { return dirty_flag != DirtyLayoutType::None; }
 	bool IsSelfDirty() const { return !(dirty_flag == DirtyLayoutType::None || dirty_flag == DirtyLayoutType::Child); }
 
 	void CommitLayout(Vector2f containing_block_size, Vector2f absolutely_positioning_containing_block_size, const Box* override_box,
-		Vector2f visible_overflow_size, Optional<float> baseline_of_last_line)
-	{
-		committed_layout.emplace(CommittedLayout{
-			containing_block_size,
-			absolutely_positioning_containing_block_size,
-			override_box ? Optional<Box>(*override_box) : Optional<Box>(),
-			visible_overflow_size,
-			baseline_of_last_line,
-		});
-		ClearDirty();
-	}
+		Vector2f visible_overflow_size, Optional<float> baseline_of_last_line);
+
+	// TODO: Currently, should only be used by `Context::SetDimensions`. Ideally, we would remove this and "commit" the
+	// containing block (without the layout result, see comment in `CommitLayout`).
+	void ClearCommittedLayout() { committed_layout.reset(); }
 
 	bool CommittedLayoutMatches(Vector2f containing_block_size, Vector2f absolutely_positioning_containing_block_size, const Box* override_box) const
 	{

Tests/Source/UnitTests/LayoutIsolation.cpp

@@ -30,12 +30,14 @@
 #include "../../../Source/Core/Layout/LayoutNode.h"
 #include "../Common/TestsShell.h"
 #include "../Common/TypesToString.h"
+#include <RmlUi/Core/ComputedValues.h>
 #include <RmlUi/Core/Context.h>
 #include <RmlUi/Core/Core.h>
 #include <RmlUi/Core/Element.h>
 #include <RmlUi/Core/ElementDocument.h>
 #include <RmlUi/Core/ElementText.h>
 #include <RmlUi/Core/ElementUtilities.h>
+#include <RmlUi/Debugger/Debugger.h>
 #include <doctest.h>
 
 using namespace Rml;
@@ -88,8 +90,8 @@ static void LogDirtyLayoutTree(Element* root_element)
 	String tree_dirty_state;
 	ElementUtilities::VisitElementsDepthOrder(root_element, [&](Element* element, int tree_depth) {
 		tree_dirty_state += String(size_t(4 * tree_depth), ' ');
-		tree_dirty_state += CreateString("%s: Dirty: %d  Dirty Self: %d", element->GetAddress(false, tree_depth == 0).c_str(),
-			element->GetLayoutNode()->IsDirty(), element->GetLayoutNode()->IsSelfDirty());
+		tree_dirty_state += CreateString("%s.  Self: %d  Child: %d", element->GetAddress(false, tree_depth == 0).c_str(),
+			element->GetLayoutNode()->IsSelfDirty(), element->GetLayoutNode()->IsChildDirty());
 		tree_dirty_state += '\n';
 	});
 
@@ -376,8 +378,10 @@ TEST_CASE("LayoutIsolation.Absolute")
 
 		CHECK(container->GetOffsetWidth() != container_initial_width);
 		CHECK(element->GetOffsetWidth() != element_initial_width);
-		CHECK(format_independent_tracker.CountEntries() == 1);
-		CHECK(format_independent_tracker.CountFormattedEntries() == 1);
+		// The following could in principle be reduced to 1, since the size of an absolute element should not affect the
+		// layout of the formatting context it takes part in.
+		CHECK(format_independent_tracker.CountEntries() == 2);
+		CHECK(format_independent_tracker.CountFormattedEntries() == 2);
 	}
 
 	SUBCASE("Modify document width")
@@ -435,4 +439,194 @@ TEST_CASE("LayoutIsolation.Absolute")
 	TestsShell::ShutdownShell();
 }
 
+static const String layout_isolation_hidden_absolute_rml = R"(
+<rml>
+<head>
+	<title>Demo</title>
+	<link type="text/rcss" href="/assets/rml.rcss"/>
+	<style>
+		body {
+			font-family: LatoLatin;
+			font-size: 14px;
+			border: 5px #a66;
+			background: #cca;
+			color: black;
+
+			top: 200px;
+			right: 300px;
+			bottom: 200px;
+			left: 300px;
+		}
+		div {
+			width: 300px;
+			height: 200px;
+			left: 100px;
+			top: 100px;
+		}
+		.hide {
+			display: none;
+		}
+		.absolute {
+			position: absolute;
+		}
+		.wide {
+			width: 400px;
+		}
+	</style>
+</head>
+<body id="body">
+	This is a sample.
+	<div id="child">Child element</div>
+</body>
+</rml>
+)";
+
+TEST_CASE("LayoutIsolation.HiddenSkipsFormatting")
+{
+	Context* context = TestsShell::GetContext();
+
+	ElementDocument* document = context->LoadDocumentFromMemory(layout_isolation_hidden_absolute_rml);
+	Element* element = document->GetElementById("child");
+	element->SetClass("hide", true);
+
+	SUBCASE("Static") {}
+	SUBCASE("Absolute")
+	{
+		element->SetClass("absolute", true);
+	}
+
+	FormatIndependentDebugTracker format_independent_tracker;
+	document->Show();
+	TestsShell::RenderLoop();
+	CHECK(format_independent_tracker.CountFormattedEntries() == 1);
+
+	rmlui_dynamic_cast<ElementText*>(element->GetFirstChild())->SetText("Modified text");
+	CHECK(format_independent_tracker.CountFormattedEntries() == 1);
+
+	// Modifying text in a hidden element should not trigger a new layout.
+	TestsShell::RenderLoop();
+	CHECK(format_independent_tracker.CountFormattedEntries() == 1);
+
+	format_independent_tracker.LogMessage();
+
+	document->Close();
+	TestsShell::ShutdownShell();
+}
+
+TEST_CASE("LayoutIsolation.HiddenToggleAndModify")
+{
+	Context* context = TestsShell::GetContext();
+
+	ElementDocument* document = context->LoadDocumentFromMemory(layout_isolation_hidden_absolute_rml);
+	Element* element = document->GetElementById("child");
+	element->SetClass("hide", true);
+
+	float element_offset_left = 0;
+	SUBCASE("Static")
+	{
+		element_offset_left = 305;
+	}
+	SUBCASE("Absolute")
+	{
+		element->SetClass("absolute", true);
+		element_offset_left = 405;
+	}
+
+	document->Show();
+	TestsShell::RenderLoop();
+	CHECK(element->IsVisible() == false);
+
+	element->SetClass("hide", false);
+	document->UpdatePropertiesForDebug();
+	TestsShell::RenderLoop();
+	CHECK(element->GetAbsoluteLeft() == element_offset_left);
+	CHECK(element->IsVisible() == true);
+	CHECK(element->GetComputedValues().width().value == 300);
+	CHECK(element->GetOffsetWidth() == 300.f);
+
+	element->SetClass("hide", true);
+	document->UpdatePropertiesForDebug();
+	TestsShell::RenderLoop();
+	CHECK(element->IsVisible() == false);
+	CHECK(element->GetComputedValues().width().value == 300);
+	CHECK(element->GetOffsetWidth() == 300.f);
+
+	element->SetClass("wide", true);
+	document->UpdatePropertiesForDebug();
+	TestsShell::RenderLoop();
+	CHECK(element->IsVisible() == false);
+	CHECK(element->GetComputedValues().width().value == 400);
+	CHECK(element->GetOffsetWidth() == 300.f);
+
+	element->SetClass("hide", false);
+	document->UpdatePropertiesForDebug();
+	TestsShell::RenderLoop();
+	CHECK(element->IsVisible() == true);
+	CHECK(element->GetComputedValues().width().value == 400);
+	CHECK(element->GetOffsetWidth() == 400.f);
+
+	document->Close();
+	TestsShell::ShutdownShell();
+}
+
+static const String layout_isolation_document_rml = R"(
+<rml>
+<head>
+	<title>Demo</title>
+	<link type="text/rcss" href="/assets/rml.rcss"/>
+	<style>
+		body {
+			font-family: LatoLatin;
+			font-size: 14px;
+			border: 5px #a66;
+			background: #cca;
+			color: black;
+
+			top: 200px;
+			right: 300px;
+			bottom: 200px;
+			left: 300px;
+		}
+	</style>
+</head>
+<body>
+	This is a sample.
+</body>
+</rml>
+)";
+
+TEST_CASE("LayoutIsolation.Document")
+{
+	Context* context = TestsShell::GetContext();
+	REQUIRE(context->GetDimensions() == Rml::Vector2i{1500, 800});
+
+	FormatIndependentDebugTracker format_independent_tracker;
+
+	ElementDocument* document = context->LoadDocumentFromMemory(layout_isolation_document_rml);
+	document->Show();
+
+	TestsShell::RenderLoop();
+
+	CHECK(document->GetOffsetWidth() == 900.f);
+	CHECK(document->GetOffsetHeight() == 400.f);
+
+	SUBCASE("Modify absolute content")
+	{
+		context->SetDimensions(Rml::Vector2i{1600, 900});
+
+		document->UpdatePropertiesForDebug();
+		TestsShell::RenderLoop();
+
+		CHECK(document->GetOffsetWidth() == 1000.f);
+		CHECK(document->GetOffsetHeight() == 500.f);
+
+		CHECK(format_independent_tracker.CountFormattedEntries() == 2);
+	}
+
+	format_independent_tracker.LogMessage();
+
+	document->Close();
+	TestsShell::ShutdownShell();
+}
+
 #endif // RMLUI_DEBUG