1840870 - Add test for InspectorUtils.getCSStyleRules with ::highlight pseudo elements

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, defect, P2)

Description

[Nicolas Chevobbe [:nchevobbe]]

With dom.customHighlightAPI.enabled , on a page with a ::highlight rule (e.g. https://live-samples.mdn.mozilla.net/en-US/docs/Web/API/CSS_Custom_Highlight_API/_sample_.highlighting_search_results.html), InspectorUtils.getCSSStyleRules does not return the highlight rule

My first reaction is that it should work like ::selection, but maybe I'm missing something

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1840870 - [devtools] Add test for InspectorUtils.getCSStyleRules with ::highlight pseudo. r=jjaschke.

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Jan, would you know why we don't get the ::highlight rule ? The test on https://phabricator.services.mozilla.com/D182445 does fail at the moment, and shows the difference with ::selection

Comment 3

[Jan Jaeschke [:jjaschke]]

Erm, totally not sure, but shouldn't the rule be ::highlight(search)?

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Jan Jaeschke [:jjaschke] from comment #3)

Erm, totally not sure, but shouldn't the rule be ::highlight(search)?

Could be, but I updated the test with this syntax, and it doesn't seem to work either (it doesn't even return any rule)
I'll have to check if we can get a grip on the highlighter id in DeTtools

Comment 5

[Jan Jaeschke [:jjaschke]]

I'll give it a spin in rr real quick.

Comment 6

[Jan Jaeschke [:jjaschke]]

Hi Emilio,

did I miss something here when implementing the custom highlight API? Or is this related to Bug 1838262?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Somewhat, related, yeah, in so far we don't parse ::highlight() from the apis, so we probably just early out and return nothing.

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Thanks for the answer Emilio, I'll block this on Bug 1838262

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

I can confirm that with (WIP) patch from Bug 1838262, InspectorUtils.getCSSStyleRules will return the rules for the ::highlight pseudo elements, but it can be surprising how it's handling them.

• InspectorUtils.getCSSStyleRules(node, "::highlight") does not return the expected rules setting the pseudo elements.
• InspectorUtils.getCSSStyleRules(node, "::highlight()") does return all the rules setting the pseudo elements.
• InspectorUtils.getCSSStyleRules(node, "::highlight(highlighterId)") does return all the rules setting the pseudo elements, ignoring the highlighter id

We get all the supported pseudo elements using InspectorUtils.getCSSPseudoElementNames(), and this will return ::highlight.
This is fine, as we can special case those and add the parens after it, after all we just want a way to be able to consume all rules for the pseudo elements.

But I guess that one could find the results confusing ?
Ideally, InspectorUtils.getCSSStyleRules(node, "::highlight") would return all the rules setting the pseudo elements (like the current behavior of InspectorUtils.getCSSStyleRules(node, "::highlight()")).

What do you think Jan?
Also, is this still aiming to be shipped in 117 ?

Comment 10

[Jan Jaeschke [:jjaschke]]

Hi Nicolas,

custom highlight is currently only enabled on Nightly and will be shipped once this issue is resolved (and it has been fuzzed for a bit). So there's no pressure to have this fixed before the soft freeze.

Bug 1838262 is not finished yet unfortunately, ::highlight(foo) really should return the results you expect. Since highlight is the first pseudo with parameters, I needed to add some new logic to parse etc. (hence the difference between ::highlight and ::highlight()).

My impression would be that only ::highlight(foo) should return anything meaningful, no? I'm not sure if ::highlight should return all highlight rules, since it's not a "complete" identifier. But have in mind that I'm still fairly new at Mozilla and may not know what I'm talking about :)

(Getting Emilio back in the loop here to correct me if I'm wrong)

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Jan Jaeschke [:jjaschke] from comment #10)

custom highlight is currently only enabled on Nightly and will be shipped once this issue is resolved (and it has been fuzzed for a bit). So there's no pressure to have this fixed before the soft freeze.

great :)

Bug 1838262 is not finished yet unfortunately, ::highlight(foo) really should return the results you expect. Since highlight is the first pseudo with parameters, I needed to add some new logic to parse etc. (hence the difference between ::highlight and ::highlight()).

My impression would be that only ::highlight(foo) should return anything meaningful, no? I'm not sure if ::highlight should return all highlight rules, since it's not a "complete" identifier. But have in mind that I'm still fairly new at Mozilla and may not know what I'm talking about :)

I think that makes sense.
What we can do on DevTools side is ,for ::highlight pseudo, to retrieve all the registered highlight (through CSS.highlights), and then call InspectorUtils.getCSSStyleRules(node, "::highlight(" + highlightEntryName +")"), for each entry.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Yeah ::highlight should not return anything / should fail to parse.

What we can do on DevTools side is ,for ::highlight pseudo, to retrieve all the registered highlight (through CSS.highlights), and then call InspectorUtils.getCSSStyleRules(node, "::highlight(" + highlightEntryName +")"), for each entry.

Maybe? But you probably want to avoid displaying all highlights for all elements unconditionally. You probably want to only display the highlights if it's selected somehow?

Comment 13

[Jan Jaeschke [:jjaschke]]

I've updated the WIP patch to the current state I have here locally. That should clarify things here a bit.
With this patch, ::highlight(foo) should only return what's expected.
However, when (in your patch) I tried getSelectors(::highlight), it returned #div1, not sure if that is intended.

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Jan Jaeschke [:jjaschke] from comment #13)

I've updated the WIP patch to the current state I have here locally. That should clarify things here a bit.
With this patch, ::highlight(foo) should only return what's expected.

Thanks, that does seems to work well 👍
I'm able to get the pseudo element rules displayed in the inspector, getting the highlighter name from the registry (ignoring the Highlight with a size of 0)

However, when (in your patch) I tried getSelectors(::highlight), it returned #div1, not sure if that is intended.

Not sure either, but at least this wasn't causing any trouble in the inspector down the line

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7ee1dad073d0
[devtools] Add test for InspectorUtils.getCSStyleRules with ::highlight pseudo. r=jjaschke.

Comment 16

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/7ee1dad073d0