1179536 - Add-ons: Injecting stylesheets doesn't work

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect)

Description

[Eitan Isaacson [:eeejay]]

You should be able to specify CSS files via addon manifest.

Comment 1

[[:fabrice] Fabrice Desré]

Currently the add-on code is using nsDOMWindowUtils::LoadSheetUsingURIString() to load stylesheets at https://mxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ExtensionContent.jsm#138 . This ends up in nsDocument::LoadAdditionalStyleSheet where we do a Loader::LoadSheetSync().

However on b2g, resources are served from the app:// protocol that only provides asyncOpen on its channels, so LoadSheetSync() fails. So instead of using LoadSheetUsingURIString() I plan to add nsDOMWindowUtils::LoadSheetAsync() that will use Loader::LoadSheet().

David, does that seem reasonable?

Comment 2

[[:fabrice] Fabrice Desré]

Attachment: addon-async-load-css.patch

Cameron & Daniel, let me know if you are comfortable reviewing that. The usual suspects are in "no review" mode.

Comment 3

[Cameron McCormack (:heycam)]

Comment on attachment 8647247 [details] [diff] [review]
addon-async-load-css.patch

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

::: dom/base/nsDocument.cpp
@@ +4472,5 @@
> +  nsCOMPtr<nsIPrincipal> principal;
> +  if (aType == eAgentSheet) {
> +    principal = nsContentUtils::GetSystemPrincipal();
> +  }
> +  return loader->LoadSheet(aSheetURI, principal, EmptyCString(), obs);

The set of sheet loading methods on Loader is a bit of a mess, sorry.

Ideally I'd like the use of this new nsIDocument::LoadAdditionalStyleSheetAsync method (by ExtensionContent.jsm, via the nsIDOMWindowUtils method) to be have just the same as the existing sync call, except that it's async.  But it looks like this use of LoadSheet will pass aUseSystemPrincipal = false down to InternalLoadNonDocumentSheet, whereas LoadAdditionalStyleSheet's LoadSheet call passes true for that arg.

We currently assert that all async loads are for aUseSystemPrincipal = false loads.  Maybe that's why you don't add a Loader::LoadSheet method that allows you to pass down aUseSystemPrincipal = true?  I'm concerned that these assertions are in here for some undocument reason I'm unaware of.

I don't know what the consequences are of using aUseSystemPrincipal = false (in terms of what the Loader actually does), so I can't say that it is correct for ExtensionContent.jsm's purposes.

I'm also not sure that explicitly passing in the system principal as the aOriginalPrincipal is the right thing to do, or even if it differs from passing null in.

Daniel, do you know more about the use of principals in the Loader here?

Comment 4

[Daniel Holbert [:dholbert]]

I don't, sorry. :-/

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8647247 [details] [diff] [review]
addon-async-load-css.patch

(In reply to [:fabrice] Fabrice Desré from comment #2)
> Cameron & Daniel, let me know if you are comfortable reviewing that.

(Sorry, I'm pretty backlogged on reviews/tasks right now, and I'm unlikely to realistically get a chance to do the necessary research to be able to feel comfortable reviewing this before I go on vacation next week. Canceling review request.)

Comment 6

[[:fabrice] Fabrice Desré]

Jet, do you know who could help us there?

Comment 7

[Eitan Isaacson [:eeejay]]

I'm surprised there is no way to do async css loading already. what does devtools do?

Comment 8

[[:fabrice] Fabrice Desré]

(In reply to Eitan Isaacson [:eeejay] from comment #7)
> I'm surprised there is no way to do async css loading already. what does
> devtools do?

As explained in comment 1, the platform-side css async loading is not exposed to nsDOMWindowUtils. This is what I want to do here.

Where in the devtools UI can we load css? Note that we don't want to inject a <link> tag.

Comment 9

[Cameron McCormack (:heycam)]

(In reply to [:fabrice] Fabrice Desré from comment #8)
> (In reply to Eitan Isaacson [:eeejay] from comment #7)
> > I'm surprised there is no way to do async css loading already. what does
> > devtools do?
> 
> As explained in comment 1, the platform-side css async loading is not
> exposed to nsDOMWindowUtils. This is what I want to do here.

To me it looks like we don't support loading of chrome style sheets (with their enabling of unsafe CSS rules and system principal) at all.  I am not confident right now to just go ahead and allow that.  I would like either Boris to OK that, or for me to investigate further to confirm what actually happens in that case to ensure that it's safe.  I'm busy until September 1 but can look into it then.

Comment 10

[[:fabrice] Fabrice Desré]

I don't think we need unsafe rules and system principal. We just need what LoadSheetUsingURIString() does but with an async load.

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8647247 [details] [diff] [review]
addon-async-load-css.patch

Boris, I'm pretty confused here about how aUseSystemPrincipal and the actual principal passed in to LoadSheet are related, and whether there is any difference between passing in null or the system principal as that argument.  Do you mind if I pass this patch on to you?

Comment 12

[Boris Zbarsky [:bzbarsky]]

aUseSystemPrincipal means "ignore the actual principal of the channel; when parsing the sheet give the sheet itself the system principal".  It's basically a hack around the fact that the UA stylesheets are loaded from res:// channels which would not normally given them the system principal.  So we assert that this boolean is only set when doing sync loads, which are known UA-provided sheets.

It has nothing to do with the actual principal passed to LoadSheet.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8647247 [details] [diff] [review]
addon-async-load-css.patch

The todo comments about system principal in the agent-sheet case don't make sense.  If you just want to skip the loading security checks, pass null for the principal.  If you want to give the stylesheet to be loaded a system principal, that needs a bit more plumbing on the loader side.

Some issues:

1)  There's a lot of code duplication here (e.g. the exact setup AsyncCSSLoaderObserver::StyleSheetLoaded does is a copy of existing code).  Might be better to refactor code a bit instead of just copying.

2)  Why create a new Loader each time instead of using the document's existing Loader?  That seems pretty fishy, since you're then relying on the Loader to stay alive even once you drop your ref to it....

3)  Is there a reason all this is better than implementing open() on app:// channels?

Comment 15

[Māris Fogels [:mars] (please needinfo)]

[Blocking Requested - why for this release]: May be wanted for 2.5 uplift, needs evaluation.

Comment 16

[Dietrich Ayala (:dietrich)]

+1 on blocking 2.5 for this. I've been writing add-ons this week, and immediately ran into scenarios where the whole add-on could be CSS only, and far less complex than writing a bunch of JS to do the same thing.

Comment 17

[Andrew Overholt [:overholt]]

Are you still working on this, Cameron?

Given comment 16 and the dupe, we should probably get this fixed on the 2.5 (Gecko 44) branch.

Comment 18

[Cameron McCormack (:heycam)]

(In reply to Andrew Overholt [:overholt] from comment #17)
> Are you still working on this, Cameron?

Fabrice, are you able to rework the patch based on bz's review in comment 13?

Comment 19

[[:fabrice] Fabrice Desré]

Hi Cameron, probably not before ~10 days, so feel free to steal!

Comment 20

[dynamis (Tomoya ASAI)]

(In reply to Dietrich Ayala (:dietrich) from comment #16)
> +1 on blocking 2.5 for this. I've been writing add-ons this week, and
> immediately ran into scenarios where the whole add-on could be CSS only, and
> far less complex than writing a bunch of JS to do the same thing.

Yes, CSS loading make Character design Theme feature possible and I believe it's very important features to get fan users who love FxOS. Here are sample add-on code to change design of Clock app by Japanese character designers:

https://github.com/fxosorg/losika-clock
https://github.com/fxosorg/losika-clock/blob/master/screenshot.png
https://github.com/fxosorg/konetamono-clock
https://github.com/fxosorg/konetamono-clock/blob/master/screenshot.png

Once this bug is fixed, we'll start to make more add-ons to change whole FxOS design.

Comment 21

[Mike Lien[:mlien]]

any progress here?

Comment 22

[[:fabrice] Fabrice Desré]

(In reply to Mike Lien[:mlien] from comment #21)
> any progress here?

I'm taking a look at getting sync loading working for the app: protocol to implement open() properly. That will also give us support for the different run_at phases when injecting content scripts.

Comment 23

[Mahendra Potharaju [:mahe]]

As we are trying to get 2.5 stable for TV, and this bug doesn't seem to land in the next week or so, moving it to 2.6

Comment 24

[Philippe]

I am currently experiment addon and creating some with the foxfooding programm, and I just want to say that this bug is really important and blocking. Creating style rules in JS files is an alternative but it's not intuitive at all for beginners. Addons are the best feature of FirefoxOS, I hope this will be a priority. I am not good enough to solve this bug, but good luck for people who will try to solve it and thank you.

Comment 25

[Jason Duell]

This only pertained to app:// protocol requests IIUC.  And app:// is removed (see bug 1285170)