Add-ons: Injecting stylesheets doesn't work

RESOLVED WONTFIX

Status

RESOLVED WONTFIX
4 years ago
3 years ago

People

(Reporter: eeejay, Assigned: fabrice)

Tracking

unspecified
All
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.6?)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
You should be able to specify CSS files via addon manifest.
(Assignee)

Comment 1

4 years ago
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?
Blocks: 1192026
Flags: needinfo?(dbaron)
(Assignee)

Comment 2

4 years ago
Cameron & Daniel, let me know if you are comfortable reviewing that. The usual suspects are in "no review" mode.
Attachment #8647247 - Flags: review?(dholbert)
Attachment #8647247 - Flags: review?(cam)
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?
I don't, sorry. :-/
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.)
Attachment #8647247 - Flags: review?(dholbert)
(Assignee)

Comment 6

4 years ago
Jet, do you know who could help us there?
Flags: needinfo?(bugs)
(Reporter)

Comment 7

4 years ago
I'm surprised there is no way to do async css loading already. what does devtools do?
(Assignee)

Comment 8

4 years ago
(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.
(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.
(Assignee)

Comment 10

4 years ago
I don't think we need unsafe rules and system principal. We just need what LoadSheetUsingURIString() does but with an async load.
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?
Attachment #8647247 - Flags: review?(cam) → review?(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 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?
Attachment #8647247 - Flags: review?(bzbarsky) → review-
Flags: needinfo?(bugs)
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1216543
[Blocking Requested - why for this release]: May be wanted for 2.5 uplift, needs evaluation.
blocking-b2g: --- → 2.5?
+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.
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.
blocking-b2g: 2.5? → 2.5+
Flags: needinfo?(cam)
(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?
Flags: needinfo?(cam) → needinfo?(fabrice)
(Assignee)

Comment 19

3 years ago
Hi Cameron, probably not before ~10 days, so feel free to steal!
Flags: needinfo?(fabrice)
(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.

Updated

3 years ago
QA Whiteboard: [COM=Add-on]
any progress here?
(Assignee)

Comment 22

3 years ago
(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.

Updated

3 years ago
Depends on: 1226432
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
blocking-b2g: 2.5+ → 2.6?

Comment 24

3 years ago
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.
(Assignee)

Updated

3 years ago
Flags: needinfo?(dbaron)

Comment 25

3 years ago
This only pertained to app:// protocol requests IIUC.  And app:// is removed (see bug 1285170)
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.