GNOME Bugzilla – Bug 663522
css: start background-repeat
Last modified: 2011-11-08 17:44:42 UTC
By default, a background image is stretched. Instead, it is worth to have a tiled background. This patch allows background surfaces to be repeated or not, and should be compatible with future extensions and CSS.
Created attachment 200843 [details] [review] css: start background-repeat
Review of attachment 200843 [details] [review]: Hey Marc, this looks nice, thanks! In GTK we try to stick to standard CSS semantics where possible; I realize in this specific case, it might lead to opening a big can of worms though, since you don't want to implement all the various background-repeat states right away :) Even if we don't, a couple of things need a bit more thinking though: The CSS background-repeat default value as per spec is 'repeat' (i.e. repeat pattern on both sides). Until now, in GTK, we always streched background images to fit all the available space, so this would be somewhat a behavioural change - note that CSS has a lot more options to control the size/positioning of background tiles, all missing in GTK, so you can effectively have a "repeat" image being stretched (e.g. if you use { background-repeat: repeat; background-size: 100%; } or { background-repeat: repeat; background-size: cover; }). I don't think it's particularly bad to leave things as is and keep stretching all backgrounds by default for now (until at least we add more background properties), but we should be careful documenting when GTK's default behaviour differs from CSS. I also inlined some comments below. ::: gtk/gtkcsstypesprivate.h @@ +30,3 @@ + GTK_CSS_BACKGROUND_REPEAT_STYLE_NO_REPEAT, +} GtkCssBackgroundRepeatStyle; + I'm wondering whether this really needs to be an additional type or if we can reuse the GtkCssRepeatStyle enum for it (making sure any background-repeat only hint is discarded by border-image-repeat and viceversa). That way you could also avoid some duplicate parsing code in GtkCssProvider. If you think two different types are needed, I'd rather you renamed GtkCssRepeatStyle to GtkCssBorderImageRepeatStyle before doing this. @@ +46,3 @@ +struct _GtkCssBackgroundRepeat { + GtkCssBackgroundRepeatStyle repeat; +}; There should be a FIXME comment here indicating we miss separate X/Y fields for this. ::: gtk/gtkstylecontext.h @@ +154,3 @@ + * A property holding the element's background as a #cairo_pattern_t. + */ +#define GTK_STYLE_PROPERTY_BACKGROUND_REPEAT "background-repeat" This is not needed; since background-repeat is held within a private type, out of tree engines won't be able to do gtk_theming_engine_get (engine, state, "background-repeat", &repeat, NULL); which is the point of having these defines. ::: gtk/gtkstyleproperty.c @@ +1140,3 @@ + style = GTK_CSS_BACKGROUND_REPEAT_STYLE_NO_REPEAT; + else + style = GTK_CSS_REPEAT_STYLE_NONE; Should be GTK_CSS_BACKGROUND_REPEAT_STYLE_NONE, unless we unify GtkCssRepeatStyle and GtkCssBackgroundRepeatStyle.
Hi Cosimo, thanks for the review (In reply to comment #2) > ... > I don't think it's particularly bad to leave things as is and keep stretching > all backgrounds by default for now (until at least we add more background > properties), but we should be careful documenting when GTK's default behaviour > differs from CSS. Agreed all above. Is there such a place where it should be documented? (btw, I formed an opinion about the CSS styling doc in GTK. It would deserve its own document, and not a large, hard to read or modify, and limited gtk-doc) > ::: gtk/gtkcsstypesprivate.h > @@ +30,3 @@ > + GTK_CSS_BACKGROUND_REPEAT_STYLE_NO_REPEAT, > +} GtkCssBackgroundRepeatStyle; > + > > I'm wondering whether this really needs to be an additional type or if we can > reuse the GtkCssRepeatStyle enum for it (making sure any background-repeat only > hint is discarded by border-image-repeat and viceversa). That way you could > also avoid some duplicate parsing code in GtkCssProvider. > > If you think two different types are needed, I'd rather you renamed > GtkCssRepeatStyle to GtkCssBorderImageRepeatStyle before doing this. I think two different types are safer, since border and background have different values, but I don't have strong opinion (and I even attempted to ask you about it on irc earlier ;) I will rename s/GtkCssRepeatStyle/GtkCssBorderImageRepeatStyle > > @@ +46,3 @@ > +struct _GtkCssBackgroundRepeat { > + GtkCssBackgroundRepeatStyle repeat; > +}; > > There should be a FIXME comment here indicating we miss separate X/Y fields for > this. ok > > ::: gtk/gtkstylecontext.h > @@ +154,3 @@ > + * A property holding the element's background as a #cairo_pattern_t. > + */ > +#define GTK_STYLE_PROPERTY_BACKGROUND_REPEAT "background-repeat" > > This is not needed; since background-repeat is held within a private type, out > of tree engines won't be able to do > > gtk_theming_engine_get (engine, state, > "background-repeat", &repeat, NULL); > > which is the point of having these defines. ok (btw, what makes it a private type?) > ::: gtk/gtkstyleproperty.c > @@ +1140,3 @@ > + style = GTK_CSS_BACKGROUND_REPEAT_STYLE_NO_REPEAT; > + else > + style = GTK_CSS_REPEAT_STYLE_NONE; > > Should be GTK_CSS_BACKGROUND_REPEAT_STYLE_NONE, unless we unify > GtkCssRepeatStyle and GtkCssBackgroundRepeatStyle. ack thanks
(In reply to comment #3) > Agreed all above. Is there such a place where it should be documented? Current best place for this would be the doc section of gtkcssprovider.c. > (btw, I formed an opinion about the CSS styling doc in GTK. It would deserve > its own document, and not a large, hard to read or modify, and limited gtk-doc) Yeah, it's definitely not ideal and could be improved a lot. > I think two different types are safer, since border and background have > different values, but I don't have strong opinion (and I even attempted to ask > you about it on irc earlier ;) > > I will rename s/GtkCssRepeatStyle/GtkCssBorderImageRepeatStyle Okay, sounds good to me. Since those are private we're free to refactor the code in the future if more sharing between the border and background rendering implementations become necessary. > > This is not needed; since background-repeat is held within a private type, out > > of tree engines won't be able to do > > > > gtk_theming_engine_get (engine, state, > > "background-repeat", &repeat, NULL); > > > > which is the point of having these defines. > > ok (btw, what makes it a private type?) gtkcsstypesprivate.h is not installed, so every type defined in there is private. Note that if we (er...you actually! :P) wanted to implement the other repeat styles, some of the code in gtkborderimage.c (in particular gtk_border_image_render_slice()) could probably be re-used. One last thing: the CSS theming engine code in GTK has a test suite based on "reftests" (found in the tests/reftests subdirectory of the GTK repo): basically you put together two small GtkBuilder UI hierarchies - a reference one and one for the test - a CSS file, and a script will generate PNG images of the rendered output, comparing them for differences. We try to keep the code tested this way every time we introduce a new property, so it would be nice to have a reftest for background-repeat to go along with your patch (e.g. you could have the reference use background-color: red and the test use a 1px fully red background-image with background-repeat set, and verify the output match).
Created attachment 200945 [details] [review] Rename s/GtkCssRepeatStyle/GtkCssBorderRepeatStyle/g
Created attachment 200946 [details] [review] css: start background-repeat By default, a background image is stretched. Instead, it is worth to have a tiled background. This patch allows background surfaces to be repeated or not, and should be compatible with future extensions and CSS.
Created attachment 200947 [details] [review] RFC: Add a background-repeat css reftest that fails The test should probably pass, but fails for various reasons. Should we track down the problems?
Review of attachment 200945 [details] [review]: Looks good.
Review of attachment 200946 [details] [review]: Looks good.
(In reply to comment #7) > The test should probably pass, but fails for various reasons. > Should we track down the problems? Yeah, tests we commit should pass as they block distcheck IIRC...what kind of failure do you see in the test? (You could also start with something more basic like setting background-color of the toplevel window to an uniform color and verify that setting an 1px repeat background-image of the same color gives the same result, and improve from there)
Attachment 200945 [details] pushed as ee7ac4f - Rename s/GtkCssRepeatStyle/GtkCssBorderRepeatStyle/g Attachment 200946 [details] pushed as 3b436ee - css: start background-repeat I felt free to improve the test and fix a little bug in render_background_internal(), and push all this to master. Thanks!