LibWeb: Make ComputedValues the durable computed style#10712
Conversation
Cover computed style access for display: contents, display: none, and pseudo-elements that do not generate layout boxes. Exercise logical and shorthand properties, colors, line height, and image serialization.
Introduce a final, reference-counted ComputedValues with a builder for derived styles. Preserve semantic line-height and color values while separating layout resource attachment from typed value construction.
Construct typed values during style computation and retain them on elements beside the transitional generic workspace. Carry importance, inheritance, dependency, pseudo-element, display-none, and animation metadata in the durable object.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Services/WebContent/ConnectionFromClient.cpp (1)
854-861: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRefresh
computed_valuesafter updating layout.
update_layout()can publish a new immutable style snapshot, but serialization retains the pointer obtained beforehand. This can expose stale container-query or other layout-dependent styles in DevTools.Proposed fix
- auto computed_values = element.computed_values(pseudo_element); - - if (!computed_values) { - async_did_inspect_dom_node(page_id, { property_type, {} }); - return; - } - node->document().update_layout(Web::DOM::UpdateLayoutReason::Debugging); + + auto computed_values = element.computed_values(pseudo_element); + if (!computed_values) { + async_did_inspect_dom_node(page_id, { property_type, {} }); + return; + }🤖 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 `@Services/WebContent/ConnectionFromClient.cpp` around lines 854 - 861, Refresh the computed style snapshot in the DOM inspection flow after node->document().update_layout(...) completes. Update the computed_values pointer used for serialization by calling element.computed_values(pseudo_element) again, while preserving the existing early return when no values are available.
🤖 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.
Inline comments:
In `@Libraries/LibWeb/CSS/ComputedProperties.cpp`:
- Around line 1502-1525: Update the TouchAction serialization branch to append
the pinch-zoom keyword whenever action.allow_pinch_zoom is true, including mixed
values such as pan-x pinch-zoom, while preserving the existing auto,
manipulation, none, and directional pan handling.
In `@Libraries/LibWeb/CSS/ComputedValues.cpp`:
- Around line 74-78: Move calculation and application of the element’s used
color scheme in the computed-values construction flow before the color
resolution at the affected color-handling code. Ensure system colors and
subsequent currentcolor values resolve using the element-specific scheme, while
retaining the existing color-scheme property setup and output assignment.
- Around line 503-506: Update the transition-property conversion in the loop
over CSS::PropertyID::TransitionProperty so the keyword all is represented
distinctly from none; do not use an empty Optional<Utf16FlyString> for both
cases. Preserve custom identifiers and ensure explicit all transitions remain
available to computed-style consumers.
- Around line 652-659: Update the extract_side lambda in ComputedValues.cpp to
resolve calculated overflow-clip-margin offsets, not just plain lengths. Use the
existing calculated-value resolution path for overflow_clip_margin.offset() so
valid calc(...) values produce their resolved CSSPixels amount instead of
defaulting to zero, while preserving the current behavior for unsupported or
non-length values.
- Around line 827-845: Update the `aspect_ratio` two-value list branch in
`ComputedValues` so an `auto <ratio>` value with a degenerate resolved ratio is
represented as auto, matching the bare-ratio branch. Preserve the existing
preferred-ratio assignment for non-degenerate ratios and the handling of
standalone `auto`.
In `@Libraries/LibWeb/CSS/Length.cpp`:
- Around line 269-281: Update the root_font_metrics initializer in the
Length::ResolutionContext construction to use
root_computed_values->line_height() instead of computed_values->line_height(),
while leaving the target element’s font_metrics unchanged.
In `@Libraries/LibWeb/CSS/StyleComputer.cpp`:
- Around line 1604-1610: Update the currentcolor handling in the style
comparison logic around originates_from_current_color so it suppresses the
difference only when the underlying non-animated color value is unchanged. Do
not mark before_change_style_is_different false solely because both declarations
use currentcolor; preserve transitions when the resolved color changes, such as
when color changes with border-color: currentcolor.
In `@Services/WebContent/ConnectionFromClient.cpp`:
- Around line 571-573: Change the property iteration loop in the computed-style
dump to use an inclusive upper bound so it also processes
Web::CSS::last_longhand_property_id, matching the inclusive iteration behavior
elsewhere in the inspection code.
---
Outside diff comments:
In `@Services/WebContent/ConnectionFromClient.cpp`:
- Around line 854-861: Refresh the computed style snapshot in the DOM inspection
flow after node->document().update_layout(...) completes. Update the
computed_values pointer used for serialization by calling
element.computed_values(pseudo_element) again, while preserving the existing
early return when no values are available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 111f42ad-3ed4-42eb-bdb3-397d59faca2b
📒 Files selected for processing (204)
Libraries/LibWeb/Animations/AnimationEffect.cppLibraries/LibWeb/Animations/KeyframeEffect.cppLibraries/LibWeb/Animations/KeyframeEffect.hLibraries/LibWeb/CMakeLists.txtLibraries/LibWeb/CSS/CSSStyleProperties.cppLibraries/LibWeb/CSS/ComputedProperties.cppLibraries/LibWeb/CSS/ComputedProperties.hLibraries/LibWeb/CSS/ComputedValues.cppLibraries/LibWeb/CSS/ComputedValues.hLibraries/LibWeb/CSS/ContainerQuery.cppLibraries/LibWeb/CSS/CountersSet.cppLibraries/LibWeb/CSS/EasingFunction.cppLibraries/LibWeb/CSS/EasingFunction.hLibraries/LibWeb/CSS/Enums.jsonLibraries/LibWeb/CSS/FontComputer.cppLibraries/LibWeb/CSS/Length.cppLibraries/LibWeb/CSS/StyleComputer.cppLibraries/LibWeb/CSS/StyleComputer.hLibraries/LibWeb/CSS/StyleInvalidation.cppLibraries/LibWeb/CSS/StyleInvalidation.hLibraries/LibWeb/CSS/StylePropertyMapReadOnly.cppLibraries/LibWeb/CSS/StyleSheetInvalidation.cppLibraries/LibWeb/CSS/StyleValues/ColorStyleValue.cppLibraries/LibWeb/CSS/StyleValues/CounterStyleStyleValue.hLibraries/LibWeb/CSS/StyleValues/ShorthandStyleValue.cppLibraries/LibWeb/CSS/StyleValues/StyleValue.cppLibraries/LibWeb/CSS/StyleValues/StyleValue.hLibraries/LibWeb/CSS/UpdateStyle.cppLibraries/LibWeb/CSS/UpdateStyle.hLibraries/LibWeb/DOM/AbstractElement.cppLibraries/LibWeb/DOM/AbstractElement.hLibraries/LibWeb/DOM/Document.cppLibraries/LibWeb/DOM/Document.hLibraries/LibWeb/DOM/Element.cppLibraries/LibWeb/DOM/Element.hLibraries/LibWeb/DOM/Node.cppLibraries/LibWeb/DOM/PseudoElement.cppLibraries/LibWeb/DOM/PseudoElement.hLibraries/LibWeb/Dump.cppLibraries/LibWeb/Forward.hLibraries/LibWeb/HTML/Canvas/AbstractCanvasMixin.cppLibraries/LibWeb/HTML/Canvas/CanvasTextDrawingStyles.cppLibraries/LibWeb/HTML/CanvasRenderingContext2D.cppLibraries/LibWeb/HTML/HTMLAudioElement.cppLibraries/LibWeb/HTML/HTMLAudioElement.hLibraries/LibWeb/HTML/HTMLBRElement.cppLibraries/LibWeb/HTML/HTMLBRElement.hLibraries/LibWeb/HTML/HTMLCanvasElement.cppLibraries/LibWeb/HTML/HTMLCanvasElement.hLibraries/LibWeb/HTML/HTMLElement.cppLibraries/LibWeb/HTML/HTMLFieldSetElement.cppLibraries/LibWeb/HTML/HTMLFieldSetElement.hLibraries/LibWeb/HTML/HTMLHtmlElement.cppLibraries/LibWeb/HTML/HTMLIFrameElement.cppLibraries/LibWeb/HTML/HTMLIFrameElement.hLibraries/LibWeb/HTML/HTMLImageElement.cppLibraries/LibWeb/HTML/HTMLImageElement.hLibraries/LibWeb/HTML/HTMLInputElement.cppLibraries/LibWeb/HTML/HTMLInputElement.hLibraries/LibWeb/HTML/HTMLLegendElement.cppLibraries/LibWeb/HTML/HTMLLegendElement.hLibraries/LibWeb/HTML/HTMLObjectElement.cppLibraries/LibWeb/HTML/HTMLObjectElement.hLibraries/LibWeb/HTML/HTMLSelectElement.cppLibraries/LibWeb/HTML/HTMLTableCellElement.cppLibraries/LibWeb/HTML/HTMLTextAreaElement.cppLibraries/LibWeb/HTML/HTMLTextAreaElement.hLibraries/LibWeb/HTML/HTMLVideoElement.cppLibraries/LibWeb/HTML/HTMLVideoElement.hLibraries/LibWeb/HTML/LocalNavigable.cppLibraries/LibWeb/Layout/AudioBox.cppLibraries/LibWeb/Layout/AudioBox.hLibraries/LibWeb/Layout/BlockContainer.cppLibraries/LibWeb/Layout/BlockContainer.hLibraries/LibWeb/Layout/BlockFormattingContext.cppLibraries/LibWeb/Layout/Box.cppLibraries/LibWeb/Layout/Box.hLibraries/LibWeb/Layout/BreakNode.cppLibraries/LibWeb/Layout/BreakNode.hLibraries/LibWeb/Layout/CanvasBox.cppLibraries/LibWeb/Layout/CanvasBox.hLibraries/LibWeb/Layout/CheckBox.cppLibraries/LibWeb/Layout/CheckBox.hLibraries/LibWeb/Layout/FieldSetBox.cppLibraries/LibWeb/Layout/FieldSetBox.hLibraries/LibWeb/Layout/FormattingContext.cppLibraries/LibWeb/Layout/ImageBox.cppLibraries/LibWeb/Layout/ImageBox.hLibraries/LibWeb/Layout/InlineNode.cppLibraries/LibWeb/Layout/InlineNode.hLibraries/LibWeb/Layout/LegendBox.cppLibraries/LibWeb/Layout/LegendBox.hLibraries/LibWeb/Layout/ListItemBox.cppLibraries/LibWeb/Layout/ListItemBox.hLibraries/LibWeb/Layout/ListItemMarkerBox.cppLibraries/LibWeb/Layout/ListItemMarkerBox.hLibraries/LibWeb/Layout/NavigableContainerViewport.cppLibraries/LibWeb/Layout/NavigableContainerViewport.hLibraries/LibWeb/Layout/Node.cppLibraries/LibWeb/Layout/Node.hLibraries/LibWeb/Layout/RadioButton.cppLibraries/LibWeb/Layout/RadioButton.hLibraries/LibWeb/Layout/RangeInputBox.cppLibraries/LibWeb/Layout/RangeInputBox.hLibraries/LibWeb/Layout/ReplacedBox.cppLibraries/LibWeb/Layout/ReplacedBox.hLibraries/LibWeb/Layout/SVGBox.cppLibraries/LibWeb/Layout/SVGBox.hLibraries/LibWeb/Layout/SVGClipBox.cppLibraries/LibWeb/Layout/SVGClipBox.hLibraries/LibWeb/Layout/SVGForeignObjectBox.cppLibraries/LibWeb/Layout/SVGForeignObjectBox.hLibraries/LibWeb/Layout/SVGGeometryBox.cppLibraries/LibWeb/Layout/SVGGeometryBox.hLibraries/LibWeb/Layout/SVGGraphicsBox.cppLibraries/LibWeb/Layout/SVGGraphicsBox.hLibraries/LibWeb/Layout/SVGImageBox.cppLibraries/LibWeb/Layout/SVGImageBox.hLibraries/LibWeb/Layout/SVGMaskBox.cppLibraries/LibWeb/Layout/SVGMaskBox.hLibraries/LibWeb/Layout/SVGPatternBox.cppLibraries/LibWeb/Layout/SVGPatternBox.hLibraries/LibWeb/Layout/SVGSVGBox.cppLibraries/LibWeb/Layout/SVGSVGBox.hLibraries/LibWeb/Layout/SVGTextBox.cppLibraries/LibWeb/Layout/SVGTextBox.hLibraries/LibWeb/Layout/SVGTextPathBox.cppLibraries/LibWeb/Layout/SVGTextPathBox.hLibraries/LibWeb/Layout/TableWrapper.cppLibraries/LibWeb/Layout/TableWrapper.hLibraries/LibWeb/Layout/TextAreaBox.cppLibraries/LibWeb/Layout/TextAreaBox.hLibraries/LibWeb/Layout/TextInputBox.cppLibraries/LibWeb/Layout/TextInputBox.hLibraries/LibWeb/Layout/TreeBuilder.cppLibraries/LibWeb/Layout/TreeBuilder.hLibraries/LibWeb/Layout/VideoBox.cppLibraries/LibWeb/Layout/VideoBox.hLibraries/LibWeb/Layout/Viewport.cppLibraries/LibWeb/Layout/Viewport.hLibraries/LibWeb/Painting/AccumulatedVisualContext.cppLibraries/LibWeb/Painting/BackgroundPainting.cppLibraries/LibWeb/Painting/Paintable.cppLibraries/LibWeb/Painting/Paintable.hLibraries/LibWeb/Painting/StackingContext.cppLibraries/LibWeb/SVG/SVGAElement.cppLibraries/LibWeb/SVG/SVGAElement.hLibraries/LibWeb/SVG/SVGClipPathElement.cppLibraries/LibWeb/SVG/SVGClipPathElement.hLibraries/LibWeb/SVG/SVGDecodedImageData.cppLibraries/LibWeb/SVG/SVGDefsElement.hLibraries/LibWeb/SVG/SVGDescElement.cppLibraries/LibWeb/SVG/SVGDescElement.hLibraries/LibWeb/SVG/SVGElement.cppLibraries/LibWeb/SVG/SVGFEDropShadowElement.cppLibraries/LibWeb/SVG/SVGFEFloodElement.cppLibraries/LibWeb/SVG/SVGFEFloodElement.hLibraries/LibWeb/SVG/SVGFilterElement.cppLibraries/LibWeb/SVG/SVGForeignObjectElement.cppLibraries/LibWeb/SVG/SVGForeignObjectElement.hLibraries/LibWeb/SVG/SVGGElement.cppLibraries/LibWeb/SVG/SVGGElement.hLibraries/LibWeb/SVG/SVGGeometryElement.cppLibraries/LibWeb/SVG/SVGGeometryElement.hLibraries/LibWeb/SVG/SVGGradientElement.cppLibraries/LibWeb/SVG/SVGImageElement.cppLibraries/LibWeb/SVG/SVGImageElement.hLibraries/LibWeb/SVG/SVGMaskElement.cppLibraries/LibWeb/SVG/SVGMaskElement.hLibraries/LibWeb/SVG/SVGMetadataElement.cppLibraries/LibWeb/SVG/SVGMetadataElement.hLibraries/LibWeb/SVG/SVGPatternElement.cppLibraries/LibWeb/SVG/SVGPatternElement.hLibraries/LibWeb/SVG/SVGSVGElement.cppLibraries/LibWeb/SVG/SVGSVGElement.hLibraries/LibWeb/SVG/SVGStopElement.cppLibraries/LibWeb/SVG/SVGSwitchElement.cppLibraries/LibWeb/SVG/SVGSwitchElement.hLibraries/LibWeb/SVG/SVGSymbolElement.cppLibraries/LibWeb/SVG/SVGSymbolElement.hLibraries/LibWeb/SVG/SVGTSpanElement.cppLibraries/LibWeb/SVG/SVGTSpanElement.hLibraries/LibWeb/SVG/SVGTextElement.cppLibraries/LibWeb/SVG/SVGTextElement.hLibraries/LibWeb/SVG/SVGTextPathElement.cppLibraries/LibWeb/SVG/SVGTextPathElement.hLibraries/LibWeb/SVG/SVGTitleElement.cppLibraries/LibWeb/SVG/SVGTitleElement.hLibraries/LibWeb/SVG/SVGUseElement.cppLibraries/LibWeb/SVG/SVGUseElement.hServices/WebContent/ConnectionFromClient.cppServices/WebContent/WebDriverConnection.cppTests/LibWeb/Text/expected/css/computed-style-map-counters.txtTests/LibWeb/Text/expected/css/computed-style-without-layout-box.txtTests/LibWeb/Text/expected/css/computed-values-svg-geometry.txtTests/LibWeb/Text/expected/css/list-style-symbols-inherit.txtTests/LibWeb/Text/expected/css/shape-image-threshold-inherit.txtTests/LibWeb/Text/expected/wpt-import/css/css-backgrounds/animations/background-color-interpolation.txtTests/LibWeb/Text/expected/wpt-import/css/css-backgrounds/animations/border-color-interpolation.txtTests/LibWeb/Text/input/css/computed-style-map-counters.htmlTests/LibWeb/Text/input/css/computed-style-without-layout-box.htmlTests/LibWeb/Text/input/css/computed-values-svg-geometry.htmlTests/LibWeb/Text/input/css/list-style-symbols-inherit.htmlTests/LibWeb/Text/input/css/shape-image-threshold-inherit.html
Reconstruct core property families from ComputedValues and redirect CSSOM, DOM, HTML, SVG, painting, container queries, and traversal state to typed access. Preserve currentcolor-sensitive data, font details, and remaining inherited state needed by the transitional algorithm.
Complete typed storage and reconstruction for layout, CSSOM, animation, invalidation, generated content, and remaining engine clients. Preserve exact syntax and computed semantics for inherited, color, shape, border, list, image, anchor, timeline, and SVG values.
Reconstruct transient generic workspaces for animation and inherited style updates, then remove durable ComputedProperties storage from elements and pseudo-elements. Return immutable ComputedValues directly from style computation and keep mutable access confined to consuming builders. Handle CSS animations whose effects are temporarily detached by script.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Style computation currently produces a generic
ComputedPropertiesarray ofStyleValueobjects and stores it on elements. Layout then projects that array into the typedComputedValuesused by layout and painting. This represents the same computed style twice, places CSS value conversion in layout, and makes typed style information depend on layout-node creation.This is also the wrong boundary for the planned Rust migration. Porting
StyleComputerin the current architecture would require migrating or bridging the durable generic property array and much of its downstream machinery. The intended boundary is simpler: style computation should return one immutable, typed result that existing C++ consumers can use directly.Invert the relationship between these types. Keep
ComputedPropertiesas a transient workspace used internally while applying the cascade, resolving inheritance, and calculating one style. At the end of computation, construct and return an immutableComputedValues. Store that object on elements and pseudo-elements, and share it directly with ordinary layout nodes.Keep
StyleValuefor declarations, custom properties, animation interpolation, and other cases where preserving generic CSS syntax is useful. Reconstruct temporaryStyleValueobjects fromComputedValueswhen CSSOM, inheritance, or animation code specifically requires them.This change also:
currentcolor, and calculation resolution.display: none,display: contents, and pseudo-elements without layout boxes.ComputedValues::Builderfor anonymous and layout-adjusted styles while publishing only immutable results.Add characterization coverage for no-box computed styles, logical and shorthand serialization, inherited values, colors, line height, images, SVG geometry, animation interpolation, and semantic distinctions that must survive typed storage.
This does not port style computation to Rust. It establishes the boundary needed for that work: a future Rust
StyleComputercan produceComputedValuesdirectly, while the existing C++ engine consumes a compact typed result instead of depending on the transient generic computation workspace. This substantially reduces the amount of machinery that must be ported or exposed across the language boundary.Summary by CodeRabbit
scroll-behavior: autoandsmooth.display:nonescenarios.getComputedStyle(),computedStyleMap(), and inspection tools.