⚗️ [RUM-13259] Add shadow DOM support for action selectors #4097
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ccb1ca4218
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage 🔗 Commit SHA: 729735f | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
BeltranBulbarellaDD
left a comment
BeltranBulbarellaDD
left a comment
There was a problem hiding this comment.
Nice! Works perfectly with the html test
Looks good in the explorer UI too
| import { isNodeShadowRoot } from '../browser/htmlDomUtils' | ||
| import { DEFAULT_PROGRAMMATIC_ACTION_NAME_ATTRIBUTE } from './action/actionNameConstants' | ||
|
|
||
| export const SHADOW_DOM_MARKER = '::shadow' |
There was a problem hiding this comment.
💬 suggestion: Could be nice to add a comment explaining the reasoning behind choosing this marker, like this is not a supported CSS selector etc.
|
|
||
| while (currentTarget) { | ||
| const rootNode = currentTarget.getRootNode() as Document | ShadowRoot | ||
| result.unshift({ rootNode, target: currentTarget }) |
There was a problem hiding this comment.
Nit: there's an opportunity to improve computational complexity here a bit.
Every time you call unshift(), every element that's already in the array needs to be copied to a new position to make space for the element you're unshifting. A single call to unshift() is thus O(n), which makes a loop like this O(n^2). A better approach is to use push(), which is amortized O(1), resulting in an amortized O(n) loop, and then reverse() the array, another O(n) operation. By using push() and reverse(), the overall task is O(n) instead of O(n^2).
Given that result is going to be very short in most cases, I wouldn't expect a substantial performance difference from this change, but it's good to develop the habit of using this approach, since the impact can be surprisingly large in cases where the array is larger!
| shadowRoot.appendChild(button) | ||
|
|
||
| const selector = getSelectorFromElement(button, undefined) | ||
| expect(selector).toBe(`BODY>DIV:nth-of-type(1)${SHADOW_DOM_MARKER}#foo`) |
There was a problem hiding this comment.
Just for clarity, is this the result we intended? The #foo inside and outside the shadow subtree are two different #foos, and that's perfectly fine; I'd actually have expected the result here to be this:
BODY>DIV#foo${SHADOW_DOM_MARKER}#fooUsing the id at both leaves results in a less fragile selector, so it's a better choice, unless there's some issue that's leading us to avoid it.
| * Note: This is NOT a valid CSS selector, it's an internal marker that requires custom | ||
| * parsing logic. | ||
| */ | ||
| export const SHADOW_DOM_MARKER = '::shadow' |
There was a problem hiding this comment.
Sorry for not realizing this earlier, but I think this should actually be ::shadow>; otherwise the selectors you're generating would like like e.g. ::shadowBUTTON, when what we really want is ::shadow>BUTTON.
Motivation
Second part of shadow DOM support for actions. This PR adds selector generation for elements inside shadow DOM, using a
/shadow/marker to indicate shadow boundaries.Follows up on the first PR that added action name support.
Changes
These two pictures are coming from the same click with and without the parameter enabled. The selector is correct and more precise that's why the width and height are smaller.
Before
After
Selector query
Test instructions
Checklist