Closed Bug 1502346 Opened 6 years ago Closed 5 years ago

Slotted rules don't show up in the inspector.

Categories

(Core :: CSS Parsing and Computation, defect, P2)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla66
Tracking Flags:
Tracking Status
firefox64 --- wontfix
firefox65 --- fixed
firefox66 --- fixed

People

(Reporter: emilio, Assigned: jdescottes)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files)

slotted_rules.html
317 bytes, text/html
Details
Bug 1502346 - Retrieve ::slotted rules in InspectorUtils getCSSStyleRules;r=emilio
47 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-beta+
Details | Review 
<!doctype html>
<div id="host"><div></div></div>
<script>
  host.attachShadow({ mode: "open" }).innerHTML = `
    <style>
      ::slotted(div) {
        background: red;
        width: 100px;
        height: 100px;
      }
    </style>
    <slot></slot>
  `;
</script>

Shows a red div, but when you inspect it you don't see the rule.
Blocks: devtools-webcomponents
Priority: -- → P2
Similar but not identical to Bug 1490615
See Also: → 1490615
Attached file slotted_rules.html — 
test case
It looks like for now we can't get the rule from Servo.

Devtools is calling InspectorUtils::getCSSStyleRules ([1]), which then calls Servo_ComputedValues_GetStyleRuleList. When testing with a debug build, we get the following crash:

  We should be able to map a raw rule to a rule: StyleRule { selectors: SelectorList([Selector(::slotted(div), specificity = 0x2)]), block: [background-color: red, background-position-x: 0%, background-position-y: 0%, background-repeat: repeat, background-attachment: scroll, background-image: none, background-size: auto, background-origin: padding-box, background-clip: border-box, width: 100px, height: 100px], source_location: SourceLocation { line: 2, column: 7 } }
  ) at /builds/worker/workspace/build/src/layout/inspector/InspectorUtils.cpp:225

So it looks like we are unable to get the rule when calling `&shadow->ServoStyleRuleMap()`? [2]

[1] https://searchfox.org/mozilla-central/rev/6eae3b2be58c3ecb05e1b0159a1f5a785d2cc727/layout/inspector/InspectorUtils.cpp#150
[2] https://searchfox.org/mozilla-central/rev/6eae3b2be58c3ecb05e1b0159a1f5a785d2cc727/layout/inspector/InspectorUtils.cpp#199-206

Emilio: I would like to try and fix this myself in Servo, let me know if you have any pointers I could use to get started on this (assuming my investigation is correct!)
Flags: needinfo?(emilio)
Oh, that makes total sense, actually.

This doesn't need a Servo fix, just an InspectorsUtils change so that we look up rules in trees where ::slotted can come from.

Should be something like:

for (each of the assigned slots the rules can come from)
  if (the containing shadow of the slot is not null)
    maps.AppendElement(&shadow->ServoStyleRuleMap());

Around here:

  https://searchfox.org/mozilla-central/rev/6eae3b2be58c3ecb05e1b0159a1f5a785d2cc727/layout/inspector/InspectorUtils.cpp#207

Would you still want to give a go at writing the patch?

Tyson, do you know if fuzzers could fuzz the InspectorUtils APIs? That should make these errors trivial to catch.

I made the API available to fuzzers in bug 1493222, but I guess the fuzzers still don't have the IDL file around? This method is by far the most interesting:

  https://searchfox.org/mozilla-central/rev/6eae3b2be58c3ecb05e1b0159a1f5a785d2cc727/dom/chrome-webidl/InspectorUtils.webidl#16
Component: CSS Rules Inspector → CSS Parsing and Computation
Flags: needinfo?(twsmith)
Flags: needinfo?(jdescottes)
Flags: needinfo?(emilio)
Product: DevTools → Core
(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)
> Oh, that makes total sense, actually.
> 
> This doesn't need a Servo fix, just an InspectorsUtils change so that we
> look up rules in trees where ::slotted can come from.
> 
> Should be something like:
> 
> for (each of the assigned slots the rules can come from)
>   if (the containing shadow of the slot is not null)
>     maps.AppendElement(&shadow->ServoStyleRuleMap());
> 
> Around here:
> 
>  
> https://searchfox.org/mozilla-central/rev/
> 6eae3b2be58c3ecb05e1b0159a1f5a785d2cc727/layout/inspector/InspectorUtils.
> cpp#207
> 
> Would you still want to give a go at writing the patch?
> 

And I thought I found a perfect excuse to do some rust :) 
But sure, happy to try fixing it using your pointers.
Flags: needinfo?(jdescottes)
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)
> Tyson, do you know if fuzzers could fuzz the InspectorUtils APIs? That
> should make these errors trivial to catch.

I'll defer to Jason here since Domino is fuzzer this will be added to first.
Flags: needinfo?(twsmith) → needinfo?(jkratzer)
We don't currently have support for the InspectorUtils.webidl but it should be easy to add.  I can implement this right after the new year.
Flags: needinfo?(jkratzer)
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7acbf06c74dc
Retrieve ::slotted rules in InspectorUtils getCSSStyleRules;r=emilio
Blocks: 1514960
Filed Bug 1514960 for fuzzing InspectorUtils APIs.
https://hg.mozilla.org/mozilla-central/rev/7acbf06c74dc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox66: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Emilio, do you think this is safe to uplift to beta? Should we let it bake on Nightly for a few days before?
Flags: needinfo?(emilio)
I think it's pretty safe to uplift, yeah.
Flags: needinfo?(emilio)
Comment on attachment 9031902 [details]
Bug 1502346 - Retrieve ::slotted rules in InspectorUtils getCSSStyleRules;r=emilio

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1053898

User impact if declined: Users can not inspect ::slotted selectors in the DevTools inspector, and can't debug those CSS rules if they want to use ShadowDOM

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): small change, only used by DevTools Inspector

String changes made/needed:
Attachment #9031902 - Flags: approval-mozilla-beta?
Comment on attachment 9031902 [details]
Bug 1502346 - Retrieve ::slotted rules in InspectorUtils getCSSStyleRules;r=emilio

[Triage Comment]
Fixes some debugging issues with Shadow DOM. Thanks for including a test. Approved for 65.0b6.
Attachment #9031902 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: