Feat/css grid integration tests #775
Conversation
…racks in indefinite container`
…solves circular dependencies` case
…tent in content-sized
…m aspect ratio issue
- Grow implicit columns for large span N items in row auto-flow - Avoid inflating implicit/auto rows when fixed tracks already satisfy spanning height - Add css-grid/grid-items/spanning.ts coverage + snapshots
Accept width/min/max keywords (min-content/max-content/fit-content) and use intrinsic contributions so fr tracks don’t overlap; enable integration test for intrinsic sizing keywords.`
…ne case and snapshots
…t for implicit <area>-start/<area>-end line name
- Parse place-items/place-self shorthands (space/slash, baseline keywords) - Adjust row/column auto-flow placement ordering to keep remaining items in order-modified document order - Add regression assertion for row dense implicit-row span placement
RenderGridLayout now hit tests children using the painting order. Add integration spec to verify document.elementFromPoint works for grid items (including relative offsets).
| The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds broad CSS Grid support and tests while introducing intrinsic sizing keywords (min-content, max-content, fit-content), baseline/lastBaseline alignment, shorthand parsing improvements, and several rendering/layout adjustments across Dart code; also updates .gitignore and documentation. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
webf/lib/src/rendering/flex.dart (1)
2395-2403:_hasBaselineAlignedChildrenignoreslastBaseline, causing inconsistent fast-path behaviorMost baseline-related paths now treat
AlignSelf.lastBaseline/AlignItems.lastBaselinelikebaseline, but_hasBaselineAlignedChildren()still only checks the plainbaselinevariants. That means a flex container using onlylastBaselinecan be classified as “no baseline alignment”, allowing_tryNoFlexNoStretchNoBaselineFastPath()to run even though baseline alignment is in effect, which can corrupt run cross-size and baseline calculations.Recommend updating this helper to recognize both baseline variants:
Proposed fix
bool _hasBaselineAlignedChildren(List<_RunMetrics> runMetrics) { if (!_isHorizontalFlexDirection) return false; - if (renderStyle.alignItems == AlignItems.baseline) return true; + if (renderStyle.alignItems == AlignItems.baseline || + renderStyle.alignItems == AlignItems.lastBaseline) { + return true; + } for (final _RunMetrics metrics in runMetrics) { for (final _RunChild runChild in metrics.runChildren) { - if (runChild.alignSelf == AlignSelf.baseline) return true; + if (runChild.alignSelf == AlignSelf.baseline || + runChild.alignSelf == AlignSelf.lastBaseline) { + return true; + } } } return false; }webf/lib/src/css/computed_style_declaration.dart (1)
300-303:MaxWidthcomputed value incorrectly returnsmaxHeightIn
_valueForPropertyInStyle, theMaxWidthbranch currently uses the height field:case CSSPropertyID.MaxWidth: return style.maxHeight.cssText();This causes
getComputedStyle(...).maxWidth(and related API usage) to report the element’smax-heightinstead ofmax-width, which can directly break layout logic and tests relying on the computed style.Recommended fix:
Proposed correction
- case CSSPropertyID.MaxWidth: - return style.maxHeight.cssText(); + case CSSPropertyID.MaxWidth: + return style.maxWidth.cssText();
🧹 Nitpick comments (11)
.gitignore (1)
72-72: Add trailing slash to.preview-bundlesfor consistency.The pattern
**/.preview-bundlesshould be**/.preview-bundles/to match directory-ignoring conventions used elsewhere in the file (lines 9, 38–39, 74–75).🔎 Proposed fix
-**/.preview-bundles +**/.preview-bundles/integration_tests/specs/css/css-grid/placement/overlapping-items.ts (1)
156-211: Consider adding comments for skipped tests.The skipped tests (
xit) would benefit from brief comments explaining why they're disabled (e.g., feature not yet implemented, known issue, or deferred coverage). This helps future maintainers understand the test status without investigating commit history.Example comment pattern
+ // TODO: Enable once negative z-index stacking context is fully implemented xit('handles negative z-index', async () => {+ // TODO: Deferred - partial overlap rendering needs further investigation xit('overlaps with partial grid areas', async () => {Also applies to: 322-385
integration_tests/specs/css/css-grid/placement/line-based.ts (2)
203-253: Named line placement tests are solid.The tests correctly validate both unique named lines and repeated named line references with span notation. The assertions confirm proper positioning based on named grid lines.
Consider adding an edge case test for non-existent named lines (should fall back to auto placement per CSS Grid spec):
🔎 Suggested edge case test
it('handles non-existent named lines', async () => { const grid = document.createElement('div'); grid.style.display = 'grid'; grid.style.gridTemplateColumns = '[start] 80px [end]'; const item = document.createElement('div'); item.style.gridColumnStart = 'nonexistent'; // Should fallback to auto grid.appendChild(item); document.body.appendChild(grid); await waitForFrame(); await snapshot(); // Verify auto-placement occurred expect(item.getBoundingClientRect().width).toBe(80); grid.remove(); });Also applies to: 255-301
303-361: Auto placement test validates spanning behavior.The test correctly validates auto placement combined with explicit span values. Width assertions confirm the spanning behavior.
The comment on line 357 mentions wrapping to the next row, but no position assertions verify this. Consider adding:
🔎 Optional position assertions
// Item 3: 3 columns wide (wraps to next row if needed) expect(items[2].getBoundingClientRect().width).toBe(210); // 70px * 3 +expect(items[2].getBoundingClientRect().top).toBe(grid.getBoundingClientRect().top + 60); // Row 2integration_tests/specs/css/css-grid/grid-lanes/track-sizing-fr.ts (1)
33-33: Remove duplicate semicolons.Multiple lines contain duplicate semicolons (
'400px';;), which should be cleaned up for consistency.🔎 Proposed fix
- grid.style.width = '400px';; + grid.style.width = '400px';Apply this change to lines 33, 69, 104, 140, 175, 212, and 327.
Also applies to: 69-69, 104-104, 140-140, 175-175, 212-212, 327-327
integration_tests/specs/css/css-grid/grid-lanes/track-sizing-intrinsic.ts (1)
41-41: Remove duplicate semicolons.Multiple lines contain duplicate semicolons (
'400px';;).🔎 Proposed fix
- grid.style.width = '400px';; + grid.style.width = '400px';Apply this change to lines 41, 149, and 248.
Also applies to: 149-149, 248-248
integration_tests/specs/css/css-grid/grid-lanes/track-sizing-content.ts (1)
85-85: Remove duplicate semicolons.Lines contain duplicate semicolons (
'400px';;).🔎 Proposed fix
- grid.style.width = '400px';; + grid.style.width = '400px';Apply this change to lines 85 and 249.
Also applies to: 249-249
integration_tests/specs/css/css-grid/grid-items/min-max-content.ts (1)
11-25: Consider batching DOM operations to minimize reflows.The test sets style properties individually on items, which can trigger multiple reflows. Consider batching these operations by building the style string once or using
cssText.Based on learnings, batching DOM operations is recommended in integration tests.
integration_tests/specs/css/css-grid/grid-definition/auto-repeat.ts (1)
26-27: Correct the explanatory math for auto-fill track countIn this comment the gap math is slightly off: with a 320px container,
repeat(auto-fill, 100px)and a 10px gap, three tracks actually fit exactly (3×100px + 2×10px = 320px). The assertions (checking item widths) are fine, but updating the comment will avoid confusion for future readers.integration_tests/specs/css/css-grid/grid-items/stretch.ts (1)
292-321: Use integer precision fortoBeCloseToin aspect-ratio test
toBeCloseTo(16/9, 0.5)passes a non-integer precision where the matcher expects “number of decimal digits”. Using an integer (e.g. 1 or 2) is clearer and avoids any runner-specific quirks.Proposed change
- expect(ratio).toBeCloseTo(16/9, 0.5); + expect(ratio).toBeCloseTo(16 / 9, 2);webf/lib/src/css/style_property.dart (1)
1490-1570: Shared parser forplace-items/place-selfshorthands is a solid consolidationUsing
_parsePlaceShorthandAxesto drive bothsetShorthandPlaceItemsandsetShorthandPlaceSelfremoves duplication and correctly supports bothalign / justifysyntax and space-separated forms, including compound tokens likefirst baselineandsafe center. If you later decide to support the same extended axis syntax onplace-content, you could consider reusing this helper there as well for consistency.
| **/.preview-bundles | ||
| | ||
| **/.dartServer/ | ||
| **/.dart-tool/ |
There was a problem hiding this comment.
Fix the .dart_tool/ directory pattern: change .dart-tool/ to .dart_tool/
The directory created by Dart tooling is .dart_tool/ (with underscore), not .dart-tool/ (with hyphen). Without the correct pattern, the actual Dart-generated directory will not be ignored.
🤖 Prompt for AI Agents
In .gitignore around line 75, the pattern incorrectly references `.dart-tool/` (hyphen) but the Dart tooling directory is `.dart_tool/` (underscore); update the line to use `.dart_tool/` so the actual Dart-generated directory is ignored by Git. | onImageLoad(child, async () => { | ||
| await snapshot(0.1); | ||
| const rect = child.getBoundingClientRect(); | ||
| expect(rect.width).toBeGreaterThan(0); | ||
| expect(rect.height).toBeGreaterThan(0); | ||
| done(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Search for onImageLoad definition in the test directory rg -nP 'onImageLoad|function.*onImageLoad' integration_tests/ --type ts # Also check if it's exported from a common test utilities file fd -e ts integration_tests/specs/ | head -20Repository: openwebf/webf
Length of output: 6555
🏁 Script executed:
#!/bin/bash # Look at the test file to understand the context better cat -n integration_tests/specs/css/css-sizing/block-fit.ts | head -70Repository: openwebf/webf
Length of output: 1927
🏁 Script executed:
#!/bin/bash # Search for helper functions or utilities in integration_tests fd -e ts integration_tests/ | xargs grep -l "onImageLoad" 2>/dev/null | head -10Repository: openwebf/webf
Length of output: 434
🏁 Script executed:
#!/bin/bash # Search for onImageLoad definition anywhere in codebase rg -nP 'export\s+.*onImageLoad|function\s+onImageLoad|const\s+onImageLoad' --type ts # Also search for where it might be used rg -nP '\bonImageLoad\(' --type ts | head -20Repository: openwebf/webf
Length of output: 2391
Add test cleanup to remove created parent element.
Both tests in this file are missing cleanup. The parent element should be removed from BODY before calling done() to ensure test isolation per the coding guideline: "Remove created elements after tests (test cleanup)".
Add BODY.removeChild(parent); before the done(); call on line 64.
🤖 Prompt for AI Agents
In integration_tests/specs/css/css-sizing/block-fit.ts around lines 58–64, the test creates a parent element but does not remove it, breaking test isolation; before calling done() remove the parent from the document (e.g. call document.body.removeChild(parent) or BODY.removeChild(parent) depending on the test helper) so the created element is cleaned up, then call done(). 
Add CSS Grid test baseon Web Platform Tests (WPT)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.