[EventTiming] Enhance targetSelector with context-aware traversal [chromium/src : main]
Mohamed Youns (Gerrit)
Mohamed Youns added 1 comment![]( "Open in Gerrit")
I've implemented the traversal algorithm (checking ancestors for ID/Name/Alt/Src) as discussed.
I wanted to highlight two specific open questions:
1. **Feature Flag for New Attributes:** Should I put the new attribute checks (e.g., `name`, `alt`) behind the `EventTargetStringIdentifierUsesQuotesEnabled` flag, similar to how `src` is currently guarded? Or should they be enabled by default since this is a new feature? 2. **Handling Detached Nodes (The "Async" Issue):** There is a limitation with the current traversal logic when nodes are removed immediately from the DOM (e.g., `element.remove()` in a click handler).
- The traversal relies on `current->parentNode()`. If the node is detached before `EventTargetToString` runs (which happens asynchronously for Event Timing), `parentNode()` returns `nullptr`.
- The algorithm fails to find ancestors and falls back to just the target selector (e.g., `SPAN` instead of `DIV#card > SPAN`).
- I've created a demo here: [https://jsbin.com/fixabuzeru/edit?html,output](https://jsbin.com/fixabuzeru/edit?html,output). If you uncomment `this.remove()` in `handleInteraction`, the parent context is lost. If you use `display: none`, it works correctly.
- Is there a synchronous hook we should use instead?
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mohamed Youns (Gerrit)
Mohamed Youns added 1 comment![]( "Open in Gerrit")
I've implemented the traversal algorithm (checking ancestors for ID/Name/Alt/Src) as discussed.
I wanted to highlight two specific open questions:
1. **Feature Flag for New Attributes:** Should I put the new attribute checks (e.g., `name`, `alt`) behind the `EventTargetStringIdentifierUsesQuotesEnabled` flag, similar to how `src` is currently guarded? Or should they be enabled by default since this is a new feature? 2. **Handling Detached Nodes (The "Async" Issue):** There is a limitation with the current traversal logic when nodes are removed immediately from the DOM (e.g., `element.remove()` in a click handler).
- The traversal relies on `current->parentNode()`. If the node is detached before `EventTargetToString` runs (which happens asynchronously for Event Timing), `parentNode()` returns `nullptr`.
- The algorithm fails to find ancestors and falls back to just the target selector (e.g., `SPAN` instead of `DIV#card > SPAN`).
- I've created a demo here: [https://jsbin.com/fixabuzeru/edit?html,output](https://jsbin.com/fixabuzeru/edit?html,output). If you uncomment `this.remove()` in `handleInteraction`, the parent context is lost. If you use `display: none`, it works correctly.
- Is there a synchronous hook we should use instead?
1. After the fix of the async issue with the event target https://chromium-review.googlesource.com/c/chromium/src/+/7617568, I think we are ready to update the WPT test
2. I updated the [demo](https://jsbin.com/daherucora/edit?html,output) as well to cover all uniqe cases
3. The WPT tests have been updated with the new use cases (e.g., SVG, shadow DOM)
Related details
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mohamed Youns (Gerrit)
Mohamed Youns added 1 comment![]( "Open in Gerrit")
// WebDriver would fail if there any intraction with the shadow DOM
await interactAndObserve('click', host, observerPromise);A quick question about the Shadow DOM WPT test.
I tried to click the internal shadow button directly:
await interactAndObserve('click', btn, ...);
But testdriver.js fails locally with: "Error: element click intercepted error".
I changed it to click the Shadow Host instead:
await interactAndObserve('click', host, ...);
This works perfectly. A physical click on the host naturally reaches the inner button and bubbles up. This triggers C++ EventPath logic exactly like a real user interaction, proving that our code correctly stops at the ShadowRoot and returns "DIV#shadow-host".
Does this approach align with standard WPT practices, or is there a specific testdriver API I should use to click inside a Shadow DOM?
To see the issue in action
1. change await interactAndObserve('click', host, observerPromise); to await interactAndObserve('click', btn, observerPromise);
2. run `third_party/blink/tools/run_web_tests.py -t Default external/wpt/event-timing/target-selector.tentative.html`
Mohamed Youns (Gerrit)
Mohamed Youns voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 9 comments![]( "Open in Gerrit")
I haven't finished review, just sending initial comments. Mostly:
- may you please split the WPT into smaller files in a separate directory to make it easier to read each expectation?
- the new implementation seems to change the helper that LoAF uses and this may changed specced behaviour. I would not change that function at all (see the comment at the top of the function).
- some initial design feedback on the API design for the other parts.
[EventTiming] Enhance targetSelector with context-aware traversalWhat is "context-aware traversal" ?
This CL builds upon the EventPath integration in #7617568 to revamp theYou can use crrev.com/c/7617568 to link this, the # syntax doesn't do anything special in most of the uis for chrome changelogs
on classes for identification, only the first class is extracted and
appended (e.g., DIV.primary-card).Proposal: don't expose any className unless there is only a single unique className.
I don't know if there are use cases for 2 classNames, but I know that if there is a list of ~50 classNames on an element, using just the "first one" isn't that useful, either.
3. The traversal algorithm strictly respects Web Component boundaries.
If the path hits a ShadowRoot, the internal collection is safelyIs this a new key change? Or is this just part of how the event path works?
4. Updates target-selector.tentative.html to enforce strict string
matching. Adds test coverage for name attributes, deep nesting limits,
SVG elements and Shadow DOM encapsulation boundaries.FYI If there are multiple different dom structures to test, its often better to create multiple distinct small test files.
It might even be useful to create a separate directory like: `event-timing/target-selector/tentative/*.html`
String GetElementSelector(Element* element, bool* out_is_strong) {Nit: `bool*` is a sign of an imperfect design.
I think if you flip the order: return bool, accept `String&`, that already feels better.
---
But looking at the previous implementation, it basically did the following:
- Get the EventPath and iterate all the nodes "bottom up"
- Check if any given node is "strong" by checking if it has `HasID`
- Convert each element to String, and append to a Vector
- Concatenate the Vector in reverse to a string.
I wonder if you can just decouple the "is_string" check into its own little function, and replace where we used to hard-code the "HasId" check.
(I think that "strong" is just HasId || NameAttr || SrcAttr, although I would argue that having a singleton className is also strong)
AtomicString EventTargetToString(EventTarget* target) {This code path is used by LoAF-- I haven't fully reviewed the changes but it looks like you changed the implementation here?
// WebDriver would fail if there any intraction with the shadow DOM
await interactAndObserve('click', host, observerPromise);A quick question about the Shadow DOM WPT test.
I tried to click the internal shadow button directly:
await interactAndObserve('click', btn, ...);
But testdriver.js fails locally with: "Error: element click intercepted error".
I changed it to click the Shadow Host instead:
await interactAndObserve('click', host, ...);This works perfectly. A physical click on the host naturally reaches the inner button and bubbles up. This triggers C++ EventPath logic exactly like a real user interaction, proving that our code correctly stops at the ShadowRoot and returns "DIV#shadow-host".
Does this approach align with standard WPT practices, or is there a specific testdriver API I should use to click inside a Shadow DOM?
To see the issue in action
1. change await interactAndObserve('click', host, observerPromise); to await interactAndObserve('click', btn, observerPromise);
2. run `third_party/blink/tools/run_web_tests.py -t Default external/wpt/event-timing/target-selector.tentative.html`
I haven't tested `testdriver.js` for interacting with targets inside a closed shadow dom, and I don't see any mention of what is expected in the docs: https://web-platform-tests.org/writing-tests/testdriver.html#user-interaction
In general, I do think its common for web component based testing to simulate interactions with the host element, or with a specific co-ordinate that falls within the bounds of the host component-- so your solution is reasonable I think.
---
You might also want to search for existing interaction simulations inside the shadow-dom WPT suite in case there is a solution:
Related details
- Mohamed Youns
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mohamed Youns (Gerrit)
Mohamed Youns added 5 comments![]( "Open in Gerrit")
[EventTiming] Enhance targetSelector with context-aware traversalWhat is "context-aware traversal" ?
It's the term I used to describe the new selector logic (algorithm)
- "Context-Aware" instead of looking at a generic target (like a <span>), the logic looks at its attributes (id, src, etc) to give it meaning/context
- "Traversal" it traverses up (parent, grandparent) to find a new context if there is no one in the current element
This CL builds upon the EventPath integration in #7617568 to revamp theYou can use crrev.com/c/7617568 to link this, the # syntax doesn't do anything special in most of the uis for chrome changelogs
Done
on classes for identification, only the first class is extracted and
appended (e.g., DIV.primary-card).Proposal: don't expose any className unless there is only a single unique className.
I don't know if there are use cases for 2 classNames, but I know that if there is a list of ~50 classNames on an element, using just the "first one" isn't that useful, either.
Just to clarify your proposal, do you mean we should strictly check if element `ClassNames().size() == 1`, and if it has multiple classes (> 1 class), we ignore the class attribute entirely and continue traversing up?
3. The traversal algorithm strictly respects Web Component boundaries.
If the path hits a ShadowRoot, the internal collection is safelyIs this a new key change? Or is this just part of how the event path works?
The old logic didn't consider the shadow element; now it considers it and doesn't expose the context if any
4. Updates target-selector.tentative.html to enforce strict string
matching. Adds test coverage for name attributes, deep nesting limits,
SVG elements and Shadow DOM encapsulation boundaries.FYI If there are multiple different dom structures to test, its often better to create multiple distinct small test files.
It might even be useful to create a separate directory like: `event-timing/target-selector/tentative/*.html`
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |