PALM HACKS #130

Louciole · 2025-11-19T15:17:50Z

No description provided.

…mputation to parent.

Copilot

Pull Request Overview

This PR implements border-collapse support for CSS tables along with significant refactoring of the layout system to properly handle box-sizing and spacing calculations.

Key changes:

Added full implementation of CSS table border-collapse algorithm with conflict resolution
Refactored layout functions to separate content box from border box layout with explicit UsedSpacings parameter
Fixed box-sizing handling to properly account for borders and padding in size calculations

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
src/vaev-engine/layout/table.cpp	Implements border-collapse algorithm with harmonization logic, refactors grid access from `get/set` to `at`, adds border resolution structures
src/vaev-engine/layout/base/layout-impl.cpp	Refactors layout functions to separate border box vs content box layout, adds `UsedSpacings` struct, fixes specified size calculations
src/vaev-engine/layout/base/layout.cpp	Updates function signatures to export new layout API with border/content box separation
src/vaev-engine/layout/block.cpp	Adapts block layout to use new `UsedSpacings` API and `layoutBorderBox`/`layoutAndCommitBorderBox` functions
src/vaev-engine/layout/flex.cpp	Updates flex layout to compute and pass `UsedSpacings` explicitly, fixes intrinsic size calculations
src/vaev-engine/layout/inline.cpp	Refactors inline atomic box layout to use new spacing API
src/vaev-engine/layout/paint.cpp	Adds support for painting collapsed borders via `UsedBorders` mapping from table formatting context
src/vaev-engine/layout/replaced.cpp	Simplifies SVG layout to use new border box layout functions
src/vaev-engine/values/borders.cpp	Adds template-based `_Border` struct to support both specified and used border values, adds `BorderEdge` enum
src/vaev-engine/res/html.css	Adds default table styling for box-sizing and border properties
tests/css/display/display-table.xhtml	Adds comprehensive tests for border-collapse with various scenarios (colorful borders, colspan/rowspan, col/row groups), fixes box-sizing in existing tests
tests/css/display/display-flex.xhtml	Adds test for flex intrinsic sizing with inline children
tests/css/display/display-block.xhtml	Adds test for intrinsic sizing across tree, fixes box-sizing in inline-block test, formatting improvements
tests/css/box-model/padding.xhtml	Updates expected sizes to account for box-sizing fixes
tests/css/box-model/box-sizing.xhtml	Enables previously skipped content-box test
tests/css/box-model/border.xhtml	Adds new test file for border-radius (has structural issue)
src/vaev-engine/driver/render.cpp	Updates to use renamed `layoutAndCommitRoot` function
src/vaev-engine/driver/print.cpp	Updates to use renamed `layoutAndCommitRoot` function

Comments suppressed due to low confidence (1)

src/vaev-engine/layout/base/layout-impl.cpp:216

The logic for handling IntrinsicSize::AUTO is inconsistent between width and height. For width (lines 161-187), intrinsic sizes are calculated and border box is added, but for height (lines 190-216), intrinsic sizes return NONE. This asymmetry should be documented or justified with a comment explaining why height treats intrinsic sizing differently.

Opt<Au> computeSpecifiedBorderBoxWidth(Tree& tree, Box& box, Size size, Vec2Au containingBlock, Au horizontalBorderBox, Opt<Au> capmin) {
    if (auto calc = size.is<CalcValue<PercentOr<Length>>>()) {
        auto specifiedWidth = resolve(tree, box, *calc, containingBlock.x);
        if (box.style->boxSizing == BoxSizing::CONTENT_BOX) {
            specifiedWidth += horizontalBorderBox;
        }
        return specifiedWidth;
    }

    if (size.is<Keywords::MinContent>()) {
        auto intrinsicSize = computeIntrinsicContentSize(tree, box, IntrinsicSize::MIN_CONTENT, capmin);
        return intrinsicSize.x + horizontalBorderBox;
    } else if (size.is<Keywords::MaxContent>()) {
        auto intrinsicSize = computeIntrinsicContentSize(tree, box, IntrinsicSize::MAX_CONTENT, capmin);
        return intrinsicSize.x + horizontalBorderBox;
    } else if (size.is<FitContent>()) {
        auto minIntrinsicSize = computeIntrinsicContentSize(tree, box, IntrinsicSize::MIN_CONTENT, capmin);
        auto maxIntrinsicSize = computeIntrinsicContentSize(tree, box, IntrinsicSize::MAX_CONTENT, capmin);
        auto stretchIntrinsicSize = computeIntrinsicContentSize(tree, box, IntrinsicSize::STRETCH_TO_FIT, capmin);

        return clamp(stretchIntrinsicSize.x, minIntrinsicSize.x, maxIntrinsicSize.x) + horizontalBorderBox;
    } else if (size.is<Keywords::Auto>()) {
        return NONE;
    } else {
        logWarn("unknown specified size: {}", size);
        return 0_au;
    }
}

Opt<Au> computeSpecifiedBorderBoxHeight(Tree& tree, Box& box, Size size, Vec2Au containingBlock, Au verticalBorderBox) {
    if (auto calc = size.is<CalcValue<PercentOr<Length>>>()) {
        auto specifiedHeight = resolve(tree, box, *calc, containingBlock.y);
        if (box.style->boxSizing == BoxSizing::CONTENT_BOX) {
            specifiedHeight += verticalBorderBox;
        }
        return specifiedHeight;
    }

    if (size.is<Keywords::MinContent>()) {
        // https://drafts.csswg.org/css-sizing-3/#valdef-width-min-content
        // for a box’s block size, unless otherwise specified, this is equivalent to its automatic size.
        return NONE;
    } else if (size.is<Keywords::MaxContent>()) {
        // https://drafts.csswg.org/css-sizing-3/#valdef-width-max-content
        // for a box’s block size, unless otherwise specified, this is equivalent to its automatic size.
        return NONE;
    } else if (size.is<FitContent>()) {
        // Since this depends on min/max content size, this is also unknown.
        return NONE;
    } else if (size.is<Keywords::Auto>()) {
        return NONE;
    } else {
        logWarn("unknown specified size: {}", size);
        return 0_au;
    }
}

Copilot · 2025-11-19T15:21:53Z

src/vaev-engine/layout/table.cpp

+            }
+
+            // If the currently winning border style comes first in the list, it is kept as the winner.
+            // If same specificty, the current best is kept as the winner.


Spelling error: "specificty" should be "specificity".

Suggested change

// If same specificty, the current best is kept as the winner.

// If same specificity, the current best is kept as the winner.

Copilot · 2025-11-19T15:21:53Z

tests/css/box-model/border.xhtml

@@ -0,0 +1,30 @@
+<test name="border-radii"></test>


Missing closing </test> tag. The test begins on line 1 with <test name="border-radii"></test> but this appears to be a self-closing tag followed by content that should be inside the test. The structure should be <test name="border-radii">...</test> with the closing tag at the end.

Suggested change

<test name="border-radii"></test>

<test name="border-radii">

Copilot · 2025-11-19T15:21:54Z

tests/css/display/display-table.xhtml

+    </rendering>
+</test>
+
+<test name="table: borders collapse col and rol groups" size="320">


Typo in test name: "rol groups" should be "row groups".

Suggested change

<test name="table: borders collapse col and rol groups" size="320">

<test name="table: borders collapse col and row groups" size="320">

Copilot · 2025-11-19T15:21:54Z

src/vaev-engine/layout/table.cpp


        rowHeight.resize(grid.size.y);

+        // FIXME


Comment explaining the FIXME has been removed, but the FIXME itself was not addressed. Consider adding back context or addressing the FIXME.

Suggested change

// FIXME

// FIXME: CSS 2.2 and CSS Tables Level 3 do not specify how to initialize

// row heights when percentage heights are used, or when no explicit

// heights are set. For now, we initialize all row heights to 0.

// See: https://www.w3.org/TR/CSS22/tables.html#height-layout

// and https://www.w3.org/TR/css-tables-3/#computing-the-table-height

Copilot · 2025-11-19T15:21:54Z

src/vaev-engine/layout/table.cpp

+                CacheParametersFromInput inputCacheParameters{input};
+                if (lastInput != inputCacheParameters) {
+                    lastInput = inputCacheParameters;
+                    // bad code


Comment "bad code" is unprofessional and not informative. Either remove it or replace with a proper NOTE/FIXME comment explaining what needs to be improved.

Copilot · 2025-11-19T15:21:56Z

src/vaev-engine/layout/table.cpp

+        };
+    }
+
+    Opt<Map<Box*, UsedBorders>> boxBorderMapping;


The TableFormatingContext struct is now exported but boxBorderMapping is accessed from outside in paint.cpp without proper encapsulation. Consider making this field private with a public getter method, or documenting why it needs to be publicly accessible.

Suggested change

Opt<Map<Box*, UsedBorders>> boxBorderMapping;

private:

Opt<Map<Box*, UsedBorders>> _boxBorderMapping;

public:

Opt<Map<Box*, UsedBorders>> const& boxBorderMapping() const {

return _boxBorderMapping;

}

Copilot · 2025-11-19T15:21:56Z

src/vaev-engine/values/borders.cpp

+template <>
+_Border<LineWidth>::_Border() : width(Keywords::MEDIUM) {}


The constructor definition for _Border<LineWidth> is separated from the struct definition. Consider using default member initialization in the struct definition (e.g., T width = Keywords::MEDIUM; for LineWidth specialization) or keeping the constructor definition closer to the struct for better maintainability.

Copilot · 2025-11-19T15:21:56Z

src/vaev-engine/layout/table.cpp

 template <typename T>
 struct PrefixSum {
-
    Vec<Au> pref = {};


Empty line at the beginning of the struct scope should be removed per coding guidelines.

Copilot · 2025-11-19T15:21:57Z

src/vaev-engine/layout/paint.cpp

+        // FIXME: downcasting like this?
+        TableFormatingContext* tableFormattingContext = (TableFormatingContext*)&(*frag.box->formatingContext.unwrap());


Unsafe C-style cast is being used. Consider using static_cast<TableFormatingContext*> or better yet, redesign to avoid downcasting altogether (e.g., using virtual methods or visitor pattern).

Suggested change

// FIXME: downcasting like this?

TableFormatingContext* tableFormattingContext = (TableFormatingContext*)&(*frag.box->formatingContext.unwrap());

// Downcasting: safe as per code path, use static_cast for clarity.

TableFormatingContext* tableFormattingContext = static_cast<TableFormatingContext*>(&(*frag.box->formatingContext.unwrap()));

Copilot · 2025-11-19T15:21:57Z

src/vaev-engine/layout/table.cpp

        colWidthOrNone.resize(grid.size.x);
        for (auto& col : cols) {
-
            auto const& width = col.el.style->sizing->width;


Empty line at the beginning of the scope should be removed per coding guidelines.

pauloamed and others added 9 commits November 19, 2025 15:48

vaev-engine: Make table grid API simpler and better named.

2557650

vaev-engine: Refactor layout interface, shifting sizing and insets co…

c67329e

…mputation to parent.

vaev-engine: Fit intrinsic sizing into the new layout api contract.

d5a1ca4

vaev-engine: Add support to table's border-collapse.

6d26ad1

vaev-engine: Implement box-sizing in layout flow.

586cacf

vaev-engine: Fix removed from flow boxes not laid out in inline FC.

8b320f2

vaev-engine: Add capmin parameter to width computation helpers.

66c6edc

vaev-engine: Compute and store table intrinsic sizes.

21059ac

end of the great rebase

ea7e9dd

Louciole requested review from Copilot and sleepy-monax November 19, 2025 15:17

Copilot started reviewing on behalf of Louciole November 19, 2025 15:18 View session

Copilot finished reviewing on behalf of Louciole November 19, 2025 15:20

Copilot AI reviewed Nov 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

PALM HACKS #130

PALM HACKS #130

Uh oh!

Louciole commented Nov 19, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 19, 2025

Uh oh!

Copilot AI Nov 19, 2025

Uh oh!

Copilot AI Nov 19, 2025

Uh oh!

Copilot AI Nov 19, 2025

Uh oh!

Copilot AI Nov 19, 2025

Uh oh!

Copilot AI Nov 19, 2025

Uh oh!

Copilot AI Nov 19, 2025

Uh oh!

Copilot AI Nov 19, 2025

Uh oh!

Copilot AI Nov 19, 2025

Uh oh!

Copilot AI Nov 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	// If same specificty, the current best is kept as the winner.
	// If same specificity, the current best is kept as the winner.

	<test name="table: borders collapse col and rol groups" size="320">
	<test name="table: borders collapse col and row groups" size="320">

-        // FIXME
+        // FIXME: CSS 2.2 and CSS Tables Level 3 do not specify how to initialize
+        //        row heights when percentage heights are used, or when no explicit
+        //        heights are set. For now, we initialize all row heights to 0.
+        //        See: https://www.w3.org/TR/CSS22/tables.html#height-layout
+        //        and https://www.w3.org/TR/css-tables-3/#computing-the-table-height

-    Opt<Map<Box*, UsedBorders>> boxBorderMapping;
+private:
+    Opt<Map<Box*, UsedBorders>> _boxBorderMapping;
+public:
+    Opt<Map<Box*, UsedBorders>> const& boxBorderMapping() const {
+        return _boxBorderMapping;
+    }

		template <>
		_Border<LineWidth>::_Border() : width(Keywords::MEDIUM) {}

		// FIXME: downcasting like this?
		TableFormatingContext* tableFormattingContext = (TableFormatingContext)&(frag.box->formatingContext.unwrap());

PALM HACKS #130

Are you sure you want to change the base?

PALM HACKS #130

Uh oh!

Conversation

Louciole commented Nov 19, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants