Bug 22051 – renderthemes should be able to supply additional CSS rules to the core ones

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Ojan Vafai]

From #webkit:
<othermaciej> dhyatt: if we could split the UA stylesheet into core and theme-based that might be a good design
<dhyatt> othermaciej: i agree
<dhyatt> othermaciej: that's how XUL works... :)
<dhyatt> xul.css vs. theme/global.css
<dhyatt> then the rendertheme would be supplying additional rules
<dhyatt> as a String

[Matt Perry]

Created attachment 24918 [details]
initial pass at themable css sheets

I wasn't sure what to pull out of html4.css into theme-specific files, so I just kept the Windows-specific overrides for now.

[Tor Arne Vestbø]

http://trac.webkit.org/changeset/34297

[Matt Perry]

Created attachment 24973 [details]
revised patch that merges with the Qt port's approach

I modified the patch to make Qt share the same approach.  I think it's a little cleaner now.

[Matt Perry]

Sorry, ignore that patch.  I didn't realize CSSStyleSheet::parseString appended to the original, so I can do this patch a lot more simply.

[Matt Perry]

Created attachment 24974 [details]
patch that uses Qt's approach for Win

I originally thought Qt was overriding html4.css rather than appending.  Since that's not the case, the Win/Chromium port can use the same approach.  I expanded a bit on it so that:
- there's a method to add to quirks.css as well.
- the adjust* methods are virtual rather than static.

[Tor Arne Vestbø]

Good stuff. Maybe keep the platform-stylesheets in platform-subdirectories like win/qt/mac ? (The Qt one is in WebCore/platform/qt right now)

[Matt Perry]

Created attachment 25035 [details]
same as above, with .css files in platform/win

I took Tor's suggestion and moved themeWin*.css into WebCore/platform/win, instead of WebCore/css.  The rest is unchanged from the above patch.

[Darin Adler]

(In reply to comment #7)
> I took Tor's suggestion and moved themeWin*.css into WebCore/platform/win,
> instead of WebCore/css.  The rest is unchanged from the above patch.

That's not the right place. The platform directory is lower level than CSS and shouldn't contain things like this -- backwards dependencies. This should go somewhere near the themes, maybe in the rendering directory.

[Matt Perry]

Ok, if not platform/win, then should it just stay in css, alongside html4.css?  Patch #3 does that.

[Tor Arne Vestbø]

(In reply to comment #8)
> That's not the right place. The platform directory is lower level than CSS and
> shouldn't contain things like this -- backwards dependencies. This should go
> somewhere near the themes, maybe in the rendering directory.

The adjustment callback is passed the style in RenderThemeFoo::adjustDefaultStyleSheet(CSSStyleSheet* style). The fact that some themes choose to use a file to load the rules is an implementation detail private to the theme, so I would argue those files should stay close to the theme, as you say. The question then becomes, where should the themes be located? 

Currently Qt, GTK and Wx use WebCore/platform for RenderTheme and ScrollBarTheme, Mac and Win use WebCore/rendering. Should we move them all to rendering? If so in subdirectories for each platform? I have to say I like the approach of having platform independent code somehow separated out to it's easier to see what parts a port has to implement and maintain.

[David Kilzer (:ddkilzer)]

(In reply to comment #10)
> (In reply to comment #8)
> > That's not the right place. The platform directory is lower level than CSS and
> > shouldn't contain things like this -- backwards dependencies. This should go
> > somewhere near the themes, maybe in the rendering directory.
> 
> The adjustment callback is passed the style in
> RenderThemeFoo::adjustDefaultStyleSheet(CSSStyleSheet* style). The fact that
> some themes choose to use a file to load the rules is an implementation detail
> private to the theme, so I would argue those files should stay close to the
> theme, as you say. The question then becomes, where should the themes be
> located? 
> 
> Currently Qt, GTK and Wx use WebCore/platform for RenderTheme and
> ScrollBarTheme, Mac and Win use WebCore/rendering. Should we move them all to
> rendering? If so in subdirectories for each platform? I have to say I like the
> approach of having platform independent code somehow separated out to it's
> easier to see what parts a port has to implement and maintain.

This sounds like a platform layering violation (see Bug 21354).

[Tor Arne Vestbø]

(In reply to comment #11)
> This sounds like a platform layering violation (see Bug 21354).

You're right. I've sumbitted Bug 22176 to track this.

[Dave Hyatt]

Ultimately unique RenderTheme files are going to disappear.  You can see the beginnings of this with how the Mac theme is being refactored right now.

It would probably be more forward-thinking to have an API simply return a String and not use CSSStyleSheet.  Let the caller take the String and add it to the stylesheet.

[Matt Perry]

Created attachment 25103 [details]
revised patch with dhyatt's suggestion

The API now returns a String.  The default is to return just the html4.css or quirks.css string.  The Win and Qt ports override this to concatenate their specific additions.

[Maciej Stachowiak]

Repeating comments from IRC:

I think it would be better to make the API be a string of extra rules, not a string of all the rules. Otherwise, ever platform that adds extra rules is responsible for concatenating to the standard rules, but that should just be done by the core. That is sort of the approach that is taken for SVG rules as well. It's also more logical in a way - the theme doesn't own the style rules for HTML, it only owns certain theme-specific adjustments to the style rules.

I also think this is the intent of Hyatt's original suggestion: "Let the caller take the String and add it to
the stylesheet."

[Maciej Stachowiak]

Comment on attachment 25103 [details]
revised patch with dhyatt's suggestion

r- because of the above comment - however the patch looks excellent in every other way so please revise and repost. Will gladly r+ a version revised as noted above.

[Eric Seidel (no email)]

Comment on attachment 25103 [details]
revised patch with dhyatt's suggestion

It's kinda strange that the default/quirks style sheet methods mention nothing about HTML in their name, but maybe that's a good thing.

I wonder what sort of performance impact there is in allocating two string buffers to hold the user agent style sheets.  I can't remember if they get loaded for every page or not.

+   making our form elements match Firefoxes. If we find other needs for this

Gecko's or Firefox's


This sentence is not needed:
If we find other needs for this
+   file we should seriously consider if this is the right place of them. 

WebKit (for better or worse) does not use the TODO() system, so all of these should be changed to:
FIXME:
+   TODO(ojan): Figure out how to support legacy input rendering. 

Why is it themeWin instead of ThemeWin?  Seems our style-guide would suggest it should be the latter?

This comment is mostly unneeded, and contains the same mispelling:
+/* These styles override the default styling for HTML elements in quirks-mode
+   as defined in WebCore/css/quirks.css. So far we have used this file exclusively for
+   making our form elements match Firefoxes. If we find other needs for this
+   file we should seriously consider if this is the right place of them. */

It seems we should consider (long-term) fixing UserAgentStyleSheets.h to generate a stylesheeName() function which returns a String version.  Saves typing the same "String(name, sizeof(name))" code everywhere each time.

This looks fine.  If you were a commiter, I'd just ask you to make the changes as you landed, but since you aren't (yet!).  Either one of us will need to do so, or you'll need to post a new patch.  I'll volunteer to make the tiny changes and land this tomorrow when I'm back in the office, if I fail to find time before t-day, we'll go with the second-patch approach. :)

Thanks again!

[Maciej Stachowiak]

Comment on attachment 25103 [details]
revised patch with dhyatt's suggestion

Oops, meant to say r-

[Tor Arne Vestbø]

I agree with Maciej about the caller merging the rules. If you add an updated patch for that I'll take care of the details Eric noted and land it. 

Good stuff!

[Matt Perry]

I have a patch that takes Maciej's and Eric's suggestions.  I'll commit it as soon as I finish running the tests.

[Matt Perry]

Created attachment 25526 [details]
patch with maciej's+eric's suggestions

Oops - I don't have commit access just yet (it's in the pipeline).  If anyone wants to land this, here's the patch.  Otherwise, I'll do it when I get access.

[Darin Fisher (:fishd, Google)]

http://trac.webkit.org/changeset/38788

[Darin Fisher (:fishd, Google)]

Qt bustage fix:
http://trac.webkit.org/changeset/38789