LibWeb: Implement CSS custom function evaluation#10512
Open
Calme1709 wants to merge 23 commits into
Open
Conversation
|
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 |
github-actions
Bot
added
the
conflicts
Calme1709
force-pushed
the
dashed-function-evaluation
branch
from
July 8, 2026 23:39
83b5deb to
2a0d326
Compare
github-actions
Bot
added
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 |
Calme1709
force-pushed
the
dashed-function-evaluation
branch
from
July 10, 2026 02:00
2a0d326 to
e47c9c7
Compare
github-actions
Bot
removed
the
conflicts
Member
Author
|
Rebased to resolve conflicts |
AtkinsSJ
reviewed
Jul 10, 2026
AtkinsSJ
left a comment
AtkinsSJ
left a comment
Member
There was a problem hiding this comment.
The Windows build is unhappy. Looks like either the CSSPropertyRule destructor implementation needs moving, or you need an include.
Calme1709
force-pushed
the
dashed-function-evaluation
branch
from
July 10, 2026 12:58
e47c9c7 to
db5a218
Compare
Calme1709
commented
Jul 10, 2026
Calme1709
left a comment
Calme1709
left a comment
Member
Author
There was a problem hiding this comment.
I have moved the destructor out of line - hopefully that fixes it.
Calme1709
force-pushed
the
dashed-function-evaluation
branch
from
July 10, 2026 23:45
db5a218 to
c2fea3e
Compare
Member
Author
|
Rebased to fix tests that were failing on previous master |
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 |
Calme1709
force-pushed
the
dashed-function-evaluation
branch
from
July 14, 2026 12:40
c2fea3e to
aea8935
Compare
github-actions
Bot
removed
the
conflicts
Member
Author
|
Rebased to resolve 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 |
Calme1709
force-pushed
the
dashed-function-evaluation
branch
from
July 17, 2026 00:04
aea8935 to
d40d3b6
Compare
github-actions
Bot
removed
the
conflicts
|
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:
📝 WalkthroughWalkthroughThis PR implements CSS Custom Functions with dashed-function parsing and evaluation, hypothetical-element style resolution, custom-property inheritance and computation changes, CSS-wide keyword descriptor metadata, and extensive WPT coverage. ChangesCSS Custom Functions Implementation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Parser
participant ArbitrarySubstitutionFunctions
participant StyleScope
participant CSSFunctionRule
participant StyleComputer
Parser->>ArbitrarySubstitutionFunctions: substitute dashed function
ArbitrarySubstitutionFunctions->>StyleScope: get_function_definition(name)
StyleScope-->>ArbitrarySubstitutionFunctions: CSSFunctionRule
ArbitrarySubstitutionFunctions->>CSSFunctionRule: evaluate_a_custom_function(arguments, context)
CSSFunctionRule->>StyleComputer: compute_value_of_custom_property
StyleComputer-->>CSSFunctionRule: computed values
CSSFunctionRule-->>ArbitrarySubstitutionFunctions: result value
ArbitrarySubstitutionFunctions-->>Parser: substituted component values
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
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: 13
🤖 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 `@Documentation/CSSGeneratedFiles.md`:
- Around line 198-203: Update the regular descriptor table in
Documentation/CSSGeneratedFiles.md to include allow-css-wide-keywords with the
same default, requirement, and description as the custom descriptor entry.
Ensure the documentation reflects that this field is supported by regular
descriptors, including function.result.
In `@Libraries/LibWeb/CSS/ContainerQuery.cpp`:
- Around line 539-552: Update the computed-style resolution path around
reconstruct_computed_properties in the style-query evaluation flow to reuse a
cached reconstructed ComputedProperties instance instead of rebuilding it for
every candidate element. Preserve the existing fallback behavior when
computed_values is unavailable, and ensure the cached object remains valid for
custom-property resolution.
In `@Libraries/LibWeb/CSS/CSSFunctionRule.cpp`:
- Around line 314-320: Update the for_each_effective_rule traversal in
CSSFunctionRule so `@container` conditions are evaluated against the
hypothetical/calling-context element before processing CSSFunctionDeclarations.
Only insert descriptors from rules whose container conditions match, preserving
the existing body_rule.set behavior for applicable declarations.
- Around line 411-415: Update compute_value_of_custom_property() and the
function-resolution path to retain function-specific keyword-resolution context
through substitutions. After var(), if(), or dashed-function resolution, reapply
the CSSFunctionRule handling so substituted unset, revert, and revert-layer
values become GuaranteedInvalidStyleValue, matching the existing checks around
the function rule and the corresponding logic at the additional location.
In `@Libraries/LibWeb/CSS/StyleScope.cpp`:
- Around line 1459-1497: Update StyleScope::get_function_definition so candidate
selection is performed independently for each cascade origin using that origin’s
rule_cache().qualified_layer_names_in_order, rather than comparing all
candidates through one shared result and cache. After selecting the winning
function within UserAgent, User, and Author scans, apply cascade-origin
precedence so Author overrides User, which overrides UserAgent, while preserving
layer ordering within each origin.
In
`@Tests/LibWeb/Text/expected/wpt-import/css/css-mixins/dashed-function-named-arg.tentative.txt`:
- Around line 5-6: Update the dashed-function argument parser to reject empty
named arguments, including whitespace-only values, so declarations such as
--func(--myident:) leave the property unset while valid named arguments remain
accepted. Then regenerate the expected output for
dashed-function-named-arg.tentative.txt to reflect the corrected pass/fail
results.
In `@Tests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-attr.txt`:
- Around line 5-6: Fix the attr()-taint and URL propagation paths affecting
custom-function arguments, defaults, locals, inheritance, and parent frames,
including both typed and untyped url() results. Update the relevant
implementation symbols that propagate these values, then regenerate the
function-attr expected snapshot so it reflects the corrected behavior instead of
the current failures.
In
`@Tests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-conditionals.txt`:
- Around line 20-28: Complete custom-function handling for container-dependent
conditionals: ensure nested and mixed `@container`, `@media`, and `@supports`
conditions evaluate correctly, and invalidate and recompute cached function
results when container conditions change. Update both
Tests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-conditionals.txt
lines 20-28 and function-container-dynamic.txt lines 5-6 as needed to reflect
the corrected behavior.
- Around line 5-6: Remove the five `@container-related` failures from the expected
baseline in function-conditionals.txt and fix the underlying `@container`
evaluation path used by custom-function resolution so Basic, Nested, Locals
within, `@container` within `@supports`, and combined `@supports/`@media/@container
cases pass instead of being accepted as expected failures.
In
`@Tests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-container-self.txt`:
- Around line 5-6: Fix the hypothetical custom-function evaluation context so
container-query target resolution uses the correct element: self queries must
resolve against the queried element, while style queries must use the
parent/originating element. Apply the root fix in the container-query resolution
path used during custom-function evaluation. Update
Tests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-container-self.txt:5-6
to reflect passing self-query behavior, and
Tests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-container-style.txt:5-6
to reflect the corrected style-query behavior.
In `@Tests/LibWeb/Text/input/wpt-import/css/css-mixins/dashed-function-eval.html`:
- Line 2: Update the title text in the custom-functions test so the closing
angle bracket in the “<dashed-function>” label is HTML-escaped, satisfying the
configured HTMLHint rule while preserving the displayed title.
In
`@Tests/LibWeb/Text/input/wpt-import/css/css-mixins/dashed-function-standard-property.html`:
- Around line 20-25: Update the padding declaration in the `#target` test case to
call --ten-px-typed() instead of --ten-px(), ensuring the shorthand padding
property exercises the typed dashed-function path without duplicating the width
coverage.
In `@Tests/LibWeb/Text/input/wpt-import/css/css-mixins/function-attr.html`:
- Around line 195-218: Define the custom function --g() inside the “Returned
url() is attr-tainted, initial” template, matching the helper definition used by
the preceding test so --f() resolves var(--x) through --g(). Keep the existing
--x: initial setup and expected result 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: 5780ac14-4a76-4021-99c6-73fd1547d9ab
📒 Files selected for processing (85)
Documentation/CSSGeneratedFiles.mdLibraries/LibWeb/CMakeLists.txtLibraries/LibWeb/CSS/BooleanExpression.hLibraries/LibWeb/CSS/CSSFunctionDeclarations.hLibraries/LibWeb/CSS/CSSFunctionRule.cppLibraries/LibWeb/CSS/CSSFunctionRule.hLibraries/LibWeb/CSS/CSSPropertyRule.cppLibraries/LibWeb/CSS/CSSPropertyRule.hLibraries/LibWeb/CSS/CSSStyleProperties.cppLibraries/LibWeb/CSS/CSSStyleSheet.cppLibraries/LibWeb/CSS/CSSStyleSheet.hLibraries/LibWeb/CSS/ContainerQuery.cppLibraries/LibWeb/CSS/CustomPropertyData.cppLibraries/LibWeb/CSS/CustomPropertyData.hLibraries/LibWeb/CSS/CustomPropertyRegistration.cppLibraries/LibWeb/CSS/CustomPropertyRegistration.hLibraries/LibWeb/CSS/Descriptors.jsonLibraries/LibWeb/CSS/HypotheticalElement.cppLibraries/LibWeb/CSS/HypotheticalElement.hLibraries/LibWeb/CSS/Parser/ArbitrarySubstitutionFunctions.cppLibraries/LibWeb/CSS/Parser/ArbitrarySubstitutionFunctions.hLibraries/LibWeb/CSS/Parser/DescriptorParsing.cppLibraries/LibWeb/CSS/Parser/Parser.cppLibraries/LibWeb/CSS/Parser/Parser.hLibraries/LibWeb/CSS/Parser/RuleParsing.cppLibraries/LibWeb/CSS/Parser/Types.hLibraries/LibWeb/CSS/Parser/ValueParsing.cppLibraries/LibWeb/CSS/PropertyNameAndID.hLibraries/LibWeb/CSS/StyleComputer.cppLibraries/LibWeb/CSS/StyleComputer.hLibraries/LibWeb/CSS/StylePropertyMapReadOnly.cppLibraries/LibWeb/CSS/StyleScope.cppLibraries/LibWeb/CSS/StyleScope.hLibraries/LibWeb/CSS/StyleSheetInvalidation.cppLibraries/LibWeb/DOM/Document.cppLibraries/LibWeb/DOM/Document.hLibraries/LibWeb/Forward.hMeta/Generators/generate_libweb_css_descriptors.pyTests/LibWeb/Text/expected/wpt-import/css/css-mixins/dashed-function-cycles.txtTests/LibWeb/Text/expected/wpt-import/css/css-mixins/dashed-function-eval.txtTests/LibWeb/Text/expected/wpt-import/css/css-mixins/dashed-function-named-arg.tentative.txtTests/LibWeb/Text/expected/wpt-import/css/css-mixins/dashed-function-parsing.txtTests/LibWeb/Text/expected/wpt-import/css/css-mixins/dashed-function-standard-property.txtTests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-attr.txtTests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-conditionals.txtTests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-container-dynamic.txtTests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-container-self.txtTests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-container-style.txtTests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-in-media.txtTests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-invalidation.txtTests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-layer.txtTests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-media-dynamic.txtTests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-parameter-types.tentative.txtTests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-shadow-animations.txtTests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-shadow-cache.txtTests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-shadow-container.txtTests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-shadow.txtTests/LibWeb/Text/expected/wpt-import/css/css-mixins/local-attr-substitution.txtTests/LibWeb/Text/expected/wpt-import/css/css-mixins/local-if-substitution.txtTests/LibWeb/Text/expected/wpt-import/css/css-mixins/local-inherit-substitution.txtTests/LibWeb/Text/expected/wpt-import/css/css-mixins/local-var-substitution.txtTests/LibWeb/Text/expected/wpt-import/css/css-variables/variable-css-wide-keywords.txtTests/LibWeb/Text/input/wpt-import/css/css-mixins/dashed-function-cycles.htmlTests/LibWeb/Text/input/wpt-import/css/css-mixins/dashed-function-eval.htmlTests/LibWeb/Text/input/wpt-import/css/css-mixins/dashed-function-named-arg.tentative.htmlTests/LibWeb/Text/input/wpt-import/css/css-mixins/dashed-function-parsing.htmlTests/LibWeb/Text/input/wpt-import/css/css-mixins/dashed-function-standard-property.htmlTests/LibWeb/Text/input/wpt-import/css/css-mixins/function-attr.htmlTests/LibWeb/Text/input/wpt-import/css/css-mixins/function-conditionals.htmlTests/LibWeb/Text/input/wpt-import/css/css-mixins/function-container-dynamic.htmlTests/LibWeb/Text/input/wpt-import/css/css-mixins/function-container-self.htmlTests/LibWeb/Text/input/wpt-import/css/css-mixins/function-container-style.htmlTests/LibWeb/Text/input/wpt-import/css/css-mixins/function-in-media.htmlTests/LibWeb/Text/input/wpt-import/css/css-mixins/function-invalidation.htmlTests/LibWeb/Text/input/wpt-import/css/css-mixins/function-layer.htmlTests/LibWeb/Text/input/wpt-import/css/css-mixins/function-media-dynamic.htmlTests/LibWeb/Text/input/wpt-import/css/css-mixins/function-parameter-types.tentative.htmlTests/LibWeb/Text/input/wpt-import/css/css-mixins/function-shadow-animations.htmlTests/LibWeb/Text/input/wpt-import/css/css-mixins/function-shadow-cache.htmlTests/LibWeb/Text/input/wpt-import/css/css-mixins/function-shadow-container.htmlTests/LibWeb/Text/input/wpt-import/css/css-mixins/function-shadow.htmlTests/LibWeb/Text/input/wpt-import/css/css-mixins/local-attr-substitution.htmlTests/LibWeb/Text/input/wpt-import/css/css-mixins/local-if-substitution.htmlTests/LibWeb/Text/input/wpt-import/css/css-mixins/local-inherit-substitution.htmlTests/LibWeb/Text/input/wpt-import/css/css-mixins/local-var-substitution.html
💤 Files with no reviewable changes (1)
- Libraries/LibWeb/DOM/Document.h
Comment on lines
+314
to
+320
| // FIXME: Respect container rules here. | ||
| for_each_effective_rule(TraversalOrder::Preorder, [&](CSSRule const& rule) { | ||
| if (auto const* declarations = as_if<CSSFunctionDeclarations>(rule)) { | ||
| for (auto const& descriptor : declarations->style()->descriptors()) | ||
| body_rule.set(descriptor.descriptor_name_and_id.name(), { .important = Important::No, .property_id = PropertyID::Custom, .value = descriptor.value }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Evaluate @container conditions before collecting function declarations.
This traversal currently includes declarations from non-matching container branches, allowing them to overwrite the function body’s actual result. Resolve container conditions against the hypothetical/calling-context element before inserting their descriptors.
🤖 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 `@Libraries/LibWeb/CSS/CSSFunctionRule.cpp` around lines 314 - 320, Update the
for_each_effective_rule traversal in CSSFunctionRule so `@container` conditions
are evaluated against the hypothetical/calling-context element before processing
CSSFunctionDeclarations. Only insert descriptors from rules whose container
conditions match, preserving the existing body_rule.set behavior for applicable
declarations.
Comment on lines
+20
to
+28
| Fail Basic @container | ||
| Pass Basic @container (false) | ||
| Fail Nested @container | ||
| Pass Nested @container (false) | ||
| Fail Locals within @container | ||
| Pass @supports within @container | ||
| Fail @container within @supports | ||
| Pass @container, @media, @supports | ||
| Fail @supports, @media, @container No newline at end of file |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Complete @container evaluation and invalidation for custom functions.
The failing conditional and dynamic tests show that container-dependent function results are not consistently evaluated or refreshed.
Tests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-conditionals.txt#L20-L28: fix nested and mixed@containerconditional evaluation.Tests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-container-dynamic.txt#L5-L6: invalidate and recompute results after container changes.
📍 Affects 2 files
Tests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-conditionals.txt#L20-L28(this comment)Tests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-container-dynamic.txt#L5-L6
🤖 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/expected/wpt-import/css/css-mixins/function-conditionals.txt`
around lines 20 - 28, Complete custom-function handling for container-dependent
conditionals: ensure nested and mixed `@container`, `@media`, and `@supports`
conditions evaluate correctly, and invalidate and recompute cached function
results when container conditions change. Update both
Tests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-conditionals.txt
lines 20-28 and function-container-dynamic.txt lines 5-6 as needed to reflect
the corrected behavior.
Comment on lines
+5
to
+6
| 1 Fail | ||
| Fail Custom Functions: @container can not query self No newline at end of file |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Fix scoped container-query target resolution.
The self-query and style-query failures point to an incorrect element/context being used during hypothetical custom-function evaluation.
Tests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-container-self.txt#L5-L6: enforce the correct self-query behavior.Tests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-container-style.txt#L5-L6: use the parent/originating element required by style queries.
📍 Affects 2 files
Tests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-container-self.txt#L5-L6(this comment)Tests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-container-style.txt#L5-L6
🤖 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/expected/wpt-import/css/css-mixins/function-container-self.txt`
around lines 5 - 6, Fix the hypothetical custom-function evaluation context so
container-query target resolution uses the correct element: self queries must
resolve against the queried element, while style queries must use the
parent/originating element. Apply the root fix in the container-query resolution
path used during custom-function evaluation. Update
Tests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-container-self.txt:5-6
to reflect passing self-query behavior, and
Tests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-container-style.txt:5-6
to reflect the corrected style-query behavior.
| @@ -0,0 +1,1318 @@ | |||
| <!DOCTYPE html> | |||
| <title>Custom Functions: Evaluating a <dashed-function></title> | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Escape the closing angle bracket in the title.
Line 2 leaves > unescaped, which fails the configured HTMLHint check.
Proposed fix
-<title>Custom Functions: Evaluating a <dashed-function></title>
+<title>Custom Functions: Evaluating a <dashed-function></title>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <title>Custom Functions: Evaluating a <dashed-function></title> | |
| <title>Custom Functions: Evaluating a <dashed-function></title> |
🧰 Tools
🪛 HTMLHint (1.9.2)
[error] 2-2: Special characters must be escaped : [ > ].
(spec-char-escape)
🤖 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/wpt-import/css/css-mixins/dashed-function-eval.html`
at line 2, Update the title text in the custom-functions test so the closing
angle bracket in the “<dashed-function>” label is HTML-escaped, satisfying the
configured HTMLHint rule while preserving the displayed title.
Source: Linters/SAST tools
Comment on lines
+20
to
+25
| #target { | ||
| width: --ten-px(); | ||
| height: --ten-px-typed(); | ||
| color: --green(); | ||
| padding: --ten-px(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use --ten-px-typed() in the shorthand case.
padding still uses --ten-px(), so the '<dashed-function> with typed result in shorthand' case never exercises the typed path and overlaps with the width coverage.
Proposed fix
`#target` {
width: --ten-px();
height: --ten-px-typed();
color: --green();
- padding: --ten-px();
+ padding: --ten-px-typed();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #target { | |
| width: --ten-px(); | |
| height: --ten-px-typed(); | |
| color: --green(); | |
| padding: --ten-px(); | |
| } | |
| `#target` { | |
| width: --ten-px(); | |
| height: --ten-px-typed(); | |
| color: --green(); | |
| padding: --ten-px-typed(); | |
| } |
🤖 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/wpt-import/css/css-mixins/dashed-function-standard-property.html`
around lines 20 - 25, Update the padding declaration in the `#target` test case to
call --ten-px-typed() instead of --ten-px(), ensuring the shorthand padding
property exercises the typed dashed-function path without duplicating the width
coverage.
Comment on lines
+195
to
+218
| <template data-name="Returned url() is attr-tainted, parent stack frame"> | ||
| <style> | ||
| @function --f() { | ||
| --x: attr(data-cat type(*)); | ||
| result: --g(); | ||
| } | ||
| @function --g() { | ||
| result: var(--x); | ||
| } | ||
| #actual { list-style-image: --f(); } | ||
| #expected { list-style-image: url(parent); } | ||
| </style> | ||
| </template> | ||
|
|
||
| <template data-name="Returned url() is attr-tainted, initial"> | ||
| <style> | ||
| @function --f(--x: attr(data-cat type(*))) { | ||
| --x: initial; | ||
| result: --g(); | ||
| } | ||
| #actual { list-style-image: --f(); } | ||
| #expected { list-style-image: url(parent); } | ||
| </style> | ||
| </template> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch the canonical WPT source for function-attr.html to check the "initial" template's --g() definition.
curl -s https://raw.githubusercontent.com/web-platform-tests/wpt/master/css/css-mixins/function-attr.html | sed -n '/attr-tainted, initial/,/template>/p'Repository: LadybirdBrowser/ladybird
Length of output: 162
🏁 Script executed:
sed -n '180,240p' Tests/LibWeb/Text/input/wpt-import/css/css-mixins/function-attr.htmlRepository: LadybirdBrowser/ladybird
Length of output: 1752
Define --g() in the initial template --f() currently calls an undefined --g(), so this subtest can pass for the wrong reason instead of exercising the --x: initial tainting behavior.
🤖 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/wpt-import/css/css-mixins/function-attr.html` around
lines 195 - 218, Define the custom function --g() inside the “Returned url() is
attr-tainted, initial” template, matching the helper definition used by the
preceding test so --f() resolves var(--x) through --g(). Keep the existing --x:
initial setup and expected result unchanged.
Calme1709
force-pushed
the
dashed-function-evaluation
branch
from
July 17, 2026 01:39
d40d3b6 to
5bcdf2d
Compare
Member
Author
|
Changes:
The remaining CodeRabbit comments are irrelevant comments on imported WPT tests or out of scope for this PR (potential performance improvements, support for |
Calme1709
force-pushed
the
dashed-function-evaluation
branch
from
July 17, 2026 01:49
5bcdf2d to
fa80daa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/CustomPropertyRegistration.cpp`:
- Around line 84-88: Update the inherited custom-property rewrap in the
computed-parent handling to preserve the original unresolved attr-taint metadata
instead of passing an empty flag set to UnresolvedStyleValue::create. Keep the
guaranteed-invalid path unchanged, and thread or reuse the existing unresolved
taint so inherited attr()-derived values retain their taint through URL-valued
properties.
🪄 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: b6c64426-7176-4a08-b8fd-a164a7a398bf
📒 Files selected for processing (12)
Libraries/LibWeb/CSS/ContainerQuery.cppLibraries/LibWeb/CSS/CustomPropertyRegistration.cppLibraries/LibWeb/CSS/CustomPropertyRegistration.hLibraries/LibWeb/CSS/HypotheticalElement.cppLibraries/LibWeb/CSS/HypotheticalElement.hLibraries/LibWeb/CSS/Parser/ArbitrarySubstitutionFunctions.cppLibraries/LibWeb/CSS/StyleComputer.cppLibraries/LibWeb/CSS/StyleSheetInvalidation.cppTests/LibWeb/Text/expected/wpt-import/css/css-mixins/dashed-function-cycles.txtTests/LibWeb/Text/expected/wpt-import/css/css-mixins/dashed-function-eval.txtTests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-invalidation.txtTests/LibWeb/Text/expected/wpt-import/css/css-variables/variable-css-wide-keywords.txt
🚧 Files skipped from review as they are similar to previous changes (10)
- Tests/LibWeb/Text/expected/wpt-import/css/css-variables/variable-css-wide-keywords.txt
- Tests/LibWeb/Text/expected/wpt-import/css/css-mixins/dashed-function-cycles.txt
- Libraries/LibWeb/CSS/CustomPropertyRegistration.h
- Tests/LibWeb/Text/expected/wpt-import/css/css-mixins/function-invalidation.txt
- Libraries/LibWeb/CSS/StyleSheetInvalidation.cpp
- Libraries/LibWeb/CSS/HypotheticalElement.cpp
- Libraries/LibWeb/CSS/HypotheticalElement.h
- Libraries/LibWeb/CSS/ContainerQuery.cpp
- Libraries/LibWeb/CSS/StyleComputer.cpp
- Libraries/LibWeb/CSS/Parser/ArbitrarySubstitutionFunctions.cpp
Calme1709
force-pushed
the
dashed-function-evaluation
branch
from
July 17, 2026 03:59
fa80daa to
77f70fc
Compare
Done in a standalone commit to track progress of dashed function implementation in the following commits
The APIs this will be used with expect a Utf16FlyString so this avoids unnecessary conversions.
Previously we made sure these parsed and then discarded them - we will need to use them in their parsed form later when evaluating the custom function so let's keep them around to avoid parsing twice.
Previously we always stored the values of these descriptors as `UnresolvedStyleValue` - this is different to normal custom properties where CSS-wide keywords (e.g. `initial`, `inherit`) are stored as `KeywordStyleValue`. Having CSS-wide keywords stored as `KeywordStyleValue` makes them easier to deal with when evaluating custom functions in a later commit.
Previously we assumed that these were always strings but the spec requires us to use the custom function itself (i.e. a `GC::Ref`) in "property" and "function" rather than just the name (in order to distinguish between functions of the same name in different tree scopes)
Storing the computed style on the parser only to read it back two function calls later is a bit awkward so let's just pass it directly.
Deduplicate this between StyleComputer.cpp and ContainerQuery.cpp. We know that ancestors' custom property values are already computed (since style computation occurs in tree order) so we don't need to recompute them.
Custom function evaluation requires us to use a scoped set of registrations rather than the ones defined on the document.
We will need to add some shared functionality in a future commit. This revealed a couple of false positives that we had around handling fallback `revert` values.
This is how we evaluate statements inside custom functions. We also store a function frame's custom property registry as part of the hypothetical element since they share a scope.
Parent value absorption requires that the parent values are already in their computed forms (otherwise they will be computed against the wrong element) - this isn't the case for custom function evaluation.
Custom function evaluation requires us to support computing a custom property with the name `result` despite it normally being invalid as a custom property name.
This is derived from the custom property registrations stored against the hypothetical element rather than the document.
Proper replacement will come in a later commit - for now we just evaluate to the guaranteed invalid value. The remaining failing tests in `dashed-function-parsing.html` are due to us not implementing free-form production parsing rules for `<declaration-value>`.
There are a couple of places where WPT/Chrome's implementation differ from the spec - in those cases we match the behavior of the former (with the customary AD-HOC comments). The function evaluation implementation itself is mostly complete here but as is demonstrated by the still failing WPT tests there are still a few kinks to figure out with custom property computation more broadly, these will be addressed in later commits/PRs.
Calme1709
force-pushed
the
dashed-function-evaluation
branch
from
July 18, 2026 23:20
77f70fc to
7d7413e
Compare
Member
Author
|
Rebased to resolve stealth conflicts |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Libraries/LibWeb/CSS/Parser/ArbitrarySubstitutionFunctions.cpp (1)
440-465: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse hypothetical-aware inheritance for
inherit().Line 440 now accepts hypothetical elements, but Lines 459-463 return the parent’s potentially unresolved stored value directly. During custom-function evaluation this can re-evaluate
var()or dashed functions in the wrong hypothetical context.Route this through
inherited_custom_property_value(), which already handles recomputation for hypothetical parents.Proposed fix
- if (auto element_to_inherit_style_from = element.element_to_inherit_style_from(); element_to_inherit_style_from.has_value()) { - if (auto const& inherited_value = element_to_inherit_style_from->get_custom_property(name_token.token().ident())) { - auto inherited_value_tokens = inherited_value->tokenize(); - if (!contains_guaranteed_invalid_value(inherited_value_tokens)) - return inherited_value_tokens; - } - } + auto const& name = name_token.token().ident(); + auto inherited_value = inherited_custom_property_value( + element.custom_property_registration(name), + element, + name, + replacement_context.computed_style_for_custom_property_resolution, + guarded_contexts); + auto inherited_value_tokens = inherited_value->tokenize(); + if (!contains_guaranteed_invalid_value(inherited_value_tokens)) + return inherited_value_tokens;🤖 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 `@Libraries/LibWeb/CSS/Parser/ArbitrarySubstitutionFunctions.cpp` around lines 440 - 465, Update replace_an_inherit_function to obtain the inherited custom property through inherited_custom_property_value() instead of directly returning element_to_inherit_style_from()->get_custom_property(...). Preserve the existing custom-property-name validation and guaranteed-invalid check, while using the hypothetical-aware recomputed value before tokenizing and returning it.
🤖 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.
Outside diff comments:
In `@Libraries/LibWeb/CSS/Parser/ArbitrarySubstitutionFunctions.cpp`:
- Around line 440-465: Update replace_an_inherit_function to obtain the
inherited custom property through inherited_custom_property_value() instead of
directly returning element_to_inherit_style_from()->get_custom_property(...).
Preserve the existing custom-property-name validation and guaranteed-invalid
check, while using the hypothetical-aware recomputed value before tokenizing and
returning it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6fefaf54-4540-44ed-8bdd-94a287c7f797
📒 Files selected for processing (40)
Documentation/CSSGeneratedFiles.mdLibraries/LibWeb/CMakeLists.txtLibraries/LibWeb/CSS/BooleanExpression.hLibraries/LibWeb/CSS/CSSFunctionDeclarations.hLibraries/LibWeb/CSS/CSSFunctionRule.cppLibraries/LibWeb/CSS/CSSFunctionRule.hLibraries/LibWeb/CSS/CSSPropertyRule.cppLibraries/LibWeb/CSS/CSSPropertyRule.hLibraries/LibWeb/CSS/CSSStyleProperties.cppLibraries/LibWeb/CSS/CSSStyleSheet.cppLibraries/LibWeb/CSS/CSSStyleSheet.hLibraries/LibWeb/CSS/ContainerQuery.cppLibraries/LibWeb/CSS/CustomPropertyData.cppLibraries/LibWeb/CSS/CustomPropertyData.hLibraries/LibWeb/CSS/CustomPropertyRegistration.cppLibraries/LibWeb/CSS/CustomPropertyRegistration.hLibraries/LibWeb/CSS/Descriptors.jsonLibraries/LibWeb/CSS/HypotheticalElement.cppLibraries/LibWeb/CSS/HypotheticalElement.hLibraries/LibWeb/CSS/Parser/ArbitrarySubstitutionFunctions.cppLibraries/LibWeb/CSS/Parser/ArbitrarySubstitutionFunctions.hLibraries/LibWeb/CSS/Parser/DescriptorParsing.cppLibraries/LibWeb/CSS/Parser/Parser.cppLibraries/LibWeb/CSS/Parser/Parser.hLibraries/LibWeb/CSS/Parser/RuleParsing.cppLibraries/LibWeb/CSS/Parser/Types.hLibraries/LibWeb/CSS/Parser/ValueParsing.cppLibraries/LibWeb/CSS/PropertyNameAndID.hLibraries/LibWeb/CSS/StyleComputer.cppLibraries/LibWeb/CSS/StyleComputer.hLibraries/LibWeb/CSS/StylePropertyMapReadOnly.cppLibraries/LibWeb/CSS/StyleScope.cppLibraries/LibWeb/CSS/StyleScope.hLibraries/LibWeb/CSS/StyleSheetInvalidation.cppLibraries/LibWeb/CSS/StyleValues/UnresolvedStyleValue.cppLibraries/LibWeb/CSS/StyleValues/UnresolvedStyleValue.hLibraries/LibWeb/DOM/Document.cppLibraries/LibWeb/DOM/Document.hLibraries/LibWeb/Forward.hLibraries/LibWeb/Rust/src/style_value.rs
💤 Files with no reviewable changes (1)
- Libraries/LibWeb/DOM/Document.h
🚧 Files skipped from review as they are similar to previous changes (28)
- Libraries/LibWeb/CSS/Parser/Types.h
- Libraries/LibWeb/CSS/CSSStyleProperties.cpp
- Libraries/LibWeb/CSS/PropertyNameAndID.h
- Libraries/LibWeb/CSS/CSSStyleSheet.h
- Libraries/LibWeb/CSS/CSSPropertyRule.cpp
- Libraries/LibWeb/CMakeLists.txt
- Libraries/LibWeb/CSS/BooleanExpression.h
- Libraries/LibWeb/CSS/StyleSheetInvalidation.cpp
- Libraries/LibWeb/CSS/Parser/Parser.cpp
- Libraries/LibWeb/CSS/CSSPropertyRule.h
- Libraries/LibWeb/CSS/StylePropertyMapReadOnly.cpp
- Libraries/LibWeb/Forward.h
- Libraries/LibWeb/CSS/CustomPropertyData.h
- Libraries/LibWeb/CSS/Parser/DescriptorParsing.cpp
- Libraries/LibWeb/CSS/StyleScope.cpp
- Libraries/LibWeb/CSS/HypotheticalElement.h
- Libraries/LibWeb/CSS/StyleScope.h
- Libraries/LibWeb/CSS/CSSStyleSheet.cpp
- Libraries/LibWeb/CSS/Descriptors.json
- Libraries/LibWeb/CSS/Parser/ArbitrarySubstitutionFunctions.h
- Libraries/LibWeb/CSS/Parser/RuleParsing.cpp
- Libraries/LibWeb/CSS/StyleComputer.h
- Libraries/LibWeb/DOM/Document.cpp
- Libraries/LibWeb/CSS/Parser/Parser.h
- Libraries/LibWeb/CSS/CSSFunctionRule.cpp
- Libraries/LibWeb/CSS/Parser/ValueParsing.cpp
- Libraries/LibWeb/CSS/ContainerQuery.cpp
- Libraries/LibWeb/CSS/StyleComputer.cpp
Inherting a value during custom function evaluation has a couple of
differences from normal style computation:
1. Inherited values may not be in their computed form.
2. The computed form of a value for this element may not be the same
as the parent element (due to differing custom property
registrations, whereas for normal style computation custom property
registrations are global.
To handle this we now:
1. Recompute inherited values if we aren't sure they are already in
their computed form.
2. Pass resolved keyword values through style computation to be sure
they're in the correct computed form.
The second of these also means that we now correctly compute unresolved
CSS-wide keywords on `result` as IACVT.
We also handle `inherit()` correctly for registered values on element's
without ancestors (i.e. falling back to the initial value rather than
the GIV).
The newly passed tests around `revert` handling are false positives.
Calme1709
force-pushed
the
dashed-function-evaluation
branch
from
July 19, 2026 02:45
7d7413e to
be65e25
Compare
Member
Author
|
Changes:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Most of the work here is around making our custom property computation machinery compatible with how the spec implements custom function evaluation.
There is still work to be done here fixing up the last few kinks but that is left for later PRs.
See individual commits for details.