Skip to content

LibWeb: Follow explicit cell widths when calculating fixed table layouts#9685

Closed
Darshanx256 wants to merge 1 commit into
LadybirdBrowser:masterfrom
Darshanx256:fix-fixed-table-layout
Closed

LibWeb: Follow explicit cell widths when calculating fixed table layouts#9685
Darshanx256 wants to merge 1 commit into
LadybirdBrowser:masterfrom
Darshanx256:fix-fixed-table-layout

Conversation

@Darshanx256

@Darshanx256 Darshanx256 commented May 27, 2026

Copy link
Copy Markdown
Contributor

This PR fixes a bug in the fixed table layout, where cells with
explicitly specified length/percentage widths were ignoring their
explicit width and falling back to the content size.

Specifications referred:
https://www.w3.org/TR/css-tables-3/#computing-column-measures
https://www.w3.org/TR/CSS2/tables.html#fixed-table-layout
https://www.w3.org/TR/CSS22/tables.html#auto-table-layout

The fix resolves horizontal squashing layout issues seen on live
websites, such as the JS Crossword grid at:
https://lyra.horse/fun/jscrossword/

Added a regression test:
Tests/LibWeb/Layout/input/table/fixed-layout-cell-specified-width.html

@Darshanx256
Darshanx256 force-pushed the fix-fixed-table-layout branch from 0408878 to 8a657a9 Compare May 27, 2026 15:49
Comment thread Libraries/LibWeb/Layout/TableFormattingContext.cpp
Comment thread Libraries/LibWeb/Layout/TableFormattingContext.cpp Outdated
@Psychpsyo

Copy link
Copy Markdown
Contributor

This also seems to fail some existing tests.

@Darshanx256

Copy link
Copy Markdown
Contributor Author

looking at that

@Darshanx256
Darshanx256 marked this pull request as draft May 27, 2026 17:04
@Darshanx256
Darshanx256 force-pushed the fix-fixed-table-layout branch from d34546f to 61e5272 Compare May 27, 2026 18:27
@Darshanx256

Copy link
Copy Markdown
Contributor Author

The test failures in fixed-layout-percentage-width-all-columns.html and fixed-layout-percentage-width.html were most probably false alarms.

I discovered this by regenerating the test dumps and verifying the column dimensions manually against the CSS Tables Level 3 Specification. I noticed that before this PR, there was a bug where percentage cell widths (like width: 50%) were evaluating to 0px under the hood. This forced the engine to fallback to the intrinsic text width of the cell instead of mathematically calculating the correct percentage. The old test expectation files were generated while this bug was active, meaning they captured the wrong layout.

Looking at the baseline expectation in the master, It expected the 50% cell to have a width of 295px. So I've rebaselined the expectation files to match the new, correct output.

How to reproduce the old bug:

  • Checkout the master branch.
  • Run this command to dump the layout tree for the test: Build/release/bin/test-web --dump-layout-tree Tests/LibWeb/Layout/input/table/fixed-layout-percentage-width.html
  • Look at the output for the 50% cell. Even though the table is 600px, the cell incorrectly gets assigned a width of 295px instead of 300px.
  • Switch back to this PR branch and run the exact same command. You'll see the cell now correctly gets assigned 300px, proving the layout is finally doing the math right.

@Darshanx256

Copy link
Copy Markdown
Contributor Author

First off, the code checks if the table is actually using the fixed layout mode by calling use_fixed_mode_layout().
If it's using a fixed layout, it immediately forces both the min_content_width and max_content_width of the cell to match whatever width is explicitly given, like width: 50px. If width is isnt specified at all, it just treats the content width as 0.
After that, it checks if the cell is using box-sizing: border-box. If it is, the code subtracts the cell's padding and borders from that specified width so the inner content area is sized correctly, making sure the size never drops below zero pixels.
The new amend handles percent based widths properly. Also the test timeout content-blocker-srcdoc-frame-cosmetic-source looks unrelated

@Darshanx256
Darshanx256 marked this pull request as ready for review May 27, 2026 18:52
@Darshanx256
Darshanx256 requested a review from Psychpsyo May 27, 2026 18:52
Comment thread Libraries/LibWeb/Layout/TableFormattingContext.cpp
Comment thread Libraries/LibWeb/Layout/TableFormattingContext.cpp Outdated
@Psychpsyo

Psychpsyo commented May 27, 2026

Copy link
Copy Markdown
Contributor

Just as an observation: The old test expectations match WebKit behavior. The new expectations match Chromium and Gecko.

@Darshanx256
Darshanx256 marked this pull request as draft May 28, 2026 05:22
@Darshanx256

Darshanx256 commented May 28, 2026

Copy link
Copy Markdown
Contributor Author

Just an update on top of this, I cleaned up a few more things to get us strictly 1:1 with the spec.

First off, I swapped out the manual padding and border subtraction with cell_intrinsic_width_offsets because it cleans up the math.

After that, I made sure we completely ignore any cell that isn't in the very first row when calculating column widths in fixed mode. It just skips them in the layout loops so they don't mess up the constraints or percentage distributions.

I also cleaned up those for loops by moving the use_fixed_mode_layout() checks outside the loops so it doesn't spam property lookups on every single cell, and flattened out some nested if statements.

A bug existing in the upstream code where fixed layout tables with width: auto were incorrectly expanding to satisfy percent based cell widths. Like, if a cell down in the 10th row had width: 90%, it would literally force the whole fixed table to stretch out. But the spec explicitly says that percentage expansion algorithm is only for auto-table-layouts. For fixed layouts with auto width, the browser isn't supposed to artificially expand the table to satisfy cell percentages. So I wrapped that expansion loop in an if (!use_fixed_mode_layout()) check to completely kill it for fixed layouts.

https://www.w3.org/TR/CSS22/tables.html#auto-table-layout (This defines the expansion algorithm, proving it ONLY applies to automatic layouts).
https://www.w3.org/TR/CSS22/tables.html#fixed-table-layout (This explicitly states that for fixed tables with auto width, the browser "is not required to do anything in particular")

@Darshanx256
Darshanx256 force-pushed the fix-fixed-table-layout branch from 61e5272 to 919df4d Compare May 28, 2026 08:41
@Darshanx256
Darshanx256 requested a review from Psychpsyo May 28, 2026 08:53
Comment thread Libraries/LibWeb/Layout/TableFormattingContext.cpp Outdated
Comment thread Libraries/LibWeb/Layout/TableFormattingContext.cpp Outdated
@Darshanx256
Darshanx256 force-pushed the fix-fixed-table-layout branch 3 times, most recently from 4e17b78 to 8a9d451 Compare May 28, 2026 15:51
@Darshanx256
Darshanx256 requested a review from Psychpsyo May 28, 2026 16:19
In fixed table layouts, cell percentage widths were incorrectly being
resolved against the containing block of the table wrapper (the page)
instead of the table's computed width. This caused cells with
percentage widths to evaluate to incorrect absolute dimensions and throw
off the table column distribution logic.

This commit properly queries the table's computed width first and uses
that as the reference width when resolving percentage widths in
TableFormattingContext::compute_cell_measures(), ensuring that the math
adheres to the CSS Tables Level 3 Specification. Test baselines have
been updated to match the correct output.

Additionally, this commit follows explicit cell widths when calculating
fixed table layouts.
@Darshanx256
Darshanx256 force-pushed the fix-fixed-table-layout branch from 8a9d451 to 4719a76 Compare May 28, 2026 16:30
@Darshanx256
Darshanx256 marked this pull request as ready for review May 29, 2026 10:22
@gmta

gmta commented Jun 5, 2026

Copy link
Copy Markdown
Collaborator

Thank you for taking the time to work on this pull request.

As announced in "Changing How We Develop Ladybird", we are closing all currently open public pull requests as part of ending the public code contribution path for Ladybird.

This is not a judgment on the quality of your work; we are very grateful for the effort you put into this pull request. But as of right now, code changes to the Ladybird codebase will only be introduced by project maintainers.

We still welcome clear bug reports, reduced test cases, website testing, standards discussion, design discussion, security reports, and technical feedback.

@gmta gmta closed this Jun 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants