1049199 - Style editor should show stylesheets associated with XBL bindings

Status: RESOLVED FIXED

(DevTools :: Style Editor, defect)

Description

[Matthew N. [:MattN]]

It would be awesome if I could edit stylesheets applied to XBL bindings. https://developer.mozilla.org/en-US/docs/XBL/XBL_1.0_Reference/Anonymous_Content#Binding_Stylesheets

Example markup:
<binding id="panelmultiview">
  <resources>
    <stylesheet src="chrome://browser/content/foo.css"/>
  </resources>
</binding>

Comment 1

[:Gijs (he/him)]

Attachment: style editor should show XBL stylesheets,

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=249311de0ea1

Comment 2

[:Gijs (he/him)]

I've not tried linking up any rules from the inspector to actually point at the right files.

The other issue is that this patch fixes the initial load of stylesheets, but if you e.g.:

1. open Firefox, and have 0 notification bars show up (can't use a new profile first time, because it tells you about e10s and FHR and whatnot)
2. open the browser toolbox
3. open the style editor
4. toggle some kind of notification

notification.css still won't show. If you then close and reopen the browser toolbox, it will.

I'm thinking if we should just have a 'reload' button on the style editor to reset the stylesheet list, only visible in the browser toolbox. I don't think it's worth implementing events for the XBL-based stylesheet loads.

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8568185 [details] [diff] [review]
style editor should show XBL stylesheets,

Review of attachment 8568185 [details] [diff] [review]:
-----------------------------------------------------------------

The code changes look good to me, apart from the isChrome thing which seems to be what makes tests fail.

::: toolkit/devtools/server/actors/stylesheets.js
@@ +133,5 @@
> +   */
> +  _shouldListSheet: function(doc, sheet) {
> +    // Special case about:PreferenceStyleSheet, as it is generated on the
> +    // fly and the URI is not registered with the about: handler.
> +    // https://bugzilla.mozilla.org/show_bug.cgi?id=935

Are you sure that's bug 935?

@@ +138,5 @@
> +    if (sheet.href && sheet.href.toLowerCase() == "about:preferencestylesheet") {
> +      return false;
> +    }
> +
> +    let isChrome = Services.scriptSecurityManager.isSystemPrincipal(doc.nodePrincipal);

So, this makes tests fail, because isChrome is true when tests are running.
I think `this.parentActor.isRootActor` should be enough to check whether the stylesheets actor is running for the browser toolbox or not.

@@ +147,5 @@
> +      return true;
> +    }
> +
> +    // Doublecheck this sheet is in document.styleSheets:
> +    return Array.some(doc.styleSheets, s => s == sheet);

It doesn't look like this will ever return false, unless I'm missing something.
Either isChrome is true, and we'd have returned earlier, or it's false, in which case sheet is one item of the doc.styleSheets already.

Comment 4

[Patrick Brosset <:pbro>]

Panos: what's the best way to test, from an actor, if the actor is running for the browser toolbox?

Comment 5

[:Gijs (he/him)]

This isn't just meant to work for the browser toolbox, it should work for the normal toolbox if the content page is a chrome page (with XUL+XBL bindings) like about:preferences.

Comment 6

[:Gijs (he/him)]

We can probably workaround the test framework being dumb by also checking for the page's protocol (ie isChrome should never be true for http pages).

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #4)
> Panos: what's the best way to test, from an actor, if the actor is running
> for the browser toolbox?

The actor doesn't have any knowledge about the client it is connected to, but since the browser toolbox works with global-scoped actors, inferring it from the parent actor sounds perfect.

Comment 8

[:Gijs (he/him)]

Attachment: style editor should show XBL stylesheets,

OK, let's just load those testcases over http instead...

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8569288 [details] [diff] [review]
style editor should show XBL stylesheets,

Review of attachment 8569288 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, good find! Why would these test pages be loaded via a chrome URL in the first place anyway?
I like these changes, r=me provided try is green.
I'm going to try and spend some time this morning understanding why the style-editor fails with unhandled promise rejections when  more stylesheets than expected are received. I'm thinking this might only be a problem of the style-editor test helper functions.

Comment 10

[:Gijs (he/him)]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #9)
> Comment on attachment 8569288 [details] [diff] [review]
> style editor should show XBL stylesheets,
> 
> Review of attachment 8569288 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ah, good find! Why would these test pages be loaded via a chrome URL in the
> first place anyway?
> I like these changes, r=me provided try is green.
> I'm going to try and spend some time this morning understanding why the
> style-editor fails with unhandled promise rejections when  more stylesheets
> than expected are received. I'm thinking this might only be a problem of the
> style-editor test helper functions.

Sorry, yes, the URL got lost somehow, but I did push this version of the patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=353010609870

it's only orange in the _xul test with the promise rejections. :-(

Comment 11

[:Gijs (he/him)]

Attachment: Patch for checkin assuming green try

Patrick figured out the xul test issues, and this patch includes a fix that works for both of us locally - try push to be sure: remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8217fb0d3da

Comment 12

[:Gijs (he/him)]

Comment on attachment 8569754 [details] [diff] [review]
Patch for checkin assuming green try

Carrying over r+

Comment 13

[:Gijs (he/him)]

Try is green! :-)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/f38ca99f983d

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f38ca99f983d