737003 - Offer a way to apply user agent stylesheet on a given document

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Alexandre Poirot [:ochameau]]

Currently, nsIStyleSheetService only allows to register global stylesheets.
If you want to target a particular document (like a given tab, or a given iframe, ...), you may try registering a global stylesheet and use -moz-document rule. But it will apply to all documents matching a particular URL, whereas you intend to apply a user agent stylesheet only for one particular document.

My usecase is that I'm an addon developer, or a jetpack core developer and I have to apply some stylesheets that should not be overloadaded, nor being visible by the content document.

Here is an example of such API:
nsIStyleSheetService.registerDocumentSheet(document, "chrome://browser/content/someCustomSheet.css");

Comment 1

[Boris Zbarsky [:bzbarsky]]

So what exactly are you trying to apply the stylesheet to?  A particular Document object?  A particular presentation?  Particular history entry?  Particular navigation context? It's clearly not a particular url, right?

How should this interact with printing (which uses a separate Document object from the original Document object)?

Comment 2

[Alexandre Poirot [:ochameau]]

I'd say a particular Document object. TBH I'm not a CSS expert so that I can easily fall into bad scenario. In my mind I think I just need something like what `loadAndRegisterSheet` do, but instead of targeting all top-level documents, it would target only a given document. (so only current document in history and it would not necessary match the related printing document)

Let me expose you what I'm doing. I'm trying to modify the DOM of a document during its loading. So I'm trying to keep the document content hidden until the modification is finished.

 // Hide the document ASAP, this code fires on 'document-element-inserted' event
 document.documentElement.style.visibility = "hidden";

 document.addEventListener("DOMContentLoaded", function listener() {
  // Do modify DOM
  translateDocument(document);

  // Display the document, only when it is ready. i.e. after modification so that we can't see any blinking/flashes, like webfonts one!
  document.documentElement.style.visibility = "visible";
 },false);

This code is really weak. Both sides, addon and document can easily collide and end up in a broken state. I think that user agent stylesheet behavior are better in that case, for the same reasons than bug 634764 comment 7 and 8.

Then it may simplify our upcoming implementation of "page-mod CSS" (=Like greasemonkey but for CSS). Addon authors can specify some CSS to customize websites. A patch is about to land in jetpack in order to implement this using -moz-document but it end up being quite hacky (generates some CSS text).

Does that make sense? Is this clear/specified enough?

Comment 3

[Boris Zbarsky [:bzbarsky]]

The other instances of the transfromation use case I've seen want to prevent loads from the document, in addition to not laying it out, until your transformation is done.  Do you not need that?

(Note that I'm not saying we shouldn't do this; just trying to figure out the requirements.  For example, for page-mod it seems like the styles should apply when printing too.)

Comment 4

[Alexandre Poirot [:ochameau]]

I don't really know how l20n is going to work in the platform code, but here I'd like to avoid flash effect, like the one happening with web fonts.
In jetpack, we have a very different context where locales are hosted locally, so that we do not have to wait for any network connection.
Here I used a CSS trick in order to just prevent this flash effect.

But you are right, there may be even better way to address this.
TBH, I don't really know what would be the best option. I think that it would be better to keep some simple Javascript hack like this one, instead of adding complex platform feature, like "prevent loading/laying out the document". But at the end I have no idea how complex would be such feature?

In anycase, we are clearly forking the discussion in two:
- localization hack
- page-mod usage, where we do not need anything else than what I decribed in this bug.

About printing, I don't think it should apply. I'd imagine that a `document-element-inserted` event fires for this new (printing) document instance and it will be up to the javascript code to determine if it should apply the CSS file or not.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> I'd imagine that a `document-element-inserted` event fires for this new (printing)
> document instance 

No, it most certainly does not.  That notification is fired by the parser, and the printing clone document is not created by a parser.

Comment 6

[Dave Townsend [:mossop]]

Gabor was going to do some investigating here

Comment 7

[Gabor Krizsanits (INACTIVE)]

So I'm quite noob in this area... 

> No, it most certainly does not.  That notification is fired by the parser,
> and the printing clone document is not created by a parser.

Is there an event we can catch here? Like when a document is hidden and the re-shown the selection even related to it is getting destroyed, but I found an even we could listen to and handle it. Do we have something similar here?

> The other instances of the transfromation use case I've seen want to prevent
> loads from the document, in addition to not laying it out, until your
> transformation is done.  Do you not need that?

Can I check that other use case, is there a bug for it? Just to get started with catching up somewhere...

Other than that it's not really clear to me what should be done here, so I'll just dive into nsIStyleSheetService related code to figure out what we have and follow this discussion in the meanwhile.

Comment 8

[Alexandre Poirot [:ochameau]]

I think that original bug comment still apply.
We had some fork in discussion about a particular usecase of this new feature for localization. But such new method is needed in anycase for page-mod.

To be clear, nsIStyleSheetService is missing similar method:
nsIStyleSheetService.registerDocumentSheet(document, "chrome://browser/content/someCustomSheet.css");

https://developer.mozilla.org/en/nsIStyleSheetService
nsIStyleSheetService currently only allows to register global sheets. They are applied by default to all documents. You can workaround this by using -moz-document CSS rule in order to apply a set of rules to a given document.
(https://developer.mozilla.org/en/CSS/@document)
But it doesn't work well with our usecase. We end up generating CSS files as dataURI and it is very weak. We end up with relative path issue with such thing...
So that we would like to have a new method on this component in order to register a whole CSS file being applied only to one given document reference.

Then Boris brings some implementation details about document used for printing. But we can live without this being addressed as I don't think that page-mod are currently applying on these documents.

Does that make sense?

Comment 9

[Gabor Krizsanits (INACTIVE)]

(In reply to Alexandre Poirot (:ochameau) from comment #8) 
> Does that make sense?

More or less yes. The problem is that I'm not familiar with this code, nor with the API so I don't know much about the how to implement all this... But I will look into it and come back later.

Comment 10

[Boris Zbarsky [:bzbarsky]]

If page-mod modifies the DOM, then of course it applies when printing: printing clones the already-modified DOM.

That was the point: it would be really weird if printing the document suddenly lost the page-mod modifications.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Is there an event we can catch here?

I don't know, but one could obviously be added as needed.

> Can I check that other use case, is there a bug for it?

People trying to "mobile-optimize" content on the fly, for example.  No bugs; mostly extensions and web devs trying to do this.

Comment 12

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Boris Zbarsky (:bz) from comment #1)

> So what exactly are you trying to apply the stylesheet to?  A particular
> Document object?  A particular presentation?  Particular history entry? 
> Particular navigation context? It's clearly not a particular url, right?

I'm not sure about localization hack, but for what concern the page-mod a "particular url" *could* be enough. Here I tried to give an idea of the big picture, and why we need that feature: https://github.com/mozilla/addon-sdk/wiki/contentStyle-issues

Comment 13

[Boris Zbarsky [:bzbarsky]]

If a particular url is good enough, then why is @moz-document not good enough?

Comment 14

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky (:bz) from comment #13)
> If a particular url is good enough, then why is @moz-document not good
> enough?

Just have run through the wiki page, so here is the summary I fetched from it.
"In addition, in order to applying the document filtering, we have to dynamically add the [-moz-document] rule to any contentStyle and contentStyleFile, and because the Stylesheet service requires a URL, we have to convert in a data: URL. This is not only a bit hacky, but more important it breaks relative paths." (bug 759190)

So if we could provide an API that instead of an URL takes a (data) URL AND a path (that will be used to locate relative paths), then it would solve the problem. I'm not sure if that is easy/possible.

Also there were some issued with not having the same behavior this way as in the style editor (bug 755606)

Comment 15

[Gabor Krizsanits (INACTIVE)]

About the original approach. Is it possible to simply add a user style sheet directly to the docshell of the document like mStyleSet->PrependStyleSheet(nsStyleSet::eUserSheet, oursheet)

Or where should a style sheet per document be stored and how can it be applied on the document?

Comment 16

[Boris Zbarsky [:bzbarsky]]

> I'm not sure if that is easy/possible.

It's possible.  You'd need to thread an optional base URI all the way down to CreateSheet in the CSSLoader, but once you do things should Just Work.

> mStyleSet->PrependStyleSheet

Style sets are ephemeral.  For example, taking a document in/out of fullscreen will rebuild the style set.  So just jamming something in the style set will sorta look like it works... until it breaks.

> Or where should a style sheet per document be stored and how can it be applied on the
> document?

That's the question, yes.  If it's really per-document-object, you could just store it on the document and then change the style set construction code to include it....

Comment 17

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #14)

I don't want to diverge from the original approach, that as the wiki said would solve the problem nicely. I posted the wiki URL just to give a better understanding of the big picture, and why we need this method.

> So if we could provide an API that instead of an URL takes a (data) URL AND
> a path (that will be used to locate relative paths), then it would solve the
> problem. I'm not sure if that is easy/possible.

I really prefer avoid the all conversion to data URL, and give to the method the real CSS file URL. It's not just because it's cleaner, but also because `-moz-document` is a "block" rule, so based on the CSS 2.1: "CSS 2.1 user agents must ignore any '@import' rule that occurs inside a block or after any valid statement other than an @charset or an @import rule".
Basically we adding on the fly the -moz-document rule, and if the user have @import rule on it, they won't work as they expected. And that also behave different from the style editor.

My only concern about register a stylesheet to a specific document using a document instance, when 'document-element-inserted' is notified, is the timing issue that brings the flickering effect. If that won't happen, have this method will solve all the issues we have in `contentStyleFile`.

Comment 18

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #14)

> Also there were some issued with not having the same behavior this way as in
> the style editor (bug 755606)

I added a platform bug as well for that: https://bugzilla.mozilla.org/show_bug.cgi?id=773602 (you're welcome to change the title of the bug with a shorter one. :) )

Comment 19

[Gabor Krizsanits (INACTIVE)]

Attachment: first attempt...

This is how far I got. I still need to add tests, and I want to make sure that I deliver the exact feature we need, so I want to test it against jetpack soon. But in the meanwhile care to take a look at it if I do the right thing? Also, do you have time to review this patch when it's done, or if not do you have anyone else you could suggest to do it instead?

One of my concern is that if we want user style sheet behavior I should somehow force out a PresShell::AddUserSheet for get the correct ordering, the other one is that I'm using this documentStyleSheet name for now, which probably already taken and I should find a better one.

Ochameau, ZER0: Do you guys think we need the user style or the agent style behavior? (or make that optional, or define something else)

Comment 20

[Matteo Ferretti [:zer0] [:matteo]]

for the `contentStyle` stuff, we need actually the AUTHOR_SHEET behavior, that is not implemented yet (you should be the one assigned to the bug 676054); so I would say probably that make the sheet's type optional could be a good idea. I think should work like `loadAndRegisterSheet` method of the `nsIStylesheetService` (about that: is your method Synchronous as well?).

Notice that, at least in case of AUTHOR_SHEET (see the bug) the stylesheet applied using this new method should be applied after all the document stylesheets (the author stylesheet defined in the page), even if the page added a new one dinamically after the page loading for example.

Comment 21

[Alexandre Poirot [:ochameau]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #19)
> Ochameau, ZER0: Do you guys think we need the user style or the agent style
> behavior? (or make that optional, or define something else)

+1 it would be cool to have it optional, like loadAndRegisterSheet, waiting for sheetType argument. But I can easily understand that it would bring a bunch of complexity?

Otherwise, I'm wondering if you should register these stylesheets later in this DocumentViewerImpl::CreateStyleSet method:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#2220
i.e. at the same time than nsIStyleSheetService does it.
Or may be it does not make any difference when you append/prepend?

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 644350 [details] [diff] [review]
first attempt...

I can review a final patch here, yes.

As you note, the naming is wacky.  These aren't document sheets per se.  If they're actually user sheets, maybe AddAdditionalUserSheet and similar?

If you do want user sheets, appending to the user level (as you do) makes sense.  You could maybe do it in nsDocument::FillStyleSet and then not need the extra accessor.  Have to check exactly what gets reset in nsDocument::Reset in terms of the style set...

I'm not sure under what conditions the sheet here would be inapplicable...  Maybe better to just assert that is is.

Comment 23

[Gabor Krizsanits (INACTIVE)]

Attachment: 2nd try

So I've extended the API with the sheet type arg. I had some troubles how to pull across all the layers this type info, ended up adding a new enum since the nsStyleSet::sheetType is not available in nsIDocument, and it has way more valid values than we need. It could be just a bool, but it might be very soon extended with an additional type... So this seemed to be the least bad solution.

Also added a trivial test, once I gave a binary to Alex and Metteo, and they test it against jetpack I will add more tests.

(In reply to Boris Zbarsky (:bz) from comment #22)
> Comment on attachment 644350 [details] [diff] [review]
> first attempt...
> Have to check exactly what gets reset in
> nsDocument::Reset in terms of the style set...

I think we need to reset all the additional sheets, but it's still not entirely clear when do we reset a document.

> I'm not sure under what conditions the sheet here would be inapplicable... 
> Maybe better to just assert that is is.

I did this, except one place, when they are reset. I really didn't want to duplicate that same logic 4 times there so I just added a utility function which uses IsApplicable...

So what do you think does this patch look ok? Or am I missing something? The reason why I didn't put an r? flag on this one yet is that I want to see it tested by the jetpack guys to be sure that this is what they need.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> but it's still not entirely clear when do we reset a document

Initial load and document.open, basically.

I'll try to take a look at the patch, but it might be a few days.  :(

Comment 25

[Matteo Ferretti [:zer0] [:matteo]]

So after I talked a bit with gabor, I think we should add a method to check if a stylesheet is already registered to a specific document, that is actually missing (so we could end up to register the same stylesheet for the same document twice or more times).

I wonder also if we could merge this new methods with the current `nsIStyleSheetService` methods (I also think that `loadAndRegisterSheetOnDocument` it's a really long name :) ), so basically have the method signature as follow:

void loadAndRegisterSheet(in nsIURI sheetURI, in unsigned long type, [optional] in nsIDOMDocument document);

boolean sheetRegistered(in nsIURI sheetURI, in unsigned long type, [optional] in nsIDOMDocument document);

void unregisterSheet(in nsIURI sheetURI, in unsigned long type, [optional] in nsIDOMDocument document);

Comment 26

[Boris Zbarsky [:bzbarsky]]

I'm not sure that makes sense, since there's no real "registering" going on with the service in the document != null case.

Seems like ideally this would just be a method on _document_.  Once we land the webidl bindings you'll be able to do chrome-only methods; in the meantime it might make sense to put this on windowutils.  No idea how to best do it in mozbrowser-land.

Comment 27

[Justin Lebar (not reading bugmail)]

> No idea how to best do it in mozbrowser-land.

If you wanted to call this from a remote process, you'd have to push the call information down to the child process, presumably using a new mozbrowser API method.

(That is, the caller would do frameElement.loadAndRegisterStyleSheet(...) and mozbrowser would convert that into document.loadAndRegisterStyleSheet(...).)

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 646499 [details] [diff] [review]
2nd try

There was a preexisting bug in the reset code where it nulls out the owning document on the catalog sheets but doesn't drop the sheets or reset the owning document later.  Might be worth filing as a followup.  It doesn't affect your new sheets, since you clear them on reset.

You probably want to do a style begin/end update in Load/RemoveAdditionalChildSheet around your AppendStyleSheet bits and observer notification.  Then you don't need the manual style data reconstruct.

Passing "true" as the third arg to StyleSheetAdded/Removed will break documet.styleSheets.length, I believe.  You probably want to pass false instead, with a comment explaining why.

How would a sheet in mAdditionalSheets[whatever] not have a URI?  Don't need to null-check it in RemoveAdditionalStyleSheet.  Can just assert non-null.

But most importantly, I believe this gets the ordering wrong between one of these extra sheets and a document sheet that gets added after the extra sheet.  You _may_ be able to fix that by making GetIndexOfStyleSheet return PR_INT32_MAX for these extra sheets...  You should add a test for this.

You should add a SheetTypeCount to the enum and then statically assert that your array length is the same as the sheet type count.

Comment 29

[Gabor Krizsanits (INACTIVE)]

Attachment: Offer a way to apply user agent stylesheet on a given document

Thanks for all the help, and sorry about the lag on this one, but I had to get back to some other patches first. Anyway, I've added quite some tests to check the order/priority among the different sheet types. I check these additional sheets against other author/user/agent sheets both the important and normal case, in both load order and any possible variation of these. (There is a comment in run() that explains how to interpret these tests..)

Comment 30

[Boris Zbarsky [:bzbarsky]]

Can you also post an interdiff against the previous thing I reviewed?

Comment 31

[Gabor Krizsanits (INACTIVE)]

Attachment: interdiff

Here is an interdiff between the previous version and the current version of the patch.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 656410 [details] [diff] [review]
Offer a way to apply user agent stylesheet on a given document

Thanks for the interdiff!

findSheet should be static, and have a capital 'F'.  r=me with that.

Comment 33

[Gabor Krizsanits (INACTIVE)]

https://tbpl.mozilla.org/?tree=Try&rev=0bd897bd5609

http://hg.mozilla.org/integration/mozilla-inbound/rev/4d529ec2c6ea

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4d529ec2c6ea

Comment 35

[Daniel Holbert [:dholbert]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #33)
> http://hg.mozilla.org/integration/mozilla-inbound/rev/4d529ec2c6ea

BTW: It looks like the landed cset included many whitespace-at-end-of-line fixes in unrelated chunks of the modified files. (& those whitespace fixes weren't in the r+'d patch attached to this bug)

Mostly: it's great to have those fixed, & thanks for fixing them. :) Down with bogus-whitespace!

Having said that, though: it's better to have those sorts of mega-whitespace-changes in their own entirely-whitespace-only commits.  That way, it's easier to read the actual code-changes in a commit, without the noise of the whitespace-tweaks.

I'm guessing that the whitespace-tweaks were probably added unintentionally via an hg option that you perhaps recently enabled (I seem to recall encountering some option like that in the past) -- just wanted to mention it here in case you hadn't noticed, so that you're aware that your hg configuration might be generating those extra patch-chunks.

Comment 36

[Gabor Krizsanits (INACTIVE)]

(In reply to Daniel Holbert [:dholbert] from comment #35)
> I'm guessing that the whitespace-tweaks were probably added unintentionally
> via an hg option that you perhaps recently enabled (I seem to recall
> encountering some option like that in the past) -- just wanted to mention it
> here in case you hadn't noticed, so that you're aware that your hg
> configuration might be generating those extra patch-chunks.

Exactly. So I noticed that I have all those white-space fixes all over the patch but didn't worry too much about them to as for a new review for it. Breaking the patch into two sub-patches would have been nicer indeed. Next time I will do that.

Comment 37

[Nickolay_Ponomarev]

I mentioned this on https://developer.mozilla.org/en-US/docs/Using_the_Stylesheet_Service , but https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIDOMWindowUtils still has to be updated.