Closed
Bug 1049199
Opened 10 years ago
Closed 10 years ago
Style editor should show stylesheets associated with XBL bindings
Categories
(DevTools :: Style Editor, defect)
DevTools
Style Editor
Tracking
(firefox39 fixed)
RESOLVED
FIXED
Firefox 39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: MattN, Assigned: Gijs)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
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>
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8568185 -
Flags: review?(pbrosset)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
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•10 years ago
|
||
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.
Attachment #8568185 -
Flags: review?(pbrosset)
Comment 4•10 years ago
|
||
Panos: what's the best way to test, from an actor, if the actor is running for the browser toolbox?
Flags: needinfo?(past)
Assignee | ||
Comment 5•10 years ago
|
||
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.
Assignee | ||
Comment 6•10 years ago
|
||
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•10 years ago
|
||
(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.
Flags: needinfo?(past)
Assignee | ||
Comment 8•10 years ago
|
||
OK, let's just load those testcases over http instead...
Attachment #8569288 -
Flags: review?(pbrosset)
Assignee | ||
Updated•10 years ago
|
Attachment #8568185 -
Attachment is obsolete: true
Comment 9•10 years ago
|
||
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.
Attachment #8569288 -
Flags: review?(pbrosset) → review+
Assignee | ||
Comment 10•10 years ago
|
||
(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. :-(
Assignee | ||
Comment 11•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Attachment #8569288 -
Attachment is obsolete: true
Assignee | ||
Comment 12•10 years ago
|
||
Comment on attachment 8569754 [details] [diff] [review]
Patch for checkin assuming green try
Carrying over r+
Attachment #8569754 -
Flags: review+
Comment 14•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Updated•7 years ago
|
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•