LibWeb: Resolve substituted relative URLs against the style sheet - #10792
LibWeb: Resolve substituted relative URLs against the style sheet#10792tcl3 wants to merge 3 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (10)
📝 WalkthroughWalkthroughThis change propagates stylesheet context through CSS declarations, nested declarations, cascaded values, shorthand expansion, substituted values, and keyframe animations. Rust bulk-cascade callbacks now pass declaration source IDs to C++. Keyframe sets retain their source stylesheets. New tests validate stylesheet-relative URL substitution across nested rules, shorthands, keyframes, CSSOM updates, and imported WPT cases. Sequence Diagram(s)sequenceDiagram
participant CSSStyleSheet
participant StyleComputer
participant RustCascadedProperties
participant StyleValue
CSSStyleSheet->>StyleComputer: provide declaration source
RustCascadedProperties->>StyleComputer: pass source_id during cascade
StyleComputer->>CSSStyleSheet: resolve parent stylesheet
StyleComputer->>StyleValue: attach stylesheet context
StyleComputer-->>RustCascadedProperties: return resolved value
Possibly related PRs
Suggested reviewers: Mergeability Score: ⚪ Minimal · up to Substituted relative URLs will resolve against the containing stylesheet as intended. The remaining notes concern formatting and preserving an existing interface, with no actionable user or production impact; no merge-blocking risk remains after normal checks. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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
🤖 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/CSSStyleProperties.cpp`:
- Around line 447-449: Propagate stylesheet context to custom-property values on
every declaration path: in CSSStyleProperties.cpp lines 447-449, move the
style-sheet assignment before the custom-property early return; in
CSSStyleProperties.cpp lines 1654-1658, update values in m_custom_properties as
well as m_properties; and in CSSKeyframeRule.cpp lines 33-40, propagate context
to keyframe custom properties, preferably by reusing the shared
declaration-block helper.
In `@Tests/LibWeb/Text/input/css/support/url-substitution-base.css`:
- Line 17: Add the required blank line immediately before the background-image
declaration using the --url custom property, preserving the declaration itself
unchanged.
🪄 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: a13ac9fb-c442-4263-836e-656a8d24a183
📒 Files selected for processing (26)
Libraries/LibWeb/CSS/CSSImportRule.cppLibraries/LibWeb/CSS/CSSKeyframeRule.cppLibraries/LibWeb/CSS/CSSKeyframeRule.hLibraries/LibWeb/CSS/CSSKeyframesRule.cppLibraries/LibWeb/CSS/CSSKeyframesRule.hLibraries/LibWeb/CSS/CSSNestedDeclarations.cppLibraries/LibWeb/CSS/CSSNestedDeclarations.hLibraries/LibWeb/CSS/CSSStyleProperties.cppLibraries/LibWeb/CSS/CSSStyleSheet.cppLibraries/LibWeb/CSS/CSSStyleSheet.hLibraries/LibWeb/CSS/Parser/Parser.hLibraries/LibWeb/CSS/Parser/ValueParsing.cppLibraries/LibWeb/CSS/StyleSheetList.cppLibraries/LibWeb/CSS/StyleSheetResourceContext.hLibraries/LibWeb/CSS/StyleValues/ImageStyleValue.cppLibraries/LibWeb/CSS/StyleValues/ImageStyleValue.hLibraries/LibWeb/CSS/StyleValues/StyleValue.cppLibraries/LibWeb/CSS/StyleValues/UnresolvedStyleValue.cppLibraries/LibWeb/CSS/StyleValues/UnresolvedStyleValue.hTests/LibWeb/Text/expected/css/url-substitution-resolves-relative-to-stylesheet.txtTests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-attr.txtTests/LibWeb/Text/expected/wpt-import/css/css-pseudo/parsing/marker-supported-properties-in-animation.txtTests/LibWeb/Text/expected/wpt-import/css/css-values/urls/resolve-relative-to-base.sub.txtTests/LibWeb/Text/expected/wpt-import/css/css-values/urls/resolve-relative-to-stylesheet.txtTests/LibWeb/Text/input/css/support/url-substitution-base.cssTests/LibWeb/Text/input/css/url-substitution-resolves-relative-to-stylesheet.html
f56913f to
24240b4
Compare
24240b4 to
0ac454f
Compare
|
My latest push updates this branch to make sure it works correctly with the Rust changes introduced in #10811. |
github-actions
Bot
added
the
conflicts
|
Your pull request has conflicts that need to be resolved before it can be reviewed and merged. Make sure to rebase your branch on top of the latest |
0ac454f to
66d32ba
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
github-actions
Bot
removed
the
conflicts
github-actions
Bot
added
the
conflicts
|
Your pull request has conflicts that need to be resolved before it can be reviewed and merged. Make sure to rebase your branch on top of the latest |
Previously, relative URLs in declarations following a nested rule were resolved against the document base URL instead of the style sheet's base URL.
Previously, relative URLs in declaration values set through the CSSOM were resolved against the document base URL instead of the base URL of the style sheet containing the declaration.
Declarations containing substitution functions are re-parsed at computed-value time. Previously, this re-parse only knew about the document, so any relative URL produced by substitution was resolved against the document base URL instead of the base URL of the style sheet containing the declaration. The cascade already tracks the source declaration of each applied block. We now pass that source through the substitution callbacks and attach the source's style sheet to each freshly resolved value. The existing source slot machinery restores this context whenever the value is materialized again. Shorthand expansion forwards the context marker to each longhand, so values substituted into a shorthand are restored as well. Keyframe values are resolved outside the cascade. Each keyframe set now remembers the style sheet containing its rule, and animated values receive that context when they are materialized.
66d32ba to
7d5c76b
Compare
github-actions
Bot
removed
the
conflicts
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Rebased to resolve conflicts. |
2 participants
Declarations containing substitution functions are re-parsed at computed-value time. Previously, this re-parse only knew about the document, so any relative URL produced by substitution was resolved against the document base URL instead of the URL of the style sheet containing the declaration.
We now capture the style sheet's base URL on unresolved values when they are attached to their style sheet, and give that context to the parser when resolving them, so URL values created during substitution resolve and serialize like their literally-specified equivalents.
Fixes #10771