Closed Bug 1836755 Opened 6 months ago Closed 6 months ago

CSS variables set on the shadow dom host element are shown as not set in the inherited dom

Categories

(DevTools :: Inspector: Rules, defect)

Product:
DevTools
Component:
Inspector: Rules
Version:
Firefox 114
Type:
defect

Tracking

(firefox116 fixed)

Status:
RESOLVED FIXED
Milestone:
116 Branch
Tracking Flags:
Tracking Status
firefox116 --- fixed

People

(Reporter: ff2400t, Assigned: emilio)

Details

Attachments

(2 files)

Bug 1836755 - Improve behavior of :host in InspectorUtils.selectorMatchesElement. r=jdescottes
48 bytes, text/x-phabricator-request
Details | Review
Bug 1836755 - [devtools] Use flattenedTreeParentNode to build matching selectors
48 bytes, text/x-phabricator-request
Details | Review

Actual results:

CSS variables set on the Custom Element Root with the :host selector are show as being not set in the Rules Panel i.e. their color is grey. Example

Thanks for the report, will investigate.

Flags: needinfo?(jdescottes)

The issue here is around our logic to decide if a specific selector matches an element, which relies on _buildMatchedRules and selectorMatchesElement (https://searchfox.org/mozilla-central/rev/aec3a901e6f6b3041b5ec457c9111a042cef1fb1/devtools/server/actors/inspector/css-logic.js#471-483)

They both loop on

   while (
      (element = element.parentNode) &&
      element.nodeType === nodeConstants.ELEMENT_NODE
    );

which makes them miss selectors which apply to the shadow root.

I tried updating the code to loop on shadow hosts when encountering a shadow root, but I am blocked on InspectorUtils.selectorMatchesElement(element, domRule, idx) which only seems to accept elements, and I'm not sure how this could be used to match the :host pseudo selector.

Emilio: do you have a suggestion to match :host selectors using selectorMatchesElement or should we have some custom logic?

Flags: needinfo?(jdescottes) → needinfo?(emilio)

So if the element is a shadow root, you should pass it's .host, which is an element.

But I'm a bit confused about what that code is trying to do. Is it building rules from inheritance ancestors? Because looking at .parentNode is not the right thing to check there, you want .flattenedTreeParentNode.

But all those rules come from the style engine, so at least one of them necessarily match, can't you skip all that work when the selector list length is 1?

Flags: needinfo?(emilio) → needinfo?(jdescottes)

Thanks for flattenedTreeParentNode, I didn't know about that.

I guess I got confused when debugging, the problem is actually that when looping on flattenedTreeParentNode, selectorMatchesElement always returns false for the :host rule.

Now for the reason why we have this logic is that for each rule we return for an element, we keep track of which selectors are matching it. We render "matching" selectors differently from non-matching selectors. And if users update the selector so that it no longer matches the element, we keep the rule displayed as the long as the user doesn't change the selected element (because otherwise it would very hard for them to edit selectors). So basically we try to track quite finely which selectors of a dom rule match the element.

And finally our logic to display the value of css variables is fully client side and relies on this. We check the rules displayed for the element which have a matching selector, and we see if we can find the css variable defined in there.

Not against revisiting this, but do you think selectorMatchesElement could be updated to return true when used with a :host rule on the corresponding host element?

Flags: needinfo?(jdescottes) → needinfo?(emilio)

So part of the issue is that whether :host matches depends on where the rule is.

E.g., if I have:

<style>
#div, :host {
  color: red;
}
</style>
<div id="div"></div>
<script>
  div.attachShadow({ mode: "open" }).innerHTML = `
    <style>#div, :host { background-color: red }</style>
  `;
</script>

Ideally the result should be that #div matches on the first one and :host on the second one.

Flags: needinfo?(emilio)

I sent a patch to improve the behavior of :host. DevTools should still use .flattenedTreeParentNode, and fixing the second #div case there is a bit harder, but I don't think it matters much in practice.

For non-adopted sheets, use the owner tree. Those are always good to go.

For constructed stylesheets, try to find the most likely candidate. It's
not perfect but it's likely to always be correct in practice.

We could ask for the specific host instead, though that can get rather
annoying in practice.

Assignee: nobody → emilio
Status: NEW → ASSIGNED

Thanks Emilio! I'll take a look at the patch and will provide the devtools changes and test.

Keywords: leave-open
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0e2078b102ff
Improve behavior of :host in InspectorUtils.selectorMatchesElement. r=jdescottes,layout-reviewers,dshin
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05f4ed5e1027
[devtools] Use flattenedTreeParentNode to build matching selectors r=emilio,devtools-reviewers,nchevobbe
Keywords: leave-open

https://hg.mozilla.org/mozilla-central/rev/05f4ed5e1027

Status: ASSIGNED → RESOLVED
Closed: 6 months ago
status-firefox116: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 116 Branch

Removing the [stockwell disable-recommended] tag as it was caused by a spam user starring jobs against this bug.
Sorry for the inconvenience!

Whiteboard: [stockwell disable-recommended]
You need to log in before you can comment on or make changes to this bug.