1839182 - CSS Warning message selector should be the desugared selector

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Nicolas Chevobbe [:nchevobbe][PTO back September 7]]

Steps to reproduce

1. Navigate to data:text/html,<meta charset=utf8><style>div { & span { color: blouge; background: cyan; }}</style><div><span>hello</span></div><span>world</span>
2. Open the console
3. Enable the CSS filter if it wasn't the case
4. Expand the CSS warning tgo display impacted element

Expected results

In the displayed NodeList, there's only 1 impacted element (the span inside the div)

Actual results

There are 2 impacted elements (including the span outside the div)

---

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Where do those impacted elements come from? I don't think we have code for that in platform.

Comment 2

[Nicolas Chevobbe [:nchevobbe][PTO back September 7]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #1)

Where do those impacted elements come from? I don't think we have code for that in platform.

We're using https://searchfox.org/mozilla-central/rev/4e8f62a231e71dc53eb50b6d74afca21d6b254e9/dom/bindings/nsIScriptError.idl#125-129 to execute a querySelectorAll on the document, which gets us the elements

We might keep cssSelectors as is since we display it , but we'd need the desugared selectors to be able to get the impacted elements

Comment 3

[Nicolas Chevobbe [:nchevobbe][PTO back September 7]]

Attachment: Bug 1839182 - [devtools] Add test case for CSS warning on nested rule. r=#devtools-reviewers.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

So implementing the desugaring at that stage is quite tricky, because it makes CSS parsing slower, by making us keep track of the ancestor style rule selectors which we generally don't keep around.

Comment 5

[Nicolas Chevobbe [:nchevobbe][PTO back September 7]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)

So implementing the desugaring at that stage is quite tricky, because it makes CSS parsing slower, by making us keep track of the ancestor style rule selectors which we generally don't keep around.

Alright that's good to know. That's not something we want to have immediately anyway, the selector is only shown if you expand the message in the console.
Do you think we could have a cssRule property on nsIScriptError so DevTools can use that build the desugared selector with CssRule#domRule.selectorTextAt ?

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1839182 - Use desugared selector list in css error reports. r=nchevobbe

Hopefully this isn't too slow, and we only do this when error reporting
is enabled...

Comment 7

[Emilio Cobos Álvarez [:emilio]]

No, because at the point we emit the errors we still haven't built the rule, we're parsing the declarations. But I wrote a patch to expose the desugared selector, hopefully the extra bookkeeping doesn't show up in profiles, it being enabled only when error reporting.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/beeb776a1819
Use desugared selector list in css error reports. r=nchevobbe,firefox-style-system-reviewers,boris

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f39f5b1e6230
[devtools] Add test case for CSS warning on nested rule. r=devtools-reviewers,ochameau.

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/beeb776a1819
https://hg.mozilla.org/mozilla-central/rev/f39f5b1e6230