Revert "Skip formatting when possible after FormatFitContentWidth"

This reverts commit 1f462b054d433c16ceda11be7d3643ca554518be.

This introduced some problems with explicit heights combined with fit-content width. The problem relates to setting an indefinite height during max-content width layouting, leading to the layouting also having indefinite height in the end. Revisit later if it's still needed in the end.

Source/Core/Layout/FlexFormattingContext.cpp

@@ -52,7 +52,7 @@ UniquePtr<LayoutBox> FlexFormattingContext::Format(ContainerBox* parent_containe
 	return FlexFormattingContext::FormatImpl(parent_container, element, initial_box, flex_min_size, flex_max_size);
 }
 
-UniquePtr<LayoutBox> FlexFormattingContext::DetermineMaxContentWidth(Element* element, const Box& initial_box, const FormattingMode& formatting_mode)
+float FlexFormattingContext::DetermineMaxContentWidth(Element* element, const Box& initial_box, const FormattingMode& formatting_mode)
 {
 	RMLUI_ASSERT(formatting_mode.constraint == FormattingMode::Constraint::MaxContent);
 	const Vector2f containing_block(-1.f);
@@ -60,12 +60,9 @@ UniquePtr<LayoutBox> FlexFormattingContext::DetermineMaxContentWidth(Element* el
 
 	const Vector2f min_flex_size(0.f);
 	const Vector2f max_flex_size(FLT_MAX);
+	UniquePtr<LayoutBox> layout_box = FlexFormattingContext::FormatImpl(&root, element, initial_box, min_flex_size, max_flex_size);
 
-	// Return the layout box if (and only if) we can consider the layout complete, despite formatting under a
-	// max-content constraint. This allows us to skip a formatting step in some cases. Later we may want to make some
-	// formatting simplifications during max-content sizing, optimized just for retrieving its width. In this case, we
-	// should make sure that the full flex formatting is done at least once during layouting in the end.
-	return FlexFormattingContext::FormatImpl(&root, element, initial_box, min_flex_size, max_flex_size);
+	return layout_box->GetShrinkToFitWidth();
 }
 
 UniquePtr<LayoutBox> FlexFormattingContext::FormatImpl(ContainerBox* parent_container, Element* element, const Box& initial_box,

Source/Core/Layout/FlexFormattingContext.h

@@ -48,7 +48,7 @@ public:
 	static UniquePtr<LayoutBox> Format(ContainerBox* parent_container, Element* element, Vector2f containing_block, const Box& initial_box);
 
 	/// Formats a flex container to determine its max-content width.
-	static UniquePtr<LayoutBox> DetermineMaxContentWidth(Element* element, const Box& initial_box, const FormattingMode& formatting_mode);
+	static float DetermineMaxContentWidth(Element* element, const Box& initial_box, const FormattingMode& formatting_mode);
 
 private:
 	static UniquePtr<LayoutBox> FormatImpl(ContainerBox* parent_container, Element* element, const Box& initial_box, Vector2f flex_min_size,

Source/Core/Layout/FormattingContext.cpp

@@ -119,7 +119,7 @@ UniquePtr<LayoutBox> FormattingContext::FormatIndependent(ContainerBox* parent_c
 	UniquePtr<LayoutBox> layout_box;
 
 	const FormattingMode& formatting_mode = parent_container->GetFormattingMode();
-	if (formatting_mode.constraint == FormattingMode::Constraint::None)
+	if (type != FormattingContextType::None && formatting_mode.constraint == FormattingMode::Constraint::None)
 	{
 		LayoutNode* layout_node = element->GetLayoutNode();
 		if (layout_node->CommittedLayoutMatches(parent_container->GetContainingBlockSize(Style::Position::Static),
@@ -154,33 +154,21 @@ UniquePtr<LayoutBox> FormattingContext::FormatIndependent(ContainerBox* parent_c
 
 			if (box.GetSize().x < 0.f)
 			{
-				layout_box = FormatFitContentWidth(box, element, type, formatting_mode, containing_block);
-				if (layout_box)
-				{
-					Box box_before = box;
-					LayoutDetails::ClampSizeAndBuildAutoMarginsForBlockWidth(box, containing_block, element);
-					// If the box is changed after clamping, we need to re-format the element with the new size, ignoring the returned layout box.
-					if (box != box_before)
-						layout_box.reset();
-				}
-				else
-				{
-					LayoutDetails::ClampSizeAndBuildAutoMarginsForBlockWidth(box, containing_block, element);
-				}
+				FormatFitContentWidth(box, element, type, formatting_mode, containing_block);
+				LayoutDetails::ClampSizeAndBuildAutoMarginsForBlockWidth(box, containing_block, element);
 			}
 		}
 
-		if (!layout_box)
+		switch (type)
 		{
-			switch (type)
-			{
-			case FormattingContextType::Block: layout_box = BlockFormattingContext::Format(parent_container, element, containing_block, box); break;
-			case FormattingContextType::Table: layout_box = TableFormattingContext::Format(parent_container, element, containing_block, box); break;
-			case FormattingContextType::Flex: layout_box = FlexFormattingContext::Format(parent_container, element, containing_block, box); break;
-			case FormattingContextType::Replaced: layout_box = ReplacedFormattingContext::Format(parent_container, element, box); break;
-			case FormattingContextType::None: RMLUI_ERROR; break;
-			}
+		case FormattingContextType::Block: layout_box = BlockFormattingContext::Format(parent_container, element, containing_block, box); break;
+		case FormattingContextType::Table: layout_box = TableFormattingContext::Format(parent_container, element, containing_block, box); break;
+		case FormattingContextType::Flex: layout_box = FlexFormattingContext::Format(parent_container, element, containing_block, box); break;
+		case FormattingContextType::Replaced: layout_box = ReplacedFormattingContext::Format(parent_container, element, box); break;
+		case FormattingContextType::None: RMLUI_ERROR; break;
 		}
+
+		// TODO: If MaxContent constraint, and containing block = -1, -1, store resulting size as max-content size.
 	}
 
 	if (layout_box && formatting_mode.constraint == FormattingMode::Constraint::None)
@@ -233,8 +221,8 @@ float FormattingContext::FormatFitContentWidth(ContainerBox* parent_container, E
 	return box.GetSize().x;
 }
 
-UniquePtr<LayoutBox> FormattingContext::FormatFitContentWidth(Box& box, Element* element, FormattingContextType type,
-	const FormattingMode& parent_formatting_mode, const Vector2f containing_block)
+void FormattingContext::FormatFitContentWidth(Box& box, Element* element, FormattingContextType type, const FormattingMode& parent_formatting_mode,
+	const Vector2f containing_block)
 {
 	RMLUI_ASSERT(element);
 #ifdef RMLUI_TRACY_PROFILING
@@ -244,12 +232,15 @@ UniquePtr<LayoutBox> FormattingContext::FormatFitContentWidth(Box& box, Element*
 	RMLUI_ZoneText(zone_text.c_str(), zone_text.size());
 #endif
 
-	const float box_height = box.GetSize().y;
 	LayoutNode* layout_node = element->GetLayoutNode();
 
-	UniquePtr<LayoutBox> layout_box;
-	float max_content_width = -1.f;
+	const float box_height = box.GetSize().y;
 
+	float max_content_width = -1.f;
+	// TODO: The shrink-to-fit width is only cached for every other nested flexbox during the initial
+	// GetShrinkToFitWidth. I.e. the first .outer flexbox below #nested is formatted outside of this function. Even
+	// though in principle I believe we should be able to store its formatted width. Maybe move this caching into
+	// FormatIndependent somehow?
 	if (Optional<float> cached_width = layout_node->GetMaxContentWidthIfCached())
 	{
 		max_content_width = *cached_width;
@@ -264,28 +255,15 @@ UniquePtr<LayoutBox> FormattingContext::FormatFitContentWidth(Box& box, Element*
 
 		switch (type)
 		{
-		case FormattingContextType::Block:
-		{
-			max_content_width = BlockFormattingContext::DetermineMaxContentWidth(element, box, formatting_mode);
-		}
-		break;
-		case FormattingContextType::Flex:
-		{
-			layout_box = FlexFormattingContext::DetermineMaxContentWidth(element, box, formatting_mode);
-			max_content_width = layout_box->GetShrinkToFitWidth();
-		}
-		break;
+		case FormattingContextType::Block: max_content_width = BlockFormattingContext::DetermineMaxContentWidth(element, box, formatting_mode); break;
 		case FormattingContextType::Table:
-		{
 			// Currently we don't support shrink-to-fit width for tables, just use a zero-sized width.
 			max_content_width = 0.f;
-		}
-		break;
+			break;
+		case FormattingContextType::Flex: max_content_width = FlexFormattingContext::DetermineMaxContentWidth(element, box, formatting_mode); break;
 		case FormattingContextType::Replaced:
-		{
-			RMLUI_ERRORMSG("Replaced elements are expected to have a positive intrinsice size and should not need max-content formatting.");
-		}
-		break;
+			RMLUI_ERRORMSG("Replaced elements are expected to have a positive intrinsice size and do not perform shrink-to-fit layout.");
+			break;
 		case FormattingContextType::None: RMLUI_ERROR; break;
 		}
 
@@ -303,8 +281,6 @@ UniquePtr<LayoutBox> FormattingContext::FormatFitContentWidth(Box& box, Element*
 	}
 
 	box.SetContent(Vector2f(fit_content_width, box_height));
-
-	return layout_box;
 }
 
 } // namespace Rml

Source/Core/Layout/FormattingContext.h

@@ -76,11 +76,8 @@ protected:
 	~FormattingContext() = default;
 
 private:
-	/// Formats the element under a max-content constraint and sets its (unclamped) fit-content width on the box.
-	/// @return Optionally, a layout box indicating that the formatted element can be used directly when under
-	/// max-content constraints.
-	static UniquePtr<LayoutBox> FormatFitContentWidth(Box& box, Element* element, FormattingContextType type,
-		const FormattingMode& parent_formatting_mode, Vector2f containing_block);
+	static void FormatFitContentWidth(Box& box, Element* element, FormattingContextType type, const FormattingMode& parent_formatting_mode,
+		Vector2f containing_block);
 };
 
 } // namespace Rml

Tests/Source/Benchmarks/Flexbox.cpp

@@ -667,98 +667,3 @@ TEST_CASE("flexbox.shrink-to-fit")
 
 	document->Close();
 }
-
-static const String rml_flexbox_shrink_to_fit_super_nested = R"(
-<rml>
-<head>
-    <title>Flex - Shrink-to-fit 01</title>
-    <link type="text/rcss" href="/../Tests/Data/style.rcss"/>
-	<style>
-		body { width: 1000px; }
-		.shrink-to-fit {
-			float: left;
-			clear: both;
-			margin: 10px 0;
-			border: 2px #e8e8e8;
-		}
-		.outer {
-			border: 1px red;
-			padding: 30px;
-		}
-		#basic .outer {
-			display: flex;
-		}
-		#nested .outer {
-			display: inline-flex;
-		}
-		.inner {
-			border: 1px blue;
-			padding: 30px;
-		}
-	</style>
-</head>
-<body>
-<div id="nested" class="shrink-to-fit">
-	Before
-	<div class="outer">
-		<div class="inner">
-			<div class="outer">
-				<div class="inner">
-					<div class="outer">
-						<div class="inner">Flex</div>
-					</div>
-				</div>
-			</div>
-		</div>
-	</div>
-	After
-</div>
-</body>
-</rml>
-)";
-
-TEST_CASE("flexbox.shrink-to-fit-super-nested")
-{
-	Context* context = TestsShell::GetContext();
-	REQUIRE(context);
-
-	nanobench::Bench bench;
-	bench.title("Flexbox shrink-to-fit");
-	bench.relative(true);
-
-	ElementDocument* document = context->LoadDocumentFromMemory(rml_flexbox_shrink_to_fit_super_nested);
-	Element* basic = document->GetElementById("basic");
-	Element* nested = document->GetElementById("nested");
-
-	document->Show();
-	TestsShell::RenderLoop();
-
-	{
-		document->Close();
-		return;
-	}
-
-	basic->SetProperty(PropertyId::Display, Style::Display::None);
-	nested->SetProperty(PropertyId::Display, Style::Display::None);
-
-	bench.run("Reference", [&] {
-		document->SetProperty(PropertyId::Display, Style::Display::None);
-		document->RemoveProperty(PropertyId::Display);
-		context->Update();
-		context->Render();
-	});
-	bench.run("Basic shrink-to-fit", [&] {
-		basic->RemoveProperty(PropertyId::Display);
-		nested->SetProperty(PropertyId::Display, Style::Display::None);
-		context->Update();
-		context->Render();
-	});
-	bench.run("Nested shrink-to-fit", [&] {
-		basic->SetProperty(PropertyId::Display, Style::Display::None);
-		nested->RemoveProperty(PropertyId::Display);
-		context->Update();
-		context->Render();
-	});
-
-	document->Close();
-}