Skip to content

Conversation

@CGQAQ
Copy link
Contributor

@CGQAQ CGQAQ commented Jan 2, 2026

Implements querySelector/querySelectorAll with a fallback to the Blink engine when enabled.

Changes:

  • Add fallback logic in DOM nodes/document/element
  • Add unit tests for query selector behavior

Tests:

  • node scripts/run_bridge_unit_test.js (recommended)

Summary by CodeRabbit

  • New Features

    • Extended CSS selector query capabilities to retrieve all matching elements rather than just single results. The implementation now fully supports pseudo-classes, combinators, scope selectors, and provides comprehensive error handling for invalid selector syntax. Results maintain proper ordering and deduplication.
  • Tests

    • Added comprehensive test coverage for selector functionality including multi-element matching scenarios, complex selector patterns, error cases, and result ordering verification.

✏️ Tip: You can customize this high-level summary in your review settings.

@vercel
Copy link

vercel bot commented Jan 2, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
use-case Ready Ready Preview, Comment Jan 2, 2026 9:29pm

@coderabbitai
Copy link

coderabbitai bot commented Jan 2, 2026

📝 Walkthrough

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 3.70% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: implementing querySelector/querySelectorAll with fallback to Blink engine when enabled, which matches the core functionality added across ContainerNode, Document, and Element classes.
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 36edfcc and e3a66af.

📒 Files selected for processing (5)
  • bridge/core/dom/container_node.cc
  • bridge/core/dom/container_node.h
  • bridge/core/dom/document.cc
  • bridge/core/dom/document_test.cc
  • bridge/core/dom/element.cc
🚧 Files skipped from review as they are similar to previous changes (3)
  • bridge/core/dom/container_node.h
  • bridge/core/dom/document_test.cc
  • bridge/core/dom/element.cc
🧰 Additional context used
📓 Path-based instructions (5)
bridge/**/*.{cc,cpp,h,hpp}

📄 CodeRabbit inference engine (bridge/CLAUDE.md)

bridge/**/*.{cc,cpp,h,hpp}: C++ code should follow Chromium style (.clang-format) with C++17 standard, 120 character column limit, and 2-space indentation
Use WEBF_EXPORT_C macro for exporting C functions to Dart FFI
In FFI contexts, use Dart_Handle instead of Handle for type compatibility
For C++ FFI function naming: use GetObjectPropertiesFromDart for C++ exports, NativeGetObjectPropertiesFunc for Dart typedefs, and GetObjectPropertiesFunc for Dart functions
Lambda signatures in C++ must match expected function signatures to avoid FFI type mismatches
Choose power-of-2 buffer sizes (512, 1024, 2048) for ring buffer implementation, with smaller sizes for DEBUG_BUILD, MOBILE constraints, and larger sizes for DESKTOP performance

Files:

  • bridge/core/dom/document.cc
  • bridge/core/dom/container_node.cc
bridge/**/*.{cc,cpp}

📄 CodeRabbit inference engine (bridge/CLAUDE.md)

bridge/**/*.{cc,cpp}: For async FFI operations, use Dart_NewPersistentHandle_DL to keep Dart objects alive, convert back with Dart_HandleFromPersistent_DL before use, and always call Dart_DeletePersistentHandle_DL after the async operation completes
For string handling in FFI, copy strings that might be freed using std::string(const_char_ptr), and use toNativeUtf8() with proper memory deallocation
For async callbacks with potential errors, always provide error path in callbacks (e.g., callback(object, nullptr)), handle cancellation cases in async operations, and propagate errors through callback parameters rather than exceptions
For thread-safe error reporting in FFI, copy error messages before crossing thread boundaries using std::string to ensure string lifetime, and consider error callbacks separate from result callbacks
Avoid PostToJsSync when threads may interdepend to prevent synchronous deadlocks in FFI communication
Ensure callback functions aren't invoked after context destruction to prevent use-after-free errors in FFI async operations
Implement ring buffer overflow handling with metrics and alerts to monitor command buffer capacity
Process multiple UI commands per frame in a loop with a MAX_COMMANDS_PER_FRAME limit to balance responsiveness and performance
Pin threads to cores for optimal cache usage in ring buffer operations by setting CPU affinity for UI threads
Use PostToJs for executing operations on the JS thread from other threads, PostToDart for returning results to Dart isolate, and avoid PostToJsSync to prevent deadlocks

Files:

  • bridge/core/dom/document.cc
  • bridge/core/dom/container_node.cc
bridge/**/*.{cc,h}

📄 CodeRabbit inference engine (AGENTS.md)

C++ code in bridge module must use C++17 standard with 2-space indentation, 120 column limit, and follow Chromium style guide as defined in .clang-format

bridge/**/*.{cc,h}: Use RAII patterns in C++ where possible for resource management
Use PostToJs for executing operations on the JS thread in FFI
Use PostToDart for returning results to Dart isolate
Avoid PostToJsSync synchronous execution when possible

C++ bridge code must use 2-space indent, 120 column line limit, and follow Chromium style via .clang-format

Files:

  • bridge/core/dom/document.cc
  • bridge/core/dom/container_node.cc
{bridge/**/*.{cc,h},webf/**/*.dart}

📄 CodeRabbit inference engine (CLAUDE.md)

Document memory ownership clearly in FFI implementations

Files:

  • bridge/core/dom/document.cc
  • bridge/core/dom/container_node.cc
bridge/core/**

📄 CodeRabbit inference engine (bridge/AGENTS.md)

Core bridge code should be located in bridge/core/ directory

Files:

  • bridge/core/dom/document.cc
  • bridge/core/dom/container_node.cc
🧬 Code graph analysis (2)
bridge/core/dom/document.cc (3)
bridge/core/css/parser/css_parser_context.cc (2)
  • GetExecutingContext (42-44)
  • GetExecutingContext (42-42)
bridge/core/dom/container_node.cc (8)
  • QuerySelector (104-116)
  • QuerySelector (104-104)
  • QuerySelector (118-120)
  • QuerySelector (118-118)
  • QuerySelectorAll (122-135)
  • QuerySelectorAll (122-122)
  • QuerySelectorAll (137-139)
  • QuerySelectorAll (137-137)
bridge/core/api/document.cc (2)
  • QuerySelector (178-194)
  • QuerySelector (178-181)
bridge/core/dom/container_node.cc (1)
bridge/core/executing_context.cc (2)
  • ctx (492-495)
  • ctx (492-492)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: build_windows_bridge (Debug)
  • GitHub Check: Analyze (rust)
  • GitHub Check: Analyze (c-cpp)
🔇 Additional comments (8)
bridge/core/dom/document.cc (2)

200-212: LGTM!

The Blink-enabled fallback for querySelector is correctly implemented with proper null-checking of GetExecutingContext() before accessing isBlinkEnabled(). The delegation to ContainerNode::QuerySelector aligns with the PR objectives.


214-226: LGTM!

The querySelectorAll implementation mirrors the querySelector pattern correctly, with consistent null-checking and delegation to ContainerNode::QuerySelectorAll when Blink is enabled.

bridge/core/dom/container_node.cc (6)

30-36: LGTM!

The new includes are appropriately grouped and necessary for the CSS selector parsing and matching functionality being added.


75-88: LGTM!

The MatchesAnySelectorInList helper correctly uses SelectorChecker with kQueryingRules mode and properly sets the scope for selector matching. The iteration through the selector list is implemented correctly.


104-116: LGTM!

The QuerySelector implementation correctly parses the selector, handles exceptions, and returns the first matching descendant element. The use of ElementTraversal::DescendantsOf is appropriate for DOM tree traversal.


122-139: LGTM!

The QuerySelectorAll implementation correctly collects all matching descendants and properly handles the empty result case on parse failure. The overload delegation pattern is consistent with QuerySelector.


628-630: LGTM!

The simplified querySelector wrapper that directly delegates to QuerySelector is clean and consistent with the new architecture.


55-73: The code correctly handles selector data ownership. CSSSelectorList::AdoptSelectorVector uses std::uninitialized_move to move selectors from the input span into the CSSSelectorList object's own allocated memory, which is allocated alongside the object itself via MakeSharedPtrWithAdditionalBytes. The arena vector going out of scope is safe because the selector data has been moved (not copied or referenced) into the returned selector_list, which takes full ownership of the data.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

…ink engine when enabled

- Added Blink-backed implementation for `querySelector` and `querySelectorAll` in `Document` and `ContainerNode` when the Blink engine is enabled.
- Updated associated tests in `document_test.cc` to verify correct behavior.
- Refactored CSS selector parsing and matching logic to ensure accurate query resolution.
@andycall andycall force-pushed the feat/query-selector branch from 36edfcc to e3a66af Compare January 2, 2026 21:28
@andycall andycall merged commit c68c312 into main Jan 2, 2026
32 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants