179006 - API for managing user and UA stylesheets

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

At the API review meeting today, we discussed some things related to user and UA
stylesheets.  The impetus for this discussion was that Galeon is using a bunch
of internal interfaces that they shouldn't be in order to support user-specified
locations for user stylesheets.

Thus there seems to be some demand for the ability to register/unregister user
or UA stylesheets globally.  I don't completely understand the way things work
right now, and at this hour I don't think I could coherently convey what I do
understand.

However, we discussed creating a service that would manage stylesheets.  This
would be useful for:
 * embedding clients who want user stylesheet prefs that "just work" (rather
than galeon's hack of adding them for every document loaded (I think that's what
it's doing))
 * implementing some of our own RFE bugs for Mozilla (ability to specify
location of user stylesheet, ability to enable/disable user stylesheets on the
fly), perhaps by add-on components
 * perhaps cleaning up some code that does a lot of hard-coded things and stores
things in odd places
 * may come in handy for layout-extensibility work in the (very) long run

I don't fully understand even what the requirements of the code in the chrome
registry are, nevertheless how it works.  GetUserSheets seems straightforward,
and something we would probably want this API to take over.  GetStyleSheets
seems like it should remain as-is.  It's GetAgentSheets that I can't understand
at first-glance (though I haven't looked any more than I did in the middle of
the meeting).  (Is this implementing some stylesheet mechanism from
contents.rdf?)  hyatt -- any suggestions on how you think these things (this
issue specifically, and the whole thing) should work?

We probably also should take a more careful look at what galeon is doing.  Does
it allow on-the-fly disabling of user sheets?

I'll attach a sketch of my memory of what the discussion during the meeting led
to (in pseudo-IDL), with a few little changes I've made since, and a bunch of
comments/questions.

I think jjmata (maybe others too?) has some additional notes from the meeting.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: interface sketch, i.e., what dbaron remembered from the meeting

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Accepting bug for now, although I suspect I won't be the one to end up doing
the work, since others know this code better.)

Comment 3

[Boris Zbarsky [:bzbarsky]]

One other option is something that has two methods (just user sheets for the
moment; the UA sheet issue is similar):

1)  registerUserSheet(nsIURI)
2)  registerProvider(nsIUserSheetProvider)

Then when we need to build the style set for a page, we ask all our providers
for sheets (passing them the URI of the page, eg) and union the lists and apply.
 The providers return URIs; the sheets are only applied if they have been
previously registered.

(this still needs a unregisterUserSheet, etc, but I'm just testing the overall
idea).

The point is that the logic for which user sheets apply to each page is
completely handled by the embedding app.  Gecko just asks.

Comment 4

[Benjamin Smedberg]

Random thoughts:

What kind of documents is this for?
Should non-CSS style work (XSLT) (in a theoretical world, of course)?

Comment 5

[Hixie (not reading bugmail)]

The concept of user stylesheet and user agent stylesheet is pretty specific to 
CSS, I think, no? Can you usefully cascade XSLT sheets?

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

See also
http://groups.google.com/groups?selm=slrncgbquu.ubg.philipl%40fido2.homeip.net
and the thread following.

Comment 7

[Philip Langdale]

So, dbaron motivated me sufficiently to start thinking about how to go about
implementing this.

nsStyleSet seems to implement logic to correctly handle a stylesheet passed as
type userStyle, though I might be wrong. If this is indeed the case, then it
becomes a matter of the right way to register user stylesheets so they can be
applied. It's already been stated that nsIPresShell is not a recommended way to
publicly expose such a facility but should it be the way that a user stylesheet
service would apply user styles? in which case, we'd want to add comparable
methods as for Override stylesheets. 

As for the higher level public interface, the behaviour we currently have is to
offer is to let the user pick from available styles and then apply them to the
document. bz's options work the other way round, with gecko asking us what
styles to apply. This makes sense once a given set of styles have been selected
for a page and we've stored that selection somewhere (say, our history, as
mentioned) but it seems a tad circuitous for when the user changing the style
set. It seems we'd have to force a page reload so that it would request the
updated set - but perhaps that isn't too unreasonable.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> nsStyleSet seems to implement logic to correctly handle a stylesheet

It does, yes.

> but it seems a tad circuitous for when the user changing the style set

Indeed.  For that case we would need to either notify gecko to ask for the new
sheets or to tell the page to use a particular new set (depending on which one
of the approaches I suggested we take).  The public api for this could hang off
the content viewer, maybe?

Comment 9

[Brian Ryner (not reading)]

Probably need this for xforms as well, to add default style rules for xforms
controls.

Comment 10

[Brian Ryner (not reading)]

Attachment: rough patch

This is pretty minimal but it's really all I need for XForms (I don't really
even need on-the-fly registration but it was pretty trivial to provide).  With
this patch, all registered sheets apply all the time.

Comment 11

[Benjamin Smedberg]

Comment on attachment 158220 [details] [diff] [review]
rough patch

Hey, this is great. One note is that the enumerator from
catMan->EnumerateCategory can be QIed to a nsIUTF8StringEnumerator, which might
help make RegisterFromEnumerator() a little less verbose.

Comment 12

[Philip Langdale]

Looks good.

Stylesheet service seems great to me but the application mechanism seems
mutually exclusive with any sort of selective application of stylesheets.

Of course, I'm not the one writing any code right now. *sigh*

Comment 13

[Boris Zbarsky [:bzbarsky]]

I'm ok with reviewing this if we all sort of agree that we're not planning to do
a per-URI solution of some sort.  If we _are_, I'd rather not spend cycles (both
mine and more of Brian's) on something we plan to redo anyway... (though if we
need this for XForms "right now" I suppose we can get it in while we discuss the
api we actually want... any api we end up with will support the functionality
this one does).

Philip?  David?

Comment 14

[Philip Langdale]

Well, I know that we will definitely want a per-URI mechanism for galeon, but
bryner's patch is very minimal, so if it's needed now, I don't think it's worth
holding up while the full API is discussed. And it certainly lays the groundwork
by introducing the notion of a stylesheet service that the document viewer talks
to. Rather than replacing this, I'd expect we could extend it so that the
stylesheet service would be queried with the loaded uri to return the relevant
stylesheets.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 158220 [details] [diff] [review]
rough patch

This seems to be missing the module changes to register the contractid, etc.

I think I'd prefer USER_SHEET and AGENT_SHEET to SHEET_USER and SHEET_AGENT.

>Index: src/nsDocumentViewer.cpp

>+  nsCOMPtr<nsIStyleSheetService> dummy =
>+    do_GetService(NS_STYLESHEETSERVICE_CONTRACTID);
>+
>+  nsStyleSheetService *sheetService = nsStyleSheetService::gInstance;

I sort of wonder whether this would not be better as an nsContentUtils thing,
but I guess this is OK.

>Index: src/nsStyleSheetService.cpp

>+nsStyleSheetService::RegisterSheet(nsIURI *aSheetURI, PRUint32 aSheetType)
>+{
>+  nsCOMPtr<nsICSSLoader> loader = do_GetService(kCSSLoaderCID);

CSSLoaders are not services... this will end up asserting when people
(correctly) use createInstance() to create CSSLoaders.

This function should assert on unknown values of aSheetType.

>+  nsresult rv = loader->LoadAgentSheet(aSheetURI, getter_AddRefs(sheet));

And somehow the API needs to make it clear that the sheets will be loaded
synchronously at registration time (which means that passing in http:// URIs
and the like has certain perils).

Comment 16

[Brian Ryner (not reading)]

Attachment: revised patch

I think this takes care of bz's comments.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 170617 [details] [diff] [review]
revised patch

>Index: base/nsIStyleSheetService.idl
>+   * Synchrnously loads a style sheet from |sheetURI| and adds it 

"Synchronously"

>Index: base/nsStyleSheetService.cpp
>+nsStyleSheetService::LoadAndRegisterSheet(nsIURI *aSheetURI,
>+                                          PRUint32 aSheetType)
>+{
>+  nsCOMPtr<nsICSSLoader> loader = do_CreateInstance(kCSSLoaderCID);

This is OK, but you could also just have an nsICSSLoader member in this
service, and create it in Init(), no?

r+sr=bzbarsky with those nits fixed.

Comment 18

[Brian Ryner (not reading)]

checked in