LibWeb: Move selector matching to Rust#10731
Merged
Merged
Conversation
Cover public selector APIs, combinator traversal, logical and positional selectors, relative selector shapes, XML namespace handling, and shadow DOM matching before replacing the selector engine.
Represent selectors as immutable Rust compounds and simple selectors. Precompute fast-match eligibility, target pseudo-elements, stable cache identities, and sibling invalidation distances for the matcher.
Give every parsed CSS selector an immutable Rust counterpart that owns its strings and retains nested selector arguments through shared references. This prepares selector matching to cross the FFI boundary without borrowing parser storage or recompiling selectors per match. Add an explicit C ABI for transferring the complete selector grammar, including namespaces, functional pseudo-classes, and pseudo-elements. Keep the existing C++ matcher active until the Rust matcher is complete.
Implement selector matching control flow over a generic borrowed DOM interface. Cover fast matching, combinator backtracking, functional and structural pseudo-classes, relative :has() matching, pseudo-element transitions, cache hooks, and invalidation metadata hooks. Keep DOM semantics behind a monomorphized trait so the production bridge can observe live C++ nodes without snapshots or dynamic dispatch. Add unit coverage for backtracking, relative selectors, and filtered child indices.
Connect the Rust selector matcher to live C++ DOM nodes through a synchronous semantic FFI. Borrow node, scope, shadow-root, stylesheet, and matching-context pointers for one call. Never retain or copy the DOM tree. Evaluate Rust alongside the existing matcher and assert that both results agree while leaving C++ responsible for the returned result. Suppress side effects in the comparison context so invalidation metadata and :has() counters remain unchanged. Key shared :has() cache entries by the stable Rust selector ID in preparation for the final cutover.
Retain the originating C++ selector as a borrowed identity alongside its Rust representation so the existing subtree reject filter can be reused without recomputing or translating its hashes. Port the specialized child-tag and descendant tag-and-class scans to the Rust traversal layer. Keep metadata collection on the general path and preserve successful descendant cache propagation.
Make Rust responsible for selector traversal and the returned match result. Enable its cache, counter, and invalidation metadata side effects so style computation, DOM queries, and invalidation all use one path. Keep C++ limited to semantic DOM queries across the synchronous FFI.
Keep selector traversal, combinator evaluation, pseudo-element transitions, and structural and functional pseudo-classes entirely in Rust. Reduce C++ selector matching to synchronous semantic DOM queries and existing invalidation caches used through FFI. Pass MatchContext directly across the FFI now that comparison mode no longer needs to suppress side effects. Drop the matching parameters, caches, invalidation helpers, and traversal callbacks that only served the deleted C++ engine. Rename the remaining integration surface to SelectorMatching and update all callers. Remove the old C++ matching implementation without a fallback path.
Compute the index delta and divisor in i64 before applying modulo and division. Preserve matching for extreme 32-bit coefficients without overflowing in debug or sanitizer builds. Add unit and end-to-end selector coverage for both i32 boundaries, fixed offsets, and negative steps.
Count matching siblings directly instead of starting from the parent element. Fragment-root children have no parent element, even though their Node parent still exposes element siblings. Reverse sibling counting no longer needs last_element_child, so drop the now-unused trait method and FFI callback. Add forward, reverse, filtered, type-based, and query selector coverage with text and comment nodes mixed into both fragment root types.
A slot whose root is a shadow tree can never be a slotted element, and the old C++ selector engine rejected such match targets before evaluating the ::slotted() argument selector. The Rust engine evaluates the argument first, which shows up as a :has() match invocation when matching ::slotted(:has(...)) against a slot. This test pins the current behavior, including the extra :has() match invocation for the slot target. A subsequent commit restores the old ordering and updates the expectations.
The old C++ selector engine rejected a match target that is itself a slot in a shadow tree before evaluating the ::slotted() argument selector. The Rust port folded that check into the slotted-parent FFI lookup, which runs after the argument match, so matching a ::slotted() rule against a slot element would evaluate the argument selector against the slot first. That is wasted work, and for arguments with side effects such as :has() result caching or selector involvement metadata, it touched slot elements the old engine never touched. Restore the old ordering with a dedicated is_shadow_tree_slot check that runs before the argument match, and drop the now-redundant check from the slotted-parent lookup. This removes the extra :has() match invocation pinned by the test added in the previous commit.
The old C++ ANPlusBPattern::matches special-cased step size 1 and rejected any such selector with a negative offset, so :nth-child(n-2) matched nothing. The Rust selector engine uses the general An+B formula, under which every index has a non-negative n solving n + offset = index, so these selectors match every element. That is what the specification and other engines do, but nothing pinned the behavior. Extend the extreme An+B test with step-size-1 negative offset cases for :nth-child, :nth-last-child, and :nth-of-type.
The compiled Rust selector stored id, class, tag, attribute, and state names as raw UTF-16 code unit slices, so the innermost matching callbacks compared string content or re-interned identifiers on every test. The removed C++ engine compared interned Utf16FlyStrings, where equality is a pointer comparison. Store a pointer to the C++ simple selector in the compiled Rust representation. The C++ selector outlives its compiled twin, which it owns, so matching callbacks can use its interned strings directly: - id matching is Utf16FlyString equality again - class matching uses an overload that compares interned strings in the case-sensitive path - tag names compare local_name against the interned selector name in the exact-match paths - attribute name lookups compare interned local names - :state() looks up the already-interned custom-state identifier This also lets the universal, tag, and attribute callbacks use the original qualified name instead of marshaling namespace and name strings through the FFI, and drops per-call attribute struct marshaling.
Tie borrowed DOM nodes and matching contexts to each FFI call. Tag scope nodes separately so they cannot reach element-only callbacks. Retain C++ selector identities as typed pointers, and copy transient FFI slices into owned storage. Document each unsafe boundary and its lifetime requirements. Use Rc for the single-threaded selector graph. Mirror the selector involvement flags in FfiDom as well. Hot matching loops can then skip metadata callbacks that are no-ops and avoid an FFI round trip when saving the per-:has()-argument state.
Generate the Rust pseudo-class and pseudo-element enums from the same JSON sources as their C++ counterparts. This prevents positional drift that count assertions could not detect. Generate the selector ABI header from the Rust declarations as well. This makes cbindgen the single source of truth for layouts, enum values, and function signatures on both sides of the boundary. Teach the Rust crate build helper to track and synchronize multiple generated headers.
Explain the compiled selector representation, its ownership boundaries, and match-state invariants. Include the specification algorithm for right-to-left matching beside the general recursive matcher. Quote the relevant Selectors and CSS Scoping requirements beside their matching paths. Document relative traversal, shadow transitions, cache propagation, and matcher optimizations.
Represent retained C++ selector identities with a private nullable pointer type and keep raw conversion at the FFI edges. Tie DOM handles to a matching-call token and prevent them from escaping through a higher-ranked matching closure. Borrow transient FFI record arrays while compiling selectors. Continue copying strings and other data retained by the compiled representation.
Own the mutable DOM borrow while matching a :has() argument and restore the previous state when the guard is dropped. Cover restoration during panic unwinding in the Rust selector tests.
📝 WalkthroughWalkthroughThe change replaces the C++ selector engine with a Rust-backed CSS selector compiler and matcher, adds generated multi-header FFI support, wires LibWeb call sites to the new API, and adds coverage for selector, shadow-DOM, invalidation, and ChangesRust CSS Selector Matching
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant StyleComputer
participant SelectorMatching
participant RustSelector
participant DOMCallbacks
StyleComputer->>SelectorMatching: matches(selector, target, context)
SelectorMatching->>RustSelector: rust_selector_matches(...)
RustSelector->>DOMCallbacks: evaluate selectors and traverse DOM
DOMCallbacks-->>RustSelector: match result and metadata
RustSelector-->>SelectorMatching: return boolean result
SelectorMatching-->>StyleComputer: return match result
🚥 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: 7
🤖 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/SelectorMatching.cpp`:
- Around line 1122-1125: Update selector_ffi_is_document_root to determine root
status from document position by returning Element::is_document_element() for
the element obtained via ffi_element(element), rather than checking for
HTMLHtmlElement, so detached HTML elements do not match and valid XML/SVG
document roots do.
- Around line 952-963: Update the Named namespace branch in the attribute
matching loop to treat an empty namespace URI as the null namespace, matching
the contract used by the element path. Adjust the `attribute->namespace_uri()`
condition so unnamespaced attributes can match when `selector_namespace`
resolves to an empty value, while preserving matching for non-empty namespace
URIs and the existing `attribute_matches` call.
- Around line 853-856: Update the ContainsWord branch in the attribute selector
matching logic to tokenize element_value using all CSS whitespace
characters—space, tab, newline, carriage return, and form feed—instead of only
U+0020. Preserve the existing empty selector_value check and values_equal
comparison behavior.
In `@Libraries/LibWeb/Rust/build.rs`:
- Around line 125-129: The cbindgen build path currently ignores
ParseSyntaxError and can succeed with a stale header. Update the error handling
in builder.generate() to fail the build for parse errors as well as other
cbindgen errors, rather than silently returning success.
In `@Libraries/LibWeb/Rust/src/selector_engine.rs`:
- Line 704: Update the Combinator::Column branch in the selector matching logic
to avoid invoking unimplemented!, which can terminate processing for
page-controlled selectors. Implement column combinator matching if supported;
otherwise return the existing no-match result through the surrounding matching
flow while preserving behavior for all other combinators.
- Around line 778-786: Update the SimpleSelector::PseudoClass matching logic to
handle PseudoClassType::HostContext explicitly rather than rejecting it as
featureless-host matching. Evaluate its argument selector list against the host
and relevant ancestors using the selector-matching machinery, and preserve the
existing Host, Has, Is, Where, and Scope behavior for other pseudo-classes.
Ensure the generic pseudo-class callback is not used for HostContext without its
required selector arguments.
- Around line 1257-1279: Update compound_may_match_host_for_part_scope to
recognize :host nested through both PseudoClassType::Is and
PseudoClassType::Where, using the same recursive compound-selector traversal for
either wrapper. Preserve direct :host detection and return true whenever either
wrapper contains a nested host selector.
🪄 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: 5880675a-a76a-4e84-a527-83c415773c79
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
Libraries/LibWeb/CMakeLists.txtLibraries/LibWeb/CSS/Selector.cppLibraries/LibWeb/CSS/Selector.hLibraries/LibWeb/CSS/SelectorEngine.cppLibraries/LibWeb/CSS/SelectorMatching.cppLibraries/LibWeb/CSS/SelectorMatching.hLibraries/LibWeb/CSS/SelectorRustBridge.cppLibraries/LibWeb/CSS/SelectorRustBridge.hLibraries/LibWeb/CSS/StyleComputer.cppLibraries/LibWeb/CSS/StyleComputer.hLibraries/LibWeb/CSS/StyleSheetInvalidation.cppLibraries/LibWeb/DOM/Document.cppLibraries/LibWeb/DOM/Element.cppLibraries/LibWeb/DOM/Element.hLibraries/LibWeb/DOM/ParentNode.cppLibraries/LibWeb/DOM/SelectorQuery.cppLibraries/LibWeb/Rust/Cargo.tomlLibraries/LibWeb/Rust/build.rsLibraries/LibWeb/Rust/src/css_tokenizer.rsLibraries/LibWeb/Rust/src/lib.rsLibraries/LibWeb/Rust/src/selector_engine.rsMeta/CMake/rust_crate.cmakeMeta/CMake/sync_rust_ffi_header.cmakeTests/LibWeb/Text/expected/css/nth-child-extreme-an-plus-b.txtTests/LibWeb/Text/expected/css/nth-child-non-element-parent.txtTests/LibWeb/Text/expected/css/selector-engine-characterization.txtTests/LibWeb/Text/expected/css/style-invalidation/slotted-argument-slot-early-out.txtTests/LibWeb/Text/input/css/nth-child-extreme-an-plus-b.htmlTests/LibWeb/Text/input/css/nth-child-non-element-parent.htmlTests/LibWeb/Text/input/css/selector-engine-characterization.htmlTests/LibWeb/Text/input/css/style-invalidation/slotted-argument-slot-early-out.html
Comment on lines
+853
to
+856
| case CSS::Selector::SimpleSelector::Attribute::MatchType::ContainsWord: | ||
| if (selector_value.is_empty()) | ||
| return false; | ||
| return element_value.split_view(' ', SplitBehavior::Nothing).contains([&](auto value) { return values_equal(value, selector_value); }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Tokenize ~= attribute values using all CSS whitespace.
Line 856 only splits on U+0020, so valid values separated by tabs, newlines, carriage returns, or form feeds fail to match.
Proposed fix
- case CSS::Selector::SimpleSelector::Attribute::MatchType::ContainsWord:
+ case CSS::Selector::SimpleSelector::Attribute::MatchType::ContainsWord: {
if (selector_value.is_empty())
return false;
- return element_value.split_view(' ', SplitBehavior::Nothing).contains([&](auto value) { return values_equal(value, selector_value); });
+ size_t start = 0;
+ for (size_t i = 0; i <= element_value.length_in_code_units(); ++i) {
+ if (i != element_value.length_in_code_units()
+ && !Infra::is_ascii_whitespace(element_value.code_unit_at(i))) {
+ continue;
+ }
+ if (i > start && values_equal(element_value.substring_view(start, i - start), selector_value))
+ return true;
+ start = i + 1;
+ }
+ return false;
+ }📝 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
| case CSS::Selector::SimpleSelector::Attribute::MatchType::ContainsWord: | |
| if (selector_value.is_empty()) | |
| return false; | |
| return element_value.split_view(' ', SplitBehavior::Nothing).contains([&](auto value) { return values_equal(value, selector_value); }); | |
| case CSS::Selector::SimpleSelector::Attribute::MatchType::ContainsWord: { | |
| if (selector_value.is_empty()) | |
| return false; | |
| size_t start = 0; | |
| for (size_t i = 0; i <= element_value.length_in_code_units(); ++i) { | |
| if (i != element_value.length_in_code_units() | |
| && !Infra::is_ascii_whitespace(element_value.code_unit_at(i))) { | |
| continue; | |
| } | |
| if (i > start && values_equal(element_value.substring_view(start, i - start), selector_value)) | |
| return true; | |
| start = i + 1; | |
| } | |
| return false; | |
| } |
🤖 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/SelectorMatching.cpp` around lines 853 - 856, Update the
ContainsWord branch in the attribute selector matching logic to tokenize
element_value using all CSS whitespace characters—space, tab, newline, carriage
return, and form feed—instead of only U+0020. Preserve the existing empty
selector_value check and values_equal comparison behavior.
Comment on lines
+952
to
+963
| case CSS::Selector::SimpleSelector::QualifiedName::NamespaceType::Named: { | ||
| if (!match_context.style_sheet_for_rule) | ||
| return false; | ||
| auto selector_namespace = match_context.style_sheet_for_rule->namespace_uri(qualified_name.namespace_); | ||
| if (!selector_namespace.has_value()) | ||
| return false; | ||
| for (u32 i = 0; i < target.attributes()->length(); ++i) { | ||
| auto const* attribute = target.attributes()->item(i); | ||
| if (attribute->namespace_uri().has_value() | ||
| && *selector_namespace == attribute->namespace_uri()->view() | ||
| && attribute->local_name() == attribute_name) | ||
| return attribute_matches(*attribute); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Treat an empty named namespace as the null namespace for attributes.
The element path handles this contract at Lines 744–748, but this branch requires namespace_uri().has_value(). Consequently, a prefix mapped to an empty namespace cannot match an unnamespaced attribute.
Proposed fix
for (u32 i = 0; i < target.attributes()->length(); ++i) {
auto const* attribute = target.attributes()->item(i);
- if (attribute->namespace_uri().has_value()
- && *selector_namespace == attribute->namespace_uri()->view()
+ bool namespace_matches = selector_namespace->is_empty()
+ ? (!attribute->namespace_uri().has_value() || attribute->namespace_uri()->is_empty())
+ : (attribute->namespace_uri().has_value()
+ && *selector_namespace == attribute->namespace_uri()->view());
+ if (namespace_matches
&& attribute->local_name() == attribute_name)
return attribute_matches(*attribute);
}📝 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
| case CSS::Selector::SimpleSelector::QualifiedName::NamespaceType::Named: { | |
| if (!match_context.style_sheet_for_rule) | |
| return false; | |
| auto selector_namespace = match_context.style_sheet_for_rule->namespace_uri(qualified_name.namespace_); | |
| if (!selector_namespace.has_value()) | |
| return false; | |
| for (u32 i = 0; i < target.attributes()->length(); ++i) { | |
| auto const* attribute = target.attributes()->item(i); | |
| if (attribute->namespace_uri().has_value() | |
| && *selector_namespace == attribute->namespace_uri()->view() | |
| && attribute->local_name() == attribute_name) | |
| return attribute_matches(*attribute); | |
| case CSS::Selector::SimpleSelector::QualifiedName::NamespaceType::Named: { | |
| if (!match_context.style_sheet_for_rule) | |
| return false; | |
| auto selector_namespace = match_context.style_sheet_for_rule->namespace_uri(qualified_name.namespace_); | |
| if (!selector_namespace.has_value()) | |
| return false; | |
| for (u32 i = 0; i < target.attributes()->length(); ++i) { | |
| auto const* attribute = target.attributes()->item(i); | |
| bool namespace_matches = selector_namespace->is_empty() | |
| ? (!attribute->namespace_uri().has_value() || attribute->namespace_uri()->is_empty()) | |
| : (attribute->namespace_uri().has_value() | |
| && *selector_namespace == attribute->namespace_uri()->view()); | |
| if (namespace_matches | |
| && attribute->local_name() == attribute_name) | |
| return attribute_matches(*attribute); |
🤖 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/SelectorMatching.cpp` around lines 952 - 963, Update the
Named namespace branch in the attribute matching loop to treat an empty
namespace URI as the null namespace, matching the contract used by the element
path. Adjust the `attribute->namespace_uri()` condition so unnamespaced
attributes can match when `selector_namespace` resolves to an empty value, while
preserving matching for non-empty namespace URIs and the existing
`attribute_matches` call.
Comment on lines
+1122
to
+1125
| extern "C" bool selector_ffi_is_document_root(void const* element) | ||
| { | ||
| return is<HTML::HTMLHtmlElement>(ffi_element(element)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Determine :root from document position, not HTML element type.
This makes every detached <html> match :root while rejecting valid XML/SVG document roots. Use Element::is_document_element().
Proposed fix
extern "C" bool selector_ffi_is_document_root(void const* element)
{
- return is<HTML::HTMLHtmlElement>(ffi_element(element));
+ return ffi_element(element).is_document_element();
}📝 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
| extern "C" bool selector_ffi_is_document_root(void const* element) | |
| { | |
| return is<HTML::HTMLHtmlElement>(ffi_element(element)); | |
| } | |
| extern "C" bool selector_ffi_is_document_root(void const* element) | |
| { | |
| return ffi_element(element).is_document_element(); | |
| } |
🤖 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/SelectorMatching.cpp` around lines 1122 - 1125, Update
selector_ffi_is_document_root to determine root status from document position by
returning Element::is_document_element() for the element obtained via
ffi_element(element), rather than checking for HTMLHtmlElement, so detached HTML
elements do not match and valid XML/SVG document roots do.
Comment on lines
+125
to
+129
| builder.generate().map_or_else( | ||
| |error| match error { | ||
| cbindgen::Error::ParseSyntaxError { .. } => {} | ||
| other => panic!("{other:?}"), | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C6 'ParseSyntaxError|write_to_file' Libraries/LibWeb/Rust/build.rs
rg -n -C6 'ffi_output|sync_ffi_header_commands' Meta/CMake/rust_crate.cmakeRepository: LadybirdBrowser/ladybird
Length of output: 3529
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Libraries/LibWeb/Rust/build.rs (relevant section) ---'
sed -n '1,180p' Libraries/LibWeb/Rust/build.rs
echo
echo '--- Meta/CMake/rust_crate.cmake (sync section) ---'
sed -n '1,140p' Meta/CMake/rust_crate.cmake
echo
echo '--- search for cbindgen / header cleanup / copy commands ---'
rg -n -C3 'cbindgen|copy_if_different|remove_file|ParseSyntaxError|write_to_file|OUT_DIR|ffi_out_dir' Libraries/LibWeb/Rust/build.rs Meta/CMake/rust_crate.cmakeRepository: LadybirdBrowser/ladybird
Length of output: 18278
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' Meta/CMake/sync_rust_ffi_header.cmakeRepository: LadybirdBrowser/ladybird
Length of output: 1812
Fail cbindgen on parse errors Libraries/LibWeb/Rust/build.rs:125-129
sync_rust_ffi_header.cmake copies the header from the latest build-script output, so returning success here can publish a stale OUT_DIR header alongside the new archive.
🤖 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/Rust/build.rs` around lines 125 - 129, The cbindgen build
path currently ignores ParseSyntaxError and can succeed with a stale header.
Update the error handling in builder.generate() to fail the build for parse
errors as well as other cbindgen errors, rather than silently returning success.
Comment on lines
+778
to
+786
| match simple_selector { | ||
| SimpleSelector::PseudoClass(pseudo_class) => { | ||
| !(matches!( | ||
| pseudo_class.pseudo_class, | ||
| PseudoClassType::Host | PseudoClassType::Has | PseudoClassType::Is | PseudoClassType::Where | ||
| ) || pseudo_class.pseudo_class == PseudoClassType::Scope && scope == Some(element)) | ||
| } | ||
| SimpleSelector::Nesting | SimpleSelector::PseudoElement(_) => false, | ||
| _ => true, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Implement :host-context() instead of rejecting it as featureless-host matching.
The guard explicitly omits HostContext, so it returns false before matching. Even if admitted, the generic callback receives no argument selector list, so this functional pseudo-class needs a Rust matching branch that evaluates the host/ancestor context.
Also applies to: 800-876
🤖 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/Rust/src/selector_engine.rs` around lines 778 - 786, Update
the SimpleSelector::PseudoClass matching logic to handle
PseudoClassType::HostContext explicitly rather than rejecting it as
featureless-host matching. Evaluate its argument selector list against the host
and relevant ancestors using the selector-matching machinery, and preserve the
existing Host, Has, Is, Where, and Scope behavior for other pseudo-classes.
Ensure the generic pseudo-class callback is not used for HostContext without its
required selector arguments.
Comment on lines
+1257
to
+1279
| fn compound_may_match_host_for_part_scope(compound_selector: &CompoundSelector) -> bool { | ||
| compound_selector.simple_selectors.iter().any(|simple_selector| { | ||
| let SimpleSelector::PseudoClass(pseudo_class) = simple_selector else { | ||
| return false; | ||
| }; | ||
| if pseudo_class.pseudo_class == PseudoClassType::Host { | ||
| return true; | ||
| } | ||
| pseudo_class.pseudo_class == PseudoClassType::Is | ||
| && pseudo_class.argument_selector_list.iter().any(|selector| { | ||
| selector.compound_selectors.iter().any(|compound| { | ||
| compound.simple_selectors.iter().any(|simple| { | ||
| matches!( | ||
| simple, | ||
| SimpleSelector::PseudoClass(PseudoClassSelector { | ||
| pseudo_class: PseudoClassType::Host, | ||
| .. | ||
| }) | ||
| ) | ||
| }) | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Recognize :where(:host) when selecting the ::part() traversal scope.
:where() and :is() share matching semantics, but this helper only unwraps :is(). Consequently, selectors such as :where(:host)::part(foo) incorrectly disallow the same-shadow-root path. Recurse through both wrappers.
Proposed fix
fn compound_may_match_host_for_part_scope(compound_selector: &CompoundSelector) -> bool {
- compound_selector.simple_selectors.iter().any(|simple_selector| {
+ fn may_match_host(simple_selector: &SimpleSelector) -> bool {
let SimpleSelector::PseudoClass(pseudo_class) = simple_selector else {
return false;
};
if pseudo_class.pseudo_class == PseudoClassType::Host {
return true;
}
- pseudo_class.pseudo_class == PseudoClassType::Is
+ matches!(
+ pseudo_class.pseudo_class,
+ PseudoClassType::Is | PseudoClassType::Where
+ )
&& pseudo_class.argument_selector_list.iter().any(|selector| {
selector.compound_selectors.iter().any(|compound| {
- compound.simple_selectors.iter().any(|simple| {
- matches!(
- simple,
- SimpleSelector::PseudoClass(PseudoClassSelector {
- pseudo_class: PseudoClassType::Host,
- ..
- })
- )
- })
+ compound.simple_selectors.iter().any(may_match_host)
})
})
- })
+ }
+
+ compound_selector.simple_selectors.iter().any(may_match_host)
}📝 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
| fn compound_may_match_host_for_part_scope(compound_selector: &CompoundSelector) -> bool { | |
| compound_selector.simple_selectors.iter().any(|simple_selector| { | |
| let SimpleSelector::PseudoClass(pseudo_class) = simple_selector else { | |
| return false; | |
| }; | |
| if pseudo_class.pseudo_class == PseudoClassType::Host { | |
| return true; | |
| } | |
| pseudo_class.pseudo_class == PseudoClassType::Is | |
| && pseudo_class.argument_selector_list.iter().any(|selector| { | |
| selector.compound_selectors.iter().any(|compound| { | |
| compound.simple_selectors.iter().any(|simple| { | |
| matches!( | |
| simple, | |
| SimpleSelector::PseudoClass(PseudoClassSelector { | |
| pseudo_class: PseudoClassType::Host, | |
| .. | |
| }) | |
| ) | |
| }) | |
| }) | |
| }) | |
| }) | |
| fn compound_may_match_host_for_part_scope(compound_selector: &CompoundSelector) -> bool { | |
| fn may_match_host(simple_selector: &SimpleSelector) -> bool { | |
| let SimpleSelector::PseudoClass(pseudo_class) = simple_selector else { | |
| return false; | |
| }; | |
| if pseudo_class.pseudo_class == PseudoClassType::Host { | |
| return true; | |
| } | |
| matches!( | |
| pseudo_class.pseudo_class, | |
| PseudoClassType::Is | PseudoClassType::Where | |
| ) && pseudo_class.argument_selector_list.iter().any(|selector| { | |
| selector.compound_selectors.iter().any(|compound| { | |
| compound.simple_selectors.iter().any(may_match_host) | |
| }) | |
| }) | |
| } | |
| compound_selector.simple_selectors.iter().any(may_match_host) | |
| } |
🤖 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/Rust/src/selector_engine.rs` around lines 1257 - 1279,
Update compound_may_match_host_for_part_scope to recognize :host nested through
both PseudoClassType::Is and PseudoClassType::Where, using the same recursive
compound-selector traversal for either wrapper. Preserve direct :host detection
and return true whenever either wrapper contains a nested host selector.
Member
Author
|
The review findings here are all pre-existing issues, and fixing them is outside the scope of a Rust port. |
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
1 participant
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.
This moves selector matching and the compiled selector representation from C++ to Rust. Each
CSS::Selectoris compiled once into immutable Rust data, including precomputed fast-match eligibility,:has()cache identity, pseudo-element information, and sibling invalidation distance.The DOM remains in C++. Matching borrows opaque node handles for one synchronous FFI call and accesses DOM state through narrow
SelectorDomcallbacks. Rust owns any selector data retained after compilation, while DOM nodes,MatchContext,:has()caches, and invalidation metadata keep their existing C++ ownership. Call-scoped lifetimes prevent borrowed node handles from escaping into compiled selectors or other retained state.The Rust matcher preserves the existing algorithms and optimizations: right-to-left selector matching, forward candidate discovery for relative selectors inside
:has(), descendant backtracking, specialized:has()scans, ancestor-cache propagation, and invalidation bookkeeping. The migration initially ran both engines and asserted identical results before removing the C++ matcher.cbindgennow generates the selector ABI header from Rust declarations. Rust pseudo-class and pseudo-element enums are generated from the same JSON sources as their C++ counterparts, removing the hand-maintained duplicate ABI definitions. Interned selector strings continue to be compared by C++ identity across the FFI boundary.Coverage added with the migration includes:
:has()fast paths and caching, filtered child indices, invalidation distance, and selector cache IDs;:nth-*under document fragments and shadow roots, and::slotted()argument handling;:has()state during panic unwinding.