Layout underinvalidation fixes - #11105
Conversation
Changing colspan, rowspan, or a column span synchronized the new span into the layout node's data and stopped there: the attributes are not presentational hints, so no style recompute follows, and nothing marked the layout tree dirty. A pass that had nothing else to do would skip relayout entirely and the table kept its old track assignment; with cached formatting-context runs the stale spans would also outlive the next genuine pass. The span sync reports whether any effective value actually changed and the attribute-change site invalidates layout only on a report, so a rewritten attribute string with the same effective spans stays a no-op. The raw column span is stored only for table column elements, whose column boxes are its only consumer, keeping span attributes on arbitrary elements out of the arena map.
The default preferred size of a text control follows the input size and textarea rows/cols attributes, but those attributes are not presentational hints and had no invalidation at all: changing them left layout untouched until an unrelated pass happened to run, which then rebuilt the replaced-content facts through the enrolled per-pass sync and picked the new size up incidentally. Mark the control's layout node for update from the attribute-change sites, so the change takes effect immediately. Select's size attribute is outside the facts channel and keeps its pre-existing gap.
Rendered text under a casing text-transform is keyed on the language for locale-sensitive casing, but lang reaches no computed style group, so the language invalidator's style-only walk never re-rendered the text or marked layout dirty: the old casing stayed on screen until an unrelated relayout happened to rebuild the text through the lazy text-for-rendering key comparison. The language invalidator now marks text nodes under casing transforms for re-rendering and relayout, so the change takes effect immediately, and rebuilds the layout tree for first-letter slices, whose boundaries depend on the rendered text.
📝 WalkthroughWalkthroughThe PR adds targeted layout invalidation for language changes under casing transforms, input Sequence Diagram(s)sequenceDiagram
participant DOMElement
participant LanguageInvalidator
participant LayoutTextNode
participant LayoutTree
DOMElement->>LanguageInvalidator: change lang attribute
LanguageInvalidator->>LayoutTextNode: inspect casing transform
LayoutTextNode->>LayoutTree: request text rendering or layout-tree invalidation
LayoutTree-->>DOMElement: produce updated layout and paint output
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
Tests/LibWeb/Text/input/table-colspan-change-relayout.html (1)
18-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for raw
<col span>changes.This test covers only
HTMLTableCellElement::col_span(). Add a<col>case that changesspanfrom1001to1002; both effective spans are 1000, but the raw spans differ. Assert the updated table-track geometry and layout delta.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/LibWeb/Text/input/table-colspan-change-relayout.html` around lines 18 - 30, Extend the test beyond the existing HTMLTableCellElement colspan case with a raw <col span> scenario changing from 1001 to 1002. Verify the effective table-track geometry remains 1000 while asserting the expected layout delta after forcing layout, and retain the existing no-op coverage for equivalent cell spans.Tests/LibWeb/Layout/input/lang-change-casing-text-transform-relayout.html (1)
1-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
Layout::TextSliceNodepath.This test covers a regular text node under
text-transform: uppercase. It does not exerciseLanguageInvalidator.cppLines 39-43, where a language change must rebuild the layout tree for a first-letter slice. Add a::first-lettercase with a language change and update the expected dump.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/LibWeb/Layout/input/lang-change-casing-text-transform-relayout.html` around lines 1 - 6, Extend the HTML test to include a ::first-letter text-transform case that changes the element language from English to Turkish, exercising the Layout::TextSliceNode path and LanguageInvalidator relayout behavior. Update the corresponding expected layout dump to reflect the rebuilt first-letter layout after the language change, while preserving the existing regular-text coverage.Tests/LibWeb/Text/input/textarea-rows-attribute-relayout.html (1)
11-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover
colschanges in this regression test.The test changes only
rows, butLibraries/LibWeb/HTML/HTMLTextAreaElement.cpphandles bothrowsandcolson Lines 449-455. Add acolsmutation and verify that the width grows. This prevents a regression in the column-size path from passing unnoticed.Proposed test extension
const heightAfter = document.getElementById("field").getBoundingClientRect().height; + const widthBefore = document.getElementById("field").getBoundingClientRect().width; + document.getElementById("field").setAttribute("cols", "40"); + const widthAfter = document.getElementById("field").getBoundingClientRect().width; - println(`grew: ${heightAfter > heightBefore}`); + println(`grew: ${heightAfter > heightBefore && widthAfter > widthBefore}`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/LibWeb/Text/input/textarea-rows-attribute-relayout.html` around lines 11 - 15, Extend the regression test around the existing field measurements to cover the cols mutation as well as rows: record the textarea width before changing cols, set the cols attribute to a larger value, record the width after, and print/assert that the width increased. Keep the existing rows height-growth check unchanged and use the same field element.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Tests/LibWeb/Layout/input/lang-change-casing-text-transform-relayout.html`:
- Around line 1-6: Extend the HTML test to include a ::first-letter
text-transform case that changes the element language from English to Turkish,
exercising the Layout::TextSliceNode path and LanguageInvalidator relayout
behavior. Update the corresponding expected layout dump to reflect the rebuilt
first-letter layout after the language change, while preserving the existing
regular-text coverage.
In `@Tests/LibWeb/Text/input/table-colspan-change-relayout.html`:
- Around line 18-30: Extend the test beyond the existing HTMLTableCellElement
colspan case with a raw <col span> scenario changing from 1001 to 1002. Verify
the effective table-track geometry remains 1000 while asserting the expected
layout delta after forcing layout, and retain the existing no-op coverage for
equivalent cell spans.
In `@Tests/LibWeb/Text/input/textarea-rows-attribute-relayout.html`:
- Around line 11-15: Extend the regression test around the existing field
measurements to cover the cols mutation as well as rows: record the textarea
width before changing cols, set the cols attribute to a larger value, record the
width after, and print/assert that the width increased. Keep the existing rows
height-growth check unchanged and use the same field element.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c5a0ee51-be79-4a3c-852a-dd6768bfb4c8
📒 Files selected for processing (16)
Libraries/LibWeb/CSS/Invalidation/LanguageInvalidator.cppLibraries/LibWeb/DOM/Element.cppLibraries/LibWeb/DOM/Node.hLibraries/LibWeb/HTML/HTMLInputElement.cppLibraries/LibWeb/HTML/HTMLTextAreaElement.cppLibraries/LibWeb/Layout/Node.cppLibraries/LibWeb/Layout/Node.hLibraries/LibWeb/Rust/src/layout/layout_node_arena.rsTests/LibWeb/Layout/expected/lang-change-casing-text-transform-relayout.txtTests/LibWeb/Layout/input/lang-change-casing-text-transform-relayout.htmlTests/LibWeb/Text/expected/input-size-attribute-relayout.txtTests/LibWeb/Text/expected/table-colspan-change-relayout.txtTests/LibWeb/Text/expected/textarea-rows-attribute-relayout.txtTests/LibWeb/Text/input/input-size-attribute-relayout.htmlTests/LibWeb/Text/input/table-colspan-change-relayout.htmlTests/LibWeb/Text/input/textarea-rows-attribute-relayout.html
See commit descriptions