feat(layout): impl grid layout

[TtTRz]

This PR resolves #19

[Copilot]

Pull request overview

This pull request implements CSS Grid Layout functionality for the float-pigment layout engine, adding support for display: grid and display: inline-grid with core grid properties like grid-template-rows, grid-template-columns, grid-auto-flow, justify-items, and justify-self.

Changes:

• Implements CSS Grid Layout Module Level 1 algorithms (placement, track sizing, alignment)
• Adds SizingMode enum to support min-content/max-content sizing for grid items
• Extends layout traits with grid-specific properties and methods
• Adds comprehensive test coverage for grid layout features

Reviewed changes

Copilot reviewed 65 out of 66 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
float-pigment-layout/src/lib.rs	Adds grid-related trait methods, exports SizingMode, comments out #![no_std]
float-pigment-layout/src/unit.rs	Integrates SizingMode into layout computation, adds min_content_size to results
float-pigment-layout/src/algo/grid/*	New grid layout algorithm implementation (placement, track sizing, alignment, matrix)
float-pigment-layout/src/types.rs	Adds grid template types and Clone trait bounds
float-pigment-layout/Cargo.toml	Adds grid crate dependency
float-pigment-css/src/typing.rs	Adds grid-related CSS types and properties
float-pigment-forest/*	Integrates grid layout into forest implementation and adds extensive tests

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 81 out of 82 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (4)

float-pigment-css/src/typing_stringify.rs:2406

• fmt::Display for GridTemplate stringifies TrackListItem::LineNames as a plain space-joined list, but CSS syntax requires square brackets around line name lists (e.g. [foo bar]). Without brackets this can’t round-trip to valid CSS and is ambiguous with track sizes. Consider wrapping the joined names with [ and ] when formatting.

                let mut ret = vec![];
                list.iter().for_each(|x| match x {
                    TrackListItem::LineNames(line_names) => ret.push(
                        line_names
                            .iter()
                            .map(|x| x.to_string())
                            .collect::<Vec<_>>()
                            .join(" "),
                    ),

float-pigment-layout/src/lib.rs:95

• LayoutTreeNode::LengthCustom now requires Debug + Clone (was PartialEq only). Since LayoutTreeNode is a public API, this is a breaking change for embedders that use non-Clone/non-Debug custom types. If possible, consider keeping the bound minimal on the trait and only requiring Clone/Debug on the specific APIs/types that need it (e.g. grid template data structures), or document this as an intentional semver-breaking change.
  float-pigment-layout/src/lib.rs:217
• LayoutTreeVisitor is a public trait; adding a new required method (children_iter) is a breaking API change for downstream implementers. If maintaining backward compatibility is important, consider providing a default implementation (even if it allocates) based on children_len/child_at or for_each_child, so existing implementations keep compiling while still allowing optimized overrides.
  float-pigment-layout/Cargo.toml:27
• hashbrown is added as a direct dependency of float-pigment-layout, but there are no hashbrown usages in this crate (searching under float-pigment-layout/src yields none). Consider removing it to avoid unused dependency bloat (and the extra Cargo.lock churn) unless it’s intended for upcoming follow-up changes.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

In Flow::compute, size is already run through min_max_size_limit, and the baselines are derived from that value. But for request.sizing_mode != Normal, ret.size is recomputed without the min/max limiting, so ret.size can diverge from the size used for baseline calculation and caching. Consider always returning the already-computed size (or reapply the same limiting to the recomputed value) so ret.size and the baseline/limits stay consistent.

[Copilot]

Pull request overview

Copilot reviewed 81 out of 82 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (5)

float-pigment-layout/src/algo/grid/alignment.rs:87

• Similarly, justify-self: normal should behave like stretch for grid items. Returning JustifySelf::Normal here means the item won't be stretched in the inline axis even when it should be.
  float-pigment-css/src/typing_stringify.rs:2406
• GridTemplate stringification drops the [] around line names, producing invalid CSS (e.g. it would stringify [foo bar] as foo bar). This breaks round-tripping and doesn’t match the syntax accepted by the parser. Wrap the joined line names in brackets when emitting TrackListItem::LineNames.

                let mut ret = vec![];
                list.iter().for_each(|x| match x {
                    TrackListItem::LineNames(line_names) => ret.push(
                        line_names
                            .iter()
                            .map(|x| x.to_string())
                            .collect::<Vec<_>>()
                            .join(" "),
                    ),

float-pigment-layout/src/lib.rs:217

• LayoutTreeVisitor now requires children_iter, which is a breaking change for all external implementers. If possible, consider providing a default implementation (e.g. via a small custom iterator over children_len/child_at) so existing integrations don’t break when upgrading for grid support.
  float-pigment-layout/src/algo/grid/track_size.rs:78
• total_specified_track_size is computed but never used. If it’s not needed yet, remove it to avoid dead code; if it will be needed for later phases, consider adding a comment/TODO and prefixing with _ to silence unused-variable warnings.
  float-pigment-layout/src/algo/grid/alignment.rs:54
• align-self: normal on a grid item should resolve to stretch behavior (per CSS Box Alignment; same rationale as the AlignItems::Normal => Stretch mapping above). Currently resolve_grid_align_self returns AlignSelf::Normal, which prevents the stretching path in grid layout.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

For SizingMode != Normal, ret.size is recomputed from node_size without applying min_max_size_limit, while baselines/defaults were computed using the clamped size. This can return a size that violates min/max constraints and can also make baselines inconsistent with the returned size. Consider always returning the same (already clamped) size and only varying the input constraints used to compute compute_res for intrinsic sizing modes.

[Copilot]

Pull request overview

Copilot reviewed 81 out of 82 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (7)

float-pigment-css/src/parser/property_value/mod.rs:819

• custom_ident_repr accepts Token::QuotedString, which allows quoted strings where CSS <custom-ident> is expected (e.g., grid line names). This makes the parser accept syntax that should be invalid per spec; consider only accepting Token::Ident (and possibly rejecting CSS-wide keywords if applicable).

pub(crate) fn custom_ident_repr<'a, 't: 'a, 'i: 't>(
    parser: &'a mut Parser<'i, 't>,
    _properties: &mut [PropertyMeta],
    _st: &mut ParseState,
) -> Result<String, ParseError<'i, CustomError>> {
    let next = parser.next()?.clone();
    match &next {
        Token::Ident(name) => Ok(name.to_string()),
        Token::QuotedString(name) => Ok(name.to_string()),
        _ => Err(parser.new_unexpected_token_error::<CustomError>(next.clone())),
    }

float-pigment-css/src/parser/property_value/grid.rs:47

• fr_repr returns the raw numeric value for <number>fr without validating it is non-negative. CSS Grid requires fr values to be >= 0; consider rejecting negative values (or clamping to 0) so invalid track sizes don’t propagate into layout calculations.

#[inline(never)]
pub(crate) fn fr_repr<'a, 't: 'a, 'i: 't>(
    parser: &'a mut Parser<'i, 't>,
    _properties: &mut [PropertyMeta],
    _st: &mut ParseState,
) -> Result<f32, ParseError<'i, CustomError>> {
    let next = parser.next()?;
    match next {
        Token::Dimension { value, unit, .. } => match unit as &str {
            "fr" => return Ok(*value),
            _ => {}
        },
        _ => {}
    }
    let next = next.clone();
    Err(parser.new_unexpected_token_error(next))
}

float-pigment-css/src/typing_stringify.rs:2410

• GridTemplate stringification emits line names without the required surrounding brackets (e.g. it outputs foo bar rather than [foo bar]). This produces invalid CSS and breaks round-tripping; wrap TrackListItem::LineNames output in [...] when formatting.

impl fmt::Display for GridTemplate {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        match self {
            GridTemplate::None => write!(f, "none"),
            GridTemplate::TrackList(list) => {
                let mut ret = vec![];
                list.iter().for_each(|x| match x {
                    TrackListItem::LineNames(line_names) => ret.push(
                        line_names
                            .iter()
                            .map(|x| x.to_string())
                            .collect::<Vec<_>>()
                            .join(" "),
                    ),
                    TrackListItem::TrackSize(track_size) => ret.push(track_size.to_string()),
                });
                write!(f, "{}", ret.join(" "))
            }

float-pigment-layout/src/algo/grid/track_size.rs:80

• total_specified_track_size is computed but never used. If it’s not needed, remove it to avoid dead code; if it’s intended for computing free space / available grid space, wire it into that logic so the initial sizing pass is complete.
  float-pigment-layout/Cargo.toml:27
• hashbrown is added as a direct dependency of float-pigment-layout, but there are no hashbrown uses in this crate. If it’s not required for the grid implementation, please remove it to keep the dependency surface minimal (and avoid failing unused_crate_dependencies lints in some setups).
  float-pigment-layout/src/algo/flow.rs:300
• For sizing_mode != Normal, the returned ComputeResult.size bypasses min_max_size_limit (it rebuilds the size from node_size/content totals instead of using the already-clamped size). If intrinsic sizing is still supposed to respect min/max constraints, return the clamped size for these modes as well; if skipping min/max is intentional, add an explicit comment explaining why (and consider renaming the mode to distinguish “intrinsic size” vs “intrinsic contribution”).
  float-pigment-layout/src/lib.rs:218
• LayoutTreeVisitor adds a new required method children_iter without a default implementation, which is an API-breaking change for downstream implementers. Consider providing a default implementation based on children_len/child_at (or for_each_child) to preserve backward compatibility, or clearly version-gate/document the breaking change.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.