GNOME Bugzilla – Bug 633462
support scaling of the background (background-size)
Last modified: 2012-02-06 13:02:08 UTC
CSS3 proposes a background-size property - http://www.w3.org/TR/2010/WD-css3-background-20100612/#the-background-size Currently the background implementation scales down the background image with appropriate aspect ratio if it doesn't fit into the block. I am looking for a non-aspect ratio locked background image for creating the .panel-button:active state. Ie it would need to be in absolute units for the height and 100% for the width (eg "16px 100%"). For the aspect ratio scale, 'cover' isn't as crucial as 'contain' I'd say.
Patch submitted in bug 624383
Created attachment 178447 [details] [review] Patch to begin implementation of background-size, need some work for fixed size Here is the splitted patch (first patch to apply)
Created attachment 178714 [details] [review] Patch with full background scaling scaling This patch now allow to scale the background a a fixed size, if two values, then final width and height of the background are set, if only one, it's the width with auto-scaling for the height. Still support "contain" keyword for the best-to-fit scaling.
Review of attachment 178714 [details] [review]: Sardem, do you still want to work on this? ::: src/st/st-theme-node-drawing.c @@ +371,2 @@ { + gfloat s = 1; I think spelling out "scale" isn't too harmful. @@ +376,3 @@ + s = result->x2 / w; + else if (h > result->y2) + s = result->y2 / h; I think: gint scale = MIN ((result->x2 / w), (result->y2 / h)); /* Only scale down, otherwise we already fit. */ if (scale_ratio < 1) { w *= scale; h *= scale; } would be a bit clearer. Additionally, can we be sure that h and w aren't 0? @@ +387,3 @@ + sy = sx = self->background_size_w / w; + if ( self->background_size_h > 0 ) + sy = self->background_size_h / h; sy can go unused here.
Created attachment 191651 [details] [review] Patch with full background scaling Sure I want. Here is the real, rebased and reworked patch to review.
Review of attachment 191651 [details] [review]: Watch your code style: int foo (int a, int b, int c) { if (a == b) b = c * 2; return a + b + c; } foo (a, b, c); ::: src/st/st-theme-node-drawing.c @@ +355,2 @@ static void +get_background_size(StThemeNode *node, I would like better variable names. How about "background_width", "background_height", "container_width" and "container_height". @@ +400,3 @@ + { + /* center the background on the widget */ + *x = (int)((tw / 2) - (w / 2)); Why the (int) cast? @@ +410,3 @@ ClutterActorBox *result) { + gdouble bw, bh, w, h, x, y, sx, sy; I'd like better variable names here. @@ +416,2 @@ + w = allocation->x2 - allocation->x1; + h = allocation->y2 - allocation->y1; A comment explaining that this makes background positioning area to be the equivalent of the CSS3 "padding box" would be helpful. @@ +528,3 @@ pattern = cairo_pattern_create_for_surface (surface); + gdouble w, h; ANSI C. All variable declarations need to go at the start of a block. @@ +537,1 @@ + gdouble sw, sh; ANSI C. @@ +551,1 @@ + gdouble x, y; ANSI C. ::: src/st/st-theme-node.c @@ +1675,2 @@ } + else /* We don't support other values, no resize if not contain */ I think COVER wouldn't be too hard to add and would be very useful. ::: src/st/st-types.h @@ +52,3 @@ +typedef enum { + ST_BACKGROUND_SIZE_NONE, + ST_BACKGROUND_SIZE_CONTAIN, I think "ST_BACKGROUND_SIZE_AUTO" would be a bit better.
Created attachment 192531 [details] [review] Patch with full background scaling (In reply to comment #6) > @@ +416,2 @@ > + w = allocation->x2 - allocation->x1; > + h = allocation->y2 - allocation->y1; > > A comment explaining that this makes background positioning area to be the > equivalent of the CSS3 "padding box" would be helpful. I'm not sure what you mean, this one has nothing to do with CSS, it's just a matter of having the correct size of the background image. Others reviews are applied, I think.
(In reply to comment #7) > Created an attachment (id=192531) [details] [review] > Patch with full background scaling > > (In reply to comment #6) > > @@ +416,2 @@ > > + w = allocation->x2 - allocation->x1; > > + h = allocation->y2 - allocation->y1; > > > > A comment explaining that this makes background positioning area to be the > > equivalent of the CSS3 "padding box" would be helpful. > > I'm not sure what you mean, this one has nothing to do with CSS, it's just a > matter of having the correct size of the background image. Sure it does. CSS3 provides a concept called the "background position area" that is specified with the "background-origin" property. This determines how "background-size" and other properties are interpreted. With an origin of "padding-box", it is sized to the entire box, including padding, but not border and margin. I don't believe we include the border in the allocation, but if we do, it would be "border-box" instead. See: See http://www.w3.org/TR/2011/CR-css3-background-20110215/#the-background-origin
(In reply to comment #8) > Sure it does. CSS3 provides a concept called the "background position area" > that is specified with the "background-origin" property. This determines how > "background-size" and other properties are interpreted. With an origin of > "padding-box", it is sized to the entire box, including padding, but not border > and margin. I don't believe we include the border in the allocation, but if we > do, it would be "border-box" instead. See: > > See > http://www.w3.org/TR/2011/CR-css3-background-20110215/#the-background-origin I know that, but such a comment would be better where the box is allocated. Here I only use the box to get the proper size. Correct me if I'm thinking wrong. Other comments on the code? I'm waiting for this patch to be stable to work on the others as they are based on it.
Review of attachment 192531 [details] [review]: OK, it looks like the fixed size handling for one dimension is incorrect. From the W3: "An ‘auto’ value for one dimension is resolved by using the image's intrinsic ratio and the size of the other dimension, or failing that, using the image's intrinsic size, or failing that, treating it as 100%." You seem to assign height 0, which looks like it will just keep the default of scaling by 1.0.
(In reply to comment #10) > Review of attachment 192531 [details] [review]: > > OK, it looks like the fixed size handling for one dimension is incorrect. From > the W3: > > "An ‘auto’ value for one dimension is resolved by using the image's intrinsic > ratio and the size of the other dimension, or failing that, using the image's > intrinsic size, or failing that, treating it as 100%." > > You seem to assign height 0, which looks like it will just keep the default of > scaling by 1.0. I set 0 to use the same scale factor. *scale_y = *scale_x = node->background_size_w / current_width; if ( node->background_size_h > 0 ) *scale_y = node->background_size_h / current_height; Note the "*scale_y = *scale_x". The case that is not handled is "auto 15px" because I don't check a size after the "auto". I'm working on it.
Created attachment 192536 [details] [review] Patch with full background scaling Here it is.
Review of attachment 192536 [details] [review]: ::: src/st/st-theme-node-drawing.c @@ +435,3 @@ + break; + case ST_BACKGROUND_SIZE_FIXED: + if ( node->background_size_w > 0 ) Take all the whitespace around parens out. ::: src/st/st-theme-node.c @@ +1680,3 @@ + if (decl->value->next) + { + result = get_length_from_term_int (node, decl->value->next, FALSE, &node->background_size_h); What happens if this term is not a number, but is "auto"?
(In reply to comment #13) > Review of attachment 192536 [details] [review]: > > ::: src/st/st-theme-node-drawing.c > @@ +435,3 @@ > + break; > + case ST_BACKGROUND_SIZE_FIXED: > + if ( node->background_size_w > 0 ) > > Take all the whitespace around parens out. I don't understand. A little sample ? > ::: src/st/st-theme-node.c > @@ +1680,3 @@ > + if (decl->value->next) > + { > + result = get_length_from_term_int (node, decl->value->next, > FALSE, &node->background_size_h); > > What happens if this term is not a number, but is "auto"? get_length_from_term_int returns VALUE_NOT_FOUND then I fallback to auto. I can add a pre-check, if you want.
(In reply to comment #14) > (In reply to comment #13) > > Review of attachment 192536 [details] [review] [details]: > > > > ::: src/st/st-theme-node-drawing.c > > @@ +435,3 @@ > > + break; > > + case ST_BACKGROUND_SIZE_FIXED: > > + if ( node->background_size_w > 0 ) > > > > Take all the whitespace around parens out. > > I don't understand. A little sample ? if (node->backround_size_w > 0) > > ::: src/st/st-theme-node.c > > @@ +1680,3 @@ > > + if (decl->value->next) > > + { > > + result = get_length_from_term_int (node, decl->value->next, > > FALSE, &node->background_size_h); > > > > What happens if this term is not a number, but is "auto"? > > get_length_from_term_int returns VALUE_NOT_FOUND then I fallback to auto. I can > add a pre-check, if you want. Only after a g_warning, though. A pre-check would be preferred.
Created attachment 192538 [details] [review] Patch with full background scaling (In reply to comment #15) > if (node->backround_size_w > 0) I must improve my english. > Only after a g_warning, though. A pre-check would be preferred. Added. At the other place too (when the first term is a number).
Review of attachment 192538 [details] [review]: ::: src/st/st-theme-node-drawing.c @@ +420,3 @@ + case ST_BACKGROUND_SIZE_CONTAIN: + if ((current_width > target_width) && (current_height > target_height)) + *scale_x = *scale_y = (target_width <= target_height) ? ( target_width / current_width ) : ( target_height / current_height ); Again, whitespace and parens. @@ +428,3 @@ + case ST_BACKGROUND_SIZE_COVER: + if ((current_width < target_width) && (current_height < target_height)) + *scale_x = *scale_y = (target_width >= target_height) ? ( target_width / current_width ) : ( target_height / current_height ); And here. @@ +609,3 @@ + get_background_scale (node, node->alloc_width, node->alloc_height, w, h, &scale_w, &scale_h); + if ((scale_w != 1) || (scale_h != 1)) + cairo_matrix_scale (&matrix, scale_w, scale_h); This code is doing nothing since you init_translate below. @@ +616,3 @@ + * area, then don't bother doing a background fill first + * + if (content != CAIRO_CONTENT_COLOR_ALPHA && width_ratio == height_ratio) Do not leave commented out code in the file. ::: src/st/st-theme-node.c @@ +1603,3 @@ node->background_image = NULL; node->background_position_set = FALSE; + node->background_size = ST_BACKGROUND_SIZE_AUTO; You don't need to do this. @@ -1641,3 @@ if (result == VALUE_NOT_FOUND) - { - node->background_position_set = FALSE; These changes to background_position_set are unrelated. Scrap them.
Created attachment 192550 [details] [review] Patch with full background scaling (In reply to comment #17) > Review of attachment 192538 [details] [review]: > > ::: src/st/st-theme-node-drawing.c > @@ +420,3 @@ > + case ST_BACKGROUND_SIZE_CONTAIN: > + if ((current_width > target_width) && (current_height > > target_height)) > + *scale_x = *scale_y = (target_width <= target_height) ? ( > target_width / current_width ) : ( target_height / current_height ); > > Again, whitespace and parens. > > @@ +428,3 @@ > + case ST_BACKGROUND_SIZE_COVER: > + if ((current_width < target_width) && (current_height < > target_height)) > + *scale_x = *scale_y = (target_width >= target_height) ? ( > target_width / current_width ) : ( target_height / current_height ); > > And here. Done. > @@ +609,3 @@ > + get_background_scale (node, node->alloc_width, node->alloc_height, w, h, > &scale_w, &scale_h); > + if ((scale_w != 1) || (scale_h != 1)) > + cairo_matrix_scale (&matrix, scale_w, scale_h); > > This code is doing nothing since you init_translate below. Added init_identity then use translate. > @@ +616,3 @@ > + * area, then don't bother doing a background fill first > + * > + if (content != CAIRO_CONTENT_COLOR_ALPHA && width_ratio == height_ratio) > > Do not leave commented out code in the file. I think the other check does the job so I updated its comment and get rid of this code. > ::: src/st/st-theme-node.c > @@ +1603,3 @@ > node->background_image = NULL; > node->background_position_set = FALSE; > + node->background_size = ST_BACKGROUND_SIZE_AUTO; > > You don't need to do this. The 'background' property has to reset all background properties to the default. > @@ -1641,3 @@ > if (result == VALUE_NOT_FOUND) > - { > - node->background_position_set = FALSE; > > These changes to background_position_set are unrelated. Scrap them. Leftover of the big unsplitted patch. Scrapped.
Review of attachment 192550 [details] [review]: Given that you now do something different because of the translate/scale, can you add some tests and make sure that everything still works? Other code review comments: I'd like to make the concept of the background positioning area more explicit in st-theme-node-drawing.c. Additionally, I'm going to take some to make your commit message a bit more of proper English: St: Implement background-size CSS property Implement the background-size CSS property, specified by the CSS Backgrounds and Borders Module Level 3, including the keywords "contain", "cover", and fixed-size backgrounds. ::: data/theme/gnome-shell.css @@ +5,3 @@ * + * Copyright 2011 Quentin "Sardem FF7" Glidic + * Adapt to the implementation of background-size While I don't want to shoot your contribution down, I'm going to go out and say that I don't think the copyright line here is appropriate, given that a lot of other people worked on it and never stuck their name in the header. ::: src/st/st-theme-node-drawing.c @@ +404,3 @@ static void +get_background_scale (StThemeNode *node, + gdouble target_width, "target_width" seems like a bad variable name to me. The "target" is just the image's dimensions. So use "background_width" for consistency. @@ +406,3 @@ + gdouble target_width, + gdouble target_height, + gdouble current_width, "current_width" is the widget's width. "widget_width" seems more appropriate. @@ +435,3 @@ + break; + case ST_BACKGROUND_SIZE_FIXED: + if (node->background_size_w > 0) For spec compliance, "background-size: 0;" is valid per W3, so your code will break here. Not quite practical, so I'm not afraid, as long as your code doesn't crash on something an input like that, but worth noting. @@ +497,1 @@ + get_background_coordinates (self, (allocation->x2 - allocation->x1), (allocation->y2 - allocation->y1), w, h, &x, &y); Maybe it's my fault, but I don't understand this code, because you don't pass background_width/background_height, just the original allocation area and the scaled allocation area. If that's what you're trying to do, a code comment explaining why would be nice. ::: src/st/st-theme-node.c @@ +1675,3 @@ + node->background_size = (result == VALUE_FOUND) ? ST_BACKGROUND_SIZE_FIXED : ST_BACKGROUND_SIZE_AUTO; + } + else /* not a correct case */ This is an incorrect comment now that it falls back to this case for completely correct parameters.
Created attachment 192584 [details] [review] Patch with full background scaling (I don’t have a test environment to try this patch right now, please forgive me if it does not compile or work.) (In reply to comment #19) > Given that you now do something different because of the translate/scale, can > you add some tests and make sure that everything still works? I’m a better code-styler than test-writer. I will try… > Other code review comments: I'd like to make the concept of the background > positioning area more explicit in st-theme-node-drawing.c. Another patch in my stack? Before this one, I guess? > Additionally, I'm going to take some to make your commit message a bit more of > proper English: > > St: Implement background-size CSS property > > Implement the background-size CSS property, specified by the CSS > Backgrounds and Borders Module Level 3, including the keywords "contain", > "cover", and fixed-size backgrounds. You’re a better English writer than me. > ::: src/st/st-theme-node-drawing.c > @@ +404,3 @@ > static void > +get_background_scale (StThemeNode *node, > + gdouble target_width, > > "target_width" seems like a bad variable name to me. The "target" is just the > image's dimensions. So use "background_width" for consistency. > > @@ +406,3 @@ > + gdouble target_width, > + gdouble target_height, > + gdouble current_width, > > "current_width" is the widget's width. "widget_width" seems more appropriate. If you think so… > @@ +435,3 @@ > + break; > + case ST_BACKGROUND_SIZE_FIXED: > + if (node->background_size_w > 0) > > For spec compliance, "background-size: 0;" is valid per W3, so your code will > break here. > > Not quite practical, so I'm not afraid, as long as your code doesn't crash on > something an input like that, but worth noting. I’ll then use -1. > @@ +497,1 @@ > + get_background_coordinates (self, (allocation->x2 - allocation->x1), > (allocation->y2 - allocation->y1), w, h, &x, &y); > > Maybe it's my fault, but I don't understand this code, because you don't pass > background_width/background_height, just the original allocation area and the > scaled allocation area. If that's what you're trying to do, a code comment > explaining why would be nice. I’m not sure, I’ll assume you’re right… It was a "long" time ago I started this patch…
OK, I'm going to defer reviewing until you can test the patch. Additionally, look at the JS files in tests/interactive/ and see if you can write something similar.
Created attachment 198466 [details] [review] Patch with full background scaling Rebased patch. Some y-misalignment is still here. Seems to work fine on most of the current backgrounds.
Review of attachment 198466 [details] [review]: ::: src/st/st-theme-node-drawing.c @@ +611,3 @@ + */ + if (content != CAIRO_CONTENT_COLOR_ALPHA + && -x <= 0 x >= 0 @@ +614,3 @@ + && -x + w >= node->alloc_width + && -x <= 0 + && -x + h >= node->alloc_height) uh, did you mean to use y here?
Created attachment 198744 [details] [review] Patch with full background scaling A few renames for semantic, just let me know if they seem wrong. Added tests. Fixed the cairo_matrix stuff (1/scale and -x, -y). To match the CSS3 spec, "contain" and "cover" now force the scale of the image to fit the container. gnome-shell.css part needs "real life" test.
Created attachment 198771 [details] [review] Patch with full background scaling The same with a few coding style fixes.
Created attachment 198772 [details] [review] Patch with full background scaling Here is the good one
Review of attachment 198772 [details] [review]: Patch does not apply on master.
Created attachment 201554 [details] [review] Patch with full background scaling Rebased, and complete this time
Created attachment 204147 [details] [review] Patch with full background scaling Fixed the auto issue (flipping).
Review of attachment 201554 [details] [review]: Yep. Do you need me to push?
(In reply to comment #30) > Yep. Do you need me to push? Yes, please.
Comment on attachment 204147 [details] [review] Patch with full background scaling Pushed with modification to CSS file - calendar arrows were incorrectly modified to be "contain".
Anything left to do or can this be closed?
Created attachment 206616 [details] [review] theme: More fallout from background-size The app filter arrows and scroll handles should be at 100%, not mapped to their container
Created attachment 206617 [details] [review] st-theme-node-drawing: Fix implementation of background-size It seems that accidentally, two variables were swapped in one code path of the background-size implementation, causing interesting but wrong images for some elements.
Created attachment 206618 [details] [review] st-theme-node-drawing: Remove possible subtexturing Since our implementation of background-size is now CSS-compliant, we do not need this subtexture hack that clips a "leak". The comment here is also incorrect.
Review of attachment 206616 [details] [review]: Looks good.
Review of attachment 206617 [details] [review]: Looks good.
Review of attachment 206618 [details] [review]: Yeah makes sense.
Attachment 206616 [details] pushed as fad88dd - theme: More fallout from background-size Attachment 206617 [details] pushed as 5a3de8d - st-theme-node-drawing: Fix implementation of background-size Attachment 206618 [details] pushed as 5c730dc - st-theme-node-drawing: Remove possible subtexturing