Skip to content

[Remote Inspection] Targeting heuristic may surface redundant elements in shadow roots#26941

Merged
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
whsieh:eng/272283
Apr 7, 2024
Merged

[Remote Inspection] Targeting heuristic may surface redundant elements in shadow roots#26941
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
whsieh:eng/272283

Conversation

@whsieh
Copy link
Member

@whsieh whsieh commented Apr 6, 2024

dc0433c

[Remote Inspection] Targeting heuristic may surface redundant elements in shadow roots
https://bugs.webkit.org/show_bug.cgi?id=272283
rdar://121251828

Reviewed by Megan Gardner and Richard Robinson.

Currently, in the case where a targeted element is (or contains a) shadow root that encapsulates
other candidates, we end up surfacing both the outer container element as well as the inner element
in the shadow root as targeted elements to the WebKit client.

Under normal circumstances, adding the outer element as a target should exclude the inner element
from being a candidate. However, it fails in this case because the logic to enforce this invariant:

```
candidates.removeAllMatching([&](auto& candidate) {
    if (target.ptr() != candidate.ptr() && !target->contains(candidate))
        return false;
```

...uses `Node::contains(…)`, which does not account for shadow roots into account. Fix this by
simply replacing this call with `containsIncludingShadowDOM`.

Test: ElementTargeting.ContentInsideShadowRoot

* Source/WebCore/page/ElementTargetingController.cpp:
(WebCore::selectorsForTarget):

There's also no point in surfacing a CSS selector to match content in shadow roots to the client. In
a subsequent patch, we should provide an alternate means to resolve content inside of shadow roots
that doesn't rely on (or relies on more than just) CSS selectors.

(WebCore::ElementTargetingController::findTargets):
* Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* Tools/TestWebKitAPI/Tests/WebKitCocoa/ElementTargetingTests.mm:
(TestWebKitAPI::TEST(ElementTargeting, ContentInsideShadowRoot)):
* Tools/TestWebKitAPI/Tests/WebKitCocoa/element-targeting-4.html: Added.

Add a new API test to cover this scenario.

Canonical link: https://commits.webkit.org/277171@main

4c54674

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-skia
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 tv-sim ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@whsieh whsieh requested a review from cdumez as a code owner April 6, 2024 23:33
@whsieh whsieh self-assigned this Apr 6, 2024
@whsieh whsieh added the Platform Portability improvements and other general platform improvements not driven directly by site bugs. label Apr 6, 2024
@whsieh
Copy link
Member Author

whsieh commented Apr 7, 2024

Thanks for the reviews!

@whsieh whsieh added the merge-queue Applied to send a pull request to merge-queue label Apr 7, 2024
…s in shadow roots

https://bugs.webkit.org/show_bug.cgi?id=272283
rdar://121251828

Reviewed by Megan Gardner and Richard Robinson.

Currently, in the case where a targeted element is (or contains a) shadow root that encapsulates
other candidates, we end up surfacing both the outer container element as well as the inner element
in the shadow root as targeted elements to the WebKit client.

Under normal circumstances, adding the outer element as a target should exclude the inner element
from being a candidate. However, it fails in this case because the logic to enforce this invariant:

```
candidates.removeAllMatching([&](auto& candidate) {
    if (target.ptr() != candidate.ptr() && !target->contains(candidate))
        return false;
```

...uses `Node::contains(…)`, which does not account for shadow roots into account. Fix this by
simply replacing this call with `containsIncludingShadowDOM`.

Test: ElementTargeting.ContentInsideShadowRoot

* Source/WebCore/page/ElementTargetingController.cpp:
(WebCore::selectorsForTarget):

There's also no point in surfacing a CSS selector to match content in shadow roots to the client. In
a subsequent patch, we should provide an alternate means to resolve content inside of shadow roots
that doesn't rely on (or relies on more than just) CSS selectors.

(WebCore::ElementTargetingController::findTargets):
* Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* Tools/TestWebKitAPI/Tests/WebKitCocoa/ElementTargetingTests.mm:
(TestWebKitAPI::TEST(ElementTargeting, ContentInsideShadowRoot)):
* Tools/TestWebKitAPI/Tests/WebKitCocoa/element-targeting-4.html: Added.

Add a new API test to cover this scenario.

Canonical link: https://commits.webkit.org/277171@main
@webkit-commit-queue
Copy link
Collaborator

Committed 277171@main (dc0433c): https://commits.webkit.org/277171@main

Reviewed commits have been landed. Closing PR #26941 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit dc0433c into WebKit:main Apr 7, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Apr 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Platform Portability improvements and other general platform improvements not driven directly by site bugs.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants