GNOME Bugzilla – Bug 624375
background-position doesn't seem to be supported.
Last modified: 2010-09-01 16:14:26 UTC
Created attachment 165907 [details] according to the stylesheet, the icon should have been placed on the top left corner. Key property for CSS styling is to be able to position the background image on the block. As the attached screenshot shows, this doesn't seem to be implemented: #searchEntry { padding: 4px; border-radius: 4px; color: #444; border: 1px solid #565656; background-gradient-direction: vertical; background-gradient-start: #ccc; background-gradient-end: #eee; background-image: url(search.svg); background-repeat: no-repeat; background-position: 0 0; /* doesn't seem to be supported */ caret-color: #fff; caret-size: 1px; height: 16px; padding-left: 20px; } See http://www.w3.org/TR/CSS2/colors.html#propdef-background-position
Created attachment 166133 [details] [review] [StThemeNode] Add basic background-position support Add basic support for background-position, which only supports absoulte values. Also don't require an unit to be specified for 0 (because the unit does not really matter here 0 is 0 regardless of the unit).
Created attachment 166134 [details] [review] [StThemeNode] Add basic background-position support Add basic support for background-position, which only supports absoulte values. Also don't require an unit to be specified for 0 (because the unit does not really matter here 0 is 0 regardless of the unit).
Comment on attachment 166134 [details] [review] [StThemeNode] Add basic background-position support >Add basic support for background-position, which only supports absoulte typo in "absolute" > if (new_h > box_h) > { >- /* center for new width */ >- offset = ((box_w) - new_w) * 0.5; >+ /* honor the specified position if any */ I'm not sure how background-image scaling and positioning interact in HTML, but this can't be right (the spec doesn't seem to discuss scaling at all, implying that maybe background-image is never scaled-down in HTML?). In particular, your code ends up using either the specified x position or the specified y position, but not both. >+ if (self->background_position[0] != -1) You're allowed to specify negative positions (to offset the image in the opposite direction), so you can't use -1 as a sentinel. So you'll need an explicit "background_position_set" flag or something. It would also be better to have separate background_position_x and background_position_y fields (like we do with the various height/width properties) rather than using an array (which we generally only do for properties where there's a corresponding enumeration, eg StSide or StCorner). Also, for all of the other properties, we convert to int when parsing the CSS. I'm not sure background-position needs to be any different.
(In reply to comment #3) > (From update of attachment 166134 [details] [review]) > >Add basic support for background-position, which only supports absoulte > > typo in "absolute" ... OK > > if (new_h > box_h) > > { > >- /* center for new width */ > >- offset = ((box_w) - new_w) * 0.5; > >+ /* honor the specified position if any */ > > I'm not sure how background-image scaling and positioning interact in HTML, but > this can't be right (the spec doesn't seem to discuss scaling at all, implying > that maybe background-image is never scaled-down in HTML?). It isn't ... its will just get "cut off" if it doesn't fit (at least pretty much any browser I know of does it this way; not sure which spec specifies that if any) > In particular, your > code ends up using either the specified x position or the specified y position, > but not both. That wasn't intentional, that's a stupid bug ... > >+ if (self->background_position[0] != -1) > > You're allowed to specify negative positions (to offset the image in the > opposite direction), so you can't use -1 as a sentinel. So you'll need an > explicit "background_position_set" flag or something. Indeed, good point. > It would also be better to have separate background_position_x and > background_position_y fields (like we do with the various height/width > properties) rather than using an array (which we generally only do for > properties where there's a corresponding enumeration, eg StSide or StCorner). OK > Also, for all of the other properties, we convert to int when parsing the CSS. > I'm not sure background-position needs to be any different. OK, wasn't aware of that, but yeah we should be consistent here.
Created attachment 168940 [details] [review] [StThemeNode] Add basic background-position support Add basic support for background-position, which only supports absolute values. Also don't require an unit to be specified for 0 (because the unit does not really matter here 0 is 0 regardless of the unit).
Comment on attachment 168940 [details] [review] [StThemeNode] Add basic background-position support >+ result->x1 = self->background_position_x; >+ result->x2 = self->background_position_x + new_w; >+ result->y1 = self->background_position_y; >+ result->y2 = box_h; This still seems strange... we scale the image, but not the offset. I think "if (self->background_position_set)" should be outside the check-if-it-needs-to-be-scaled, and we should just not do scaling if the image is absolutely positioned. (We may actually want to revisit the idea of scaling even in the non-positioned case...) >+ /* honor the specified position if any */ >+ if (self->background_position_set) >+ { >+ result->x1 = allocation->x1 + self->background_position_x; >+ result->y1 = allocation->y1 + self->background_position_y; Remove "allocation->x1 + " and "allocation->y1 + ". "result" is a box inside "allocation", not inside allocation's parent. the rest looks good
Created attachment 169178 [details] [review] [StThemeNode] Add basic background-position support Add basic support for background-position, which only supports absolute values. Also don't require an unit to be specified for 0 (because the unit does not really matter here 0 is 0 regardless of the unit).
Comment on attachment 169178 [details] [review] [StThemeNode] Add basic background-position support good, except >+ /* scale the background into the allocated bounds, when not being absolutly positioned */ typo. "absolutely"
Attachment 169178 [details] pushed as 9f9067e - [StThemeNode] Add basic background-position support