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.
<rdar://problem/21467303>
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.
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.
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).
> - 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().
Created attachment 381364 [details] Patch
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!
Created attachment 383405 [details] Patch
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?
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.
Created attachment 383496 [details] Patch
Comment on attachment 383496 [details] Patch Clearing flags on attachment: 383496 Committed r252436: <https://trac.webkit.org/changeset/252436>
All reviewed patches have been landed. Closing bug.
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.
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>