Implement CSS table layout (Display::Table) - #1094
Conversation
153eb18 to
7e28f10
Compare
There was a problem hiding this comment.
This is a review of everything in this PR that is NOT the actual algorithm in compute/table.rs.
In general, this looks about the right shape.
However, I'm not overjoyed about about this adding to the cache key. Especially as I think this means that every table cell will need to be relayouted if it's alignment in the parent changes. I would be interested to explore designs that avoid this. Although it may end up being that this is the best design.
AI is suggesting something like an additional field on LayoutOutput instead. Which doesn't look super-clean either, but perhaps would have better performance characteristics?
table_layout is currently in the default feature set. Keep it there, or opt-in?
Yes, we use that convention in Taffy because it makes development easier (e.g. code is enabled for Rust Analyzer). Consumers can always disable features they don't want.
Here are some screenshots tested with blitz:
Does this mean you have a Blitz branch that uses this sitting around somewhere? Have you run the table WPT tests against it?
Is this enough to consider it done, with captions, /, and the border-collapse contract tracked as follow-up issues?
I don't yet have a good read on how complete this is. I might be wrong, but I believe TableWrapper / captions may just be a block layout. <col>/<colgroup> feels like a more core part of the algorithm to me. When I did CSS Grid I had a great big todo list: #204. Something along those lines for this might be a good idea to track progress.
Putting content_offset_y in CacheKey costs 80 bytes per node. CacheKey grows from 16 to 24 bytes, and Cache from 368 to 448, since each node holds one final-layout entry and nine measure entries. The field is only ever non-zero under RunMode::PerformLayout. For callers inside the crate, the comparison it adds to the ComputeSize lookup is therefore always true. Should it move out of CacheKey and sit on the final-layout entry alone?
If we stick with the cache key approach, then I think using separate CacheKey types for the final layout cache would make sense here.
A ComputeSize cache hit returns LayoutOutput::from_outer_size, which drops baselines. Table row sizing reads first_baselines.y, so a cell takes a different baseline path when its measurement comes from the cache. This predates the PR, but table layout is the first algorithm to depend on it.
All of the other algorithms have a constraint where they do a PerformLayout if they need baselines. In general, most of the algorithms don't return useful/correct baselines unless you do PerformLayout. Baselines are a bit awkward in that they require more computation than size computation but less than a full layout. It's less critical for other Taffy layout modes because use of baseline alignment is relatively rare there (unlike table where it's the default). I'm not sure what the best solution to this is.
Anonymous box fixup (css-tables-3 §3) and border-collapse resolution are treated as the tree builder's responsibility and documented in the module docs. Should either live in Taffy instead?
Anonymous box fixup being the tree builder's responsibility I definitely agree with (we may eventually want to add this phase to TaffyTree, but for now leaving it unimplemented in Taffy is correct). Border collapse resolution I'm less sure about. It feels like that might be better in the "layout" phase than in "construction"?
| /// `display: table-row-group` (thead, tbody, tfoot) | ||
| RowGroup, | ||
| /// Any other display value | ||
| Other, |
There was a problem hiding this comment.
Should perhaps be None? What do you think?
| /// The item's role within the table, derived from its `display` value | ||
| #[inline(always)] | ||
| fn table_role(&self) -> TableRole { | ||
| TableRole::Cell |
There was a problem hiding this comment.
Should Other/None perhaps be the default?
| #[cfg(feature = "table_layout")] | ||
| (Display::TableCell | Display::TableRow | Display::TableRowGroup, true) => { |
There was a problem hiding this comment.
Perhaps Row and RowGroup should panic? Given that it's a "should never be encountered case". (open to discussion on this)
| /// Table layout reaches past its own children into the rows and cells below them, so the tree | ||
| /// must also be traversable to any depth. A cell is a block container, and reads its | ||
| /// `align_content` through [`LayoutBlockContainer`]. | ||
| pub trait LayoutTableContainer: LayoutBlockContainer + TraverseTree { |
There was a problem hiding this comment.
Why does this extend LayoutBlockContainer rather than LayoutPartialTree. This isn't necessarily wrong, but my instinct is that we should just add more methods (duplicate if necessary) unless it's a lot of methods we're inheriting here.
Works towards DioxusLabs#467. Adds automatic and fixed table layout (CSS 2.1 §17) behind a `table_layout` feature flag, with 308 Chrome-generated snapshot tests from 77 new fixtures.
Port the css-tables-3 §3.9 width distribution algorithm and the row height priority order Blink applies. Both replace the linear interpolation and even splits that stood in for them. Merge column constraints by the spec's rule, so a wrappable cell no longer widens a column with a specified width. Account for percentage columns in intrinsic table widths. Keep a rowspan inside its row group. Lend its ascent to the row it starts in, and spread its excess height over the rows it crosses in proportion to their heights. Apply a row group's specified height, and resolve percentage row heights against the table. Add 18 fixtures with expectations generated from Chrome. Drop the parallel per-cell vectors, the quadratic order rescans, and the duplicate `align_content` getter.
The default style is not a table cell, and the `Style` impl already maps every non-table display to `Other`. Returning `Cell` made an unrecognised child silently act as one.
A row, row group, or cell with `display: none` was placed into the grid, where it took up a slot and grew the table. Chrome drops it entirely, so leave it out of the grid and clear its layout the way the block and grid algorithms do.
Table layout read one method off `LayoutBlockContainer`, a cell's `align_content`. Move that method onto `TableItemStyle` so an embedder does not have to implement the whole block container trait to lay out tables.
Table cells now contribute to the table's scrollable overflow rect the way grid and block items do, measured from the padding-box origin and mirrored for RTL.
2 participants
Objective
Works towards #467
Adds table layout (CSS 2.1 §17) behind a default-on
table_layoutfeature.New display variants
Table/TableRowGroup/TableRow/TableCell, plustable_layout,border_spacing,colspan, androwspanstyle properties. Covers automatic and fixed layout, spans, border-spacing, RTL, and row baseline alignment.Cells are block containers and dispatch to
compute_block_layout. A cell reads itsalign_contentthroughBlockContainerStyle.LayoutTableContainerextendsLayoutBlockContainer + TraverseTree, because table layout reads past its own children into the rows and cells below them. Cell baseline shifts pass through a newLayoutInput::content_offset_yfield, which block layout applies at its content cursor and which participates in the cache key.css-tables-3 leaves width distribution and row height distribution underspecified, so both are ported from Blink rather than written from the spec text alone. Column widths use the four-guess distribution algorithm of css-tables-3 §3.9. Row heights follow Blink's priority order: percentage rows first, then rows a rowspan starts in, then unconstrained rows, then empty rows, then every non-empty row in proportion to its height.
Anonymous box fixup (css-tables-3 §3) and border-collapse resolution are out of scope. They belong to the tree builder. The module docs explain both.
Context
Generated by Claude and reviewed by me.
95 fixtures produce 380 tests. Chrome generates the expectations. The
table_layout-only build passes locally.Here are some screenshots tested with blitz:
Feedback wanted
LayoutInput::content_offset_yfield, consumed by block layout and included in the cache key. This mirrors Blink'sConstraintSpaceapproach, but it does widen a shared public struct. Is that acceptable, or is there a preferred mechanism?content_offset_yinCacheKeycosts 80 bytes per node.CacheKeygrows from 16 to 24 bytes, andCachefrom 368 to 448, since each node holds one final-layout entry and nine measure entries. The field is only ever non-zero underRunMode::PerformLayout. For callers inside the crate, the comparison it adds to theComputeSizelookup is therefore always true. Should it move out ofCacheKeyand sit on the final-layout entry alone?ComputeSizecache hit returnsLayoutOutput::from_outer_size, which drops baselines. Table row sizing readsfirst_baselines.y, so a cell takes a different baseline path when its measurement comes from the cache. This predates the PR, but table layout is the first algorithm to depend on it.table_layoutis currently in the default feature set. Keep it there, or opt-in?<col>/<colgroup>, and the border-collapse contract tracked as follow-up issues? Or should it stay open as the tracking issue?