Web Inspector: DOM.highlightSelector should work for "a:visited"

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

Currently, Document.querySelectorAll does not match selectors that have ":visited" pseudo-selectors.  This should be implemented as it currently works for CSS rules and should therefore also work for JavaScript.

[Radar WebKit Bug Importer]

<rdar://problem/21467303>

[Joseph Pecoraro]

The original description sounds wrong.

It is expected behavior that ":visited" pseudo-selectors doesn't work with querySelectorAll for privacy reasons.

However, for the inspector to highlight all nodes matching a selector, we should properly highlight elements matching the ":visited". The current implementation uses document.querySelectorAll and therefore misses them. We should covert to something more powerful internally to really find all the nodes.

[Devin Rousso]

Created attachment 381210 [details]
[Patch] WIP

Current bugs:
 - `:before` and `:after` don't match anything
 - the selector `div` will match internal shadow elements, like the `<div>` inside an `<input>`

The result I'm hoping to eventually get is what the CSS selector matching engine actually results in when it applies styles from a style sheet.

[Antti Koivisto]

Comment on attachment 381210 [details]
[Patch] WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=381210&action=review

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:1260
> +        SelectorChecker::CheckingContext context(SelectorChecker::Mode::CollectingRules);

I think you can make virtual pseudo elements (::before, ::after, ::first-line etc) work like this:

1) Switch to SelectorChecker::Mode::ResolvingStyle
2) After match() check if context.pseudoIDSet contains a pseudo element (match return false in this case).

[Antti Koivisto]

>  - the selector `div` will match internal shadow elements, like the `<div>`
> inside an `<input>`

You should probably just skip everything in UA shadow trees (element.containingShadowRoot()->mode() == ShadowRootMode::UserAgent) that doesn't have pseudoId().

[Devin Rousso]

Created attachment 381364 [details]
Patch

[Devin Rousso]

Comment on attachment 381364 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381364&action=review

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:1266
> +        if (auto* beforePseudoElement = descendantElement.beforePseudoElement())
> +            checkNode(*beforePseudoElement);
> +        if (auto* afterPseudoElement = descendantElement.afterPseudoElement())
> +            checkNode(*afterPseudoElement);

This works for selectors like `:before, but not `div:before`.

From what I can tell, `composedTreeDescendants` doesn't iterate over pseudo elements like `:before`/`:after`.

@Antti, is this correct, or am I missing something?

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:1273
> +            if (isInUserAgentShadowTree && (selector->match() != CSSSelector::PseudoElement || selector->serializingValue() != pseudo))

As you suggested @Antti, this works great to prevent nodes that don't have a corresponding pseudo-selector from being highlighted.  Thanks!

[Devin Rousso]

Created attachment 383405 [details]
Patch

[Antti Koivisto]

Comment on attachment 383405 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=383405&action=review

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:1276
> +            if (isInUserAgentShadowTree && (selector->match() != CSSSelector::PseudoElement || selector->serializingValue() != pseudo))

I thin that you can test 'CSSSelector::pseudoId(selector->pseudoElementType())) == pseudoId' instead of going via serializingValue. You won't need 'pseudo' variable either.

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:1306
> +                if (pseudoIDs) {
> +                    nodes.append(descendantElement);
> +                    break;
> +                }

Not sure if this is the right thing to do for pseudo elements that don't match any element (::first-line etc). Maybe it is?

[Devin Rousso]

Comment on attachment 383405 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=383405&action=review

>> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:1276
>> +            if (isInUserAgentShadowTree && (selector->match() != CSSSelector::PseudoElement || selector->serializingValue() != pseudo))
> 
> I thin that you can test 'CSSSelector::pseudoId(selector->pseudoElementType())) == pseudoId' instead of going via serializingValue. You won't need 'pseudo' variable either.

This doesn't work with user-agent pseudo selectors like `::-webkit-textfield-decoration-container`.  I derived this logic from `CSSSelector::selectorText`, which made sense to me given that I'm trying to see what elements match the selector string given to this function.

This does need to be adjusted slightly to use `CSSSelector::value` instead of `CSSSelector::serializingValue`, as the latter doesn't work with `:placeholder`.

>> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:1306
>> +                }
> 
> Not sure if this is the right thing to do for pseudo elements that don't match any element (::first-line etc). Maybe it is?

I think it's better to at least highlight elements that would match `::first-line`, rather than show nothing.  This way, there's at least some visual indication of where to look.

[Devin Rousso]

Created attachment 383496 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 383496 [details]
Patch

Clearing flags on attachment: 383496

Committed r252436: <https://trac.webkit.org/changeset/252436>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Antti Koivisto]

Comment on attachment 383496 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=383496&action=review

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:1286
> +            if (selectorChecker.match(*selector, descendantElement, context, ignoredSpecificity)) {
> +                nodes.append(descendantElement);
> +                break;
> +            }

Shouldn't this continue instead of break? If you have selector like '.foo, .foo::before' I think the current code will only highlight .foo.

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:1305
> +                    break;

This too.

[Devin Rousso]

Comment on attachment 383496 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=383496&action=review

>> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:1286
>> +            }
> 
> Shouldn't this continue instead of break? If you have selector like '.foo, .foo::before' I think the current code will only highlight .foo.

Good catch!  I was thinking that once we matched a node, we wouldn't need to check it again, but you're example is a reason why we still have to. <https://webkit.org/b/204306>