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)

defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: MattN, Assigned: Gijs)

References

Details

Attachments

(1 file, 2 obsolete files)

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: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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 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)
Panos: what's the best way to test, from an actor, if the actor is running for the browser toolbox?
Flags: needinfo?(past)
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.
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).
(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)
Attached patch style editor should show XBL stylesheets, (obsolete) (deleted) — Splinter Review
OK, let's just load those testcases over http instead...
Attachment #8569288 - Flags: review?(pbrosset)
Attachment #8568185 - Attachment is obsolete: true
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+
(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. :-(
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
Attachment #8569288 - Attachment is obsolete: true
Comment on attachment 8569754 [details] [diff] [review] Patch for checkin assuming green try Carrying over r+
Attachment #8569754 - Flags: review+
Try is green! :-)
Keywords: checkin-needed
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Blocks: 1157292
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: