Some of the Screenshots UI is in its overlay - an element cloned over to the content document's canvasFrame via document.insertAnonymousContent. This lets us draw the selection and highlight boxes while being opaque to the web content. (its the same technique used by devtools' highlighters) But it also means any CSS selectors to style that UI must be prefixed with :-moz-native-anonymous to match those elements (and to avoid our CSS bleeding into the content document.)

This in turn means we can't simply load the baseline, common stylesheets to give consistent style to the buttons and UI elements in the overlay. So we currently have to copy/paste the necessary rules over, manually inserting the prefix for every rule. There must be a better way.

My understanding is that its not possible to create a shadow root in the anonymous document - which would otherwise give us an easy, no-maintenance way to load in the shared CSS and have it be correctly scoped for the overlay UI.

We could of course do some post-processing to programatically adjust the stylesheet and insert the selector prefix. But as we don't already have this tooling in place for other browser components (about:newtab excepted) this seems a heavy-handed solution.

Any bright ideas :emilio?

So the reason you can't use shadow dom is rather arbitrary, and is because this cloneNode call doesn't clone it. We could do that, but also maybe there's not so much of a reason this API needs to be so restrictive...

The API doesn't expose nodes on purpose, but given it is trusted code maybe we can rejigger it a bit.

I think reworking this API so that all nodes are in a shadow tree of the anonymous node makes sense. Then you can just drop stylesheets there as needed...

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

So the reason you can't use shadow dom is rather arbitrary, and is because this cloneNode call doesn't clone it. We could do that, but also maybe there's not so much of a reason this API needs to be so restrictive...

I don't want to 2nd guess a historical decision here, that was presumably made for lots of good reasons. Maybe this was more necessary with XUL addons and the landscape has changed. Or not, I'm not in position to say.

I think reworking this API so that all nodes are in a shadow tree of the anonymous node makes sense. Then you can just drop stylesheets there as needed...

By "this API" you mean Document::InsertAnonymousContent? Or that Screenshots could get to the same goal with a different technique? I know devtools makes heavy use of the anonymous content. I don't know who else might have a stake in how it works today.

The historical decision was made so that chrome JS couldn't mess with the anonymous trees directly, because we rely on a node "remaining" anonymous... But in a world of shadow DOM we could make the actual anonymous node just the shadow host, and give chrome JS the ShadowRoot to play with.

There's the issue that they could call .host and move the host around, but I think the risk is fairly less than if we just exposed the node directly... WDYT Olli?

Giving a shadow root to the chrome JS sounds reasonable. I wonder if we could even add some MOZ_ASSERTs to ensure JS doesn't access the .host. Though, at least we should have already checks that NAC nodes aren't moved to be outside NAC.

WIP:

• DevTools highlighters work.

• Need to debug why accessiblecaret doesn't get the right height,
  probably something silly that I typoed.

• Need to rewrite the screenshots overlay.

• Need to rewrite the fancier find highlighter if we care about it (I
  suspect we don't).

Does something like comment 6 look appealing? If so I can finish it up, I just ran out of time r/n.

That looks really promising to me. It would definitely help eliminate a lot of the special cases, exceptions and maintenance headaches if we could reuse the same patterns and actual code we do elsewhere. :jdescottes, what do you think?

In bug 1820015 I'm in the process of figuring out how to make the overlay UI keyboard navigable, so you can tab / F6 to that stuff, the underlying content document and our toolbar and panel. I don't have a solution there yet, I just want to make sure this shadowRoot / anonymous content change doesnt somehow make that more difficult.

(In reply to Sam Foster [:sfoster] (he/him) from comment #9)

In bug 1820015 I'm in the process of figuring out how to make the overlay UI keyboard navigable, so you can tab / F6 to that stuff, the underlying content document and our toolbar and panel. I don't have a solution there yet, I just want to make sure this shadowRoot / anonymous content change doesnt somehow make that more difficult.

It shouldn't.

Nicolas, I see some failures on style inspector tests which are because of the new accessiblecaret.css files, which now show in the inspector because those are there by default.

This is a pre-existing issue, e.g., if I go to data:text/html,<video controls>, then I see videocontrols.css there even if by default I can't inspect the tree it applies to.

Do you know what's the best way to deal with them? I can special-case them / skip them in the tests but it feels rather weird they'd be there without showing the closed shadow trees etc (without the showAllAnonymousContent pref).

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

Nicolas, I see some failures on style inspector tests which are because of the new accessiblecaret.css files, which now show in the inspector because those are there by default.

This is a pre-existing issue, e.g., if I go to data:text/html,<video controls>, then I see videocontrols.css there even if by default I can't inspect the tree it applies to.

Do you know what's the best way to deal with them? I can special-case them / skip them in the tests but it feels rather weird they'd be there without showing the closed shadow trees etc (without the showAllAnonymousContent pref).

Thanks for the ping Emilio, I can indeed see that we have the issue with <video> (which is not even consistent on reload)
One thing that comes to mind is that we have a setting in DevTools: Show Browser Styles, which is off by default
When you turn it on, it will show the user agent stylesheet (and the rules in the rule view)
Given how we phrase the setting, it wouldn't be shocking if we could show them based on this pref value (even if the pref name, devtools.inspector.showUserAgentStyles, could be changed)
In the styleEditor, we even call those system stylesheet, so we can prevent people from editing them
The system flag is added from https://searchfox.org/mozilla-central/rev/0907529ff72c456ddb47839f5f7ba16291f28dce/devtools/shared/inspector/css-logic.js#127-136 . It doesn't seem that the sheet we should consider have any other properties that could let us discriminate those (besides their chrome:// scheme, but this doesn't look like a good fit) ?

If we can't find/add anything to the sheet themselves, then we could still classify them as "system" on the devtools server (by having a simple array with the sheet URLs), but that feels quite brittle.

The videocontrols.css stylesheet shows up consistently for me.

It seems to be returned by InspectorUtils::GetAllStyleSheets when called for the document of the inspected page. And as Nicolas said, there's nothing which clearly tells us that this stylesheet should be avoided (it's not even flagged as a system stylesheet AFAICT?).

It could make sense to hide chrome:// and resource:// unless showUserAgentStyles or showAllAnonymousContent is enabled? But we'd still need to list them when opening the toolbox for chrome privileged pages (eg about: pages).

Note that we also have an exclusion "list" (only one sheet for now in it) at https://searchfox.org/mozilla-central/rev/e941c8a6d49b4f62f361446de7a80214fb899186/devtools/server/actors/utils/stylesheets-manager.js#822-831

If there are not too many failures for now, I guess updating the tests is fine, and we can address that in a follow up.

Then either we can make GetAllStyleSheets exclude those stylesheets when called for regular web content document (with a flag to override this in case the prefs listed above are enabled), or we need to add some filtering in devtools.

(In reply to Sam Foster [:sfoster] (he/him) from comment #8)

That looks really promising to me. It would definitely help eliminate a lot of the special cases, exceptions and maintenance headaches if we could reuse the same patterns and actual code we do elsewhere. :jdescottes, what do you think?

Simplifying the solution around highlighters sounds great and Alex and Emilio already seem to be on it. So sounds good on my end as well!

:emilio - is this blocked or still feasible but needing time/priority to finish up? I'm building on this patch in bug 1820015. I'm still investigating there so I don't know yet if I'm blocked on this landing or not, but it would help to know the status here (and a rebase on current central would be a big help)

(In reply to Sam Foster [:sfoster] (he/him) from comment #14)

:emilio - is this blocked or still feasible but needing time/priority to finish up?

Definitely feasible... I just didn't find the time to debug / address Nicolas' concerns, but that's done now. Also rebased. Try is busted atm tho...

Hey emilio, sorry to bother you about this. This has been sitting for a while and I was wondering if you have time to wrap this up? It appears that this just needs to rebased but I could be wrong. Thanks!

Yeah, sorry for the lag, I had a couple other failures to debug, but I think they are all sorted out now. Let me try to get this landed.

Backed out for causing failures on browser_all_files_referenced.js

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel&revision=650ef77d1c4db532dcdede7a7ecd5bbd0ca218e0&selectedTaskRun=clQIhTLAStild7KG9Kbccg.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=421625191&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/47cfdb7396a28f0c4acc8961fabf86052eb1fa97

Backed out for causing multiple failures.

• Backout link

• Push with failures - mochitests plain
• Failure Log
• Failure line: PROCESS-CRASH | MOZ_ASSERT(mInitialZoomConstraints->mPresShellID == aPresShellId) [@ libxul.so + 0x00000000048f60f4] | toolkit/components/extensions/test/mochitest/mochitest.ini:toolkit/components/extensions/test/mochitest/mochitest-common.ini

• Push with failures - wpt
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | /html/browsers/browsing-the-web/navigating-across-documents/empty-iframe-load-event.html | Check execution order from nested timeout - assert_equals: Expected nested setTimeout to run second expected true but got false

https://hg.mozilla.org/mozilla-central/rev/4c734e252630

This is still backed out, latest changeset backed out is actually https://hg.mozilla.org/integration/autoland/rev/4c734e252630

This fixes the windows WPT failures in comment 23. The issue is that the
new accessiblecaret.css makes us delay the load event by a tick even in
about:blank.

Depends on D173998

Comment 26 also seems to fix the android assert.

https://hg.mozilla.org/mozilla-central/rev/f970de7b9443
https://hg.mozilla.org/mozilla-central/rev/60d4eb46c8b4
https://hg.mozilla.org/mozilla-central/rev/29f82336c736
https://hg.mozilla.org/mozilla-central/rev/ecb86cb403ab
https://hg.mozilla.org/mozilla-central/rev/76990d265adf