Bug 10794

Summary:

[CSS1] Background-attachment: scroll should scroll within the containing block

Product:

WebKit

Reporter:

Gérard Talbot (no longer involved) <browserbugs2>

Component:

CSS

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

hyatt, ian, mitz

Priority:

Version:

420+

Hardware:

OS:

OS X 10.4

URL:

http://www.gtalbot.org/BrowserBugsSection/MSIE7Bugs/BackgroundCenterOfBodyBugInIE7.html

Attachments:

Description	Flags
test case	none
patch	hyatt: review-
patch (updated)	none
patch (updated 2)	hyatt: review+

Gérard Talbot (no longer involved)

Reported 2006-09-09 18:40:20 PDT

According to CSS 1, section 5.3.5 http://www.w3.org/TR/CSS1#background-attachment and CSS 2.1, section 14.2.1 http://www.w3.org/TR/2006/WD-CSS21-20060411/colors.html#background-properties when background-attachment is scroll, then a background-image should scroll within its containing block, not within the viewport. "If a background image is specified, the value of 'background-attachment' determines if it is fixed with regard to the canvas or if it scrolls along with the content." http://www.w3.org/TR/2006/WD-CSS21-20060411/colors.html#background-properties Steps to reproduce: ------------------- 1- Load the provided URL 2- resize vertically the browser window to change the vertical dimension of the viewport. Expected results: ----------------- the lime squared "+" image should remain vertically in the middle of its containining block, which is the body. The lime squared + image should not scroll up and down. Notes ----- - I used Swift 0.1 to discover this bug. I do not have (nor can access) a Mac and/or Safari 2.x - I have searched for duplicates and couldn't find any. - The testcase at provided URL can be improved if needed, if requested. - I believe the original testcase coming from Ian "Hixie" Hickson.

Attachments
test case (1.17 KB, text/html) 2008-04-14 20:01 PDT, Anatoli Papirovski	no flags	Details
patch (38.41 KB, patch) 2008-04-14 20:03 PDT, Anatoli Papirovski	hyatt: review-	Details Formatted Diff Diff
patch (updated) (39.00 KB, patch) 2008-04-14 21:28 PDT, Anatoli Papirovski	no flags	Details Formatted Diff Diff
patch (updated 2) (24.58 KB, patch) 2008-04-15 09:18 PDT, Anatoli Papirovski	hyatt: review+	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Dave Hyatt

Comment 1 2006-09-09 20:46:20 PDT

I don't believe this is a bug. When the HTML element has no background, the body's background propagates up to to the root element and from there to the underlying canvas. This behavior is intentional in HTML documents as authors have the expectation that the "body" of the document will cover the entire canvas. See section 14.2, starting at the third paragraph: "The background of the root element becomes the background of the canvas and covers the entire canvas, anchored at the same point as it would be if it was painted only for the root element itself. The root element does not paint this background again. For HTML documents, however, we recommend that authors specify the background for the BODY element rather than the HTML element. For HTML documents whose root HTML element has computed values of 'transparent' for 'background-color' and 'none' for 'background-image', user agents must instead use the computed value of those properties from that HTML element's first BODY element child when painting backgrounds for the canvas, and must not paint a background for that BODY element. This does not apply to XHTML documents." Leaving open while i double-check with the CSS WG though.

Gérard Talbot (no longer involved)

Comment 2 2006-09-09 22:07:56 PDT

Mr Hyatt, I believe this comes from: http://www.hixie.ch/tests/adhoc/css/background/09.html You may want to consult Ian "Hixie" Hickson too... just a friendly suggestion :) Unless I'm wrong (and I may be :) ), in CSS 2.1, the root element does not (and should not) stretch vertically to cover/fill up the viewport. So, resizing the viewport should not extend the root element. So that lime squared "+" image should remained vertically-centered in whichever containing block it has and is being used for background-positioning. Regards, Gérard

Gérard Talbot (no longer involved)

Comment 3 2006-09-09 22:24:30 PDT

I've added borders around <html> and <body> in the testcase (provided URL).

Dave Hyatt

Comment 4 2006-09-10 00:33:27 PDT

The background is painted by the *canvas* and not by the root element. The canvas *does* cover the viewport. "The background of the root element becomes the background of the canvas and covers the entire canvas, anchored at the same point as it would be if it was painted only for the root element itself. The root element does not paint this background again." I suppose "anchored at the same point as it would be if it was painted only for the root element" implies maybe Safari is wrong.

David Kilzer (:ddkilzer)

Comment 5 2006-09-10 08:05:29 PDT

Hixie, your comment on this bug is desired.

Ian 'Hixie' Hickson

Comment 6 2006-09-10 15:22:52 PDT

> I suppose "anchored at the same point as it would be if it was painted only for > the root element" implies maybe Safari is wrong. Yes, Safari is wrong. If you have 'background-position: bottom' on the root element then it should paint at the bottom of the root element, not the bottom of the viewport. That's what that sentence means. :-)

Anatoli Papirovski

Comment 7 2008-04-14 20:01:05 PDT

Created attachment 20541 [details] test case

Anatoli Papirovski

Comment 8 2008-04-14 20:03:59 PDT

Created attachment 20542 [details] patch It's one of my first patches, so please be nice... I'm more than happy to learn and fix any mistakes I've made. I ran it through the webkit tests and the hixie.ch background tests to check for regressions, and made sure to cross-reference with the w3c specs.

Dave Hyatt

Comment 9 2008-04-14 20:50:10 PDT

Comment on attachment 20542 [details] patch This is on the right track. Thanks for contributing. Comments below: background-size and background-position are both supposed to be computed using the box specified by background-origin. This is CSS3 stuff so that may not really be obvious yet, but that's how it is supposed to work. Your check for background-position is correct. However background-size is now taking rather funny values, since it could end up being the viewport's width/height - margins. background-size should be using the same width and height you're passing to background-position. The destination rect for fixed backgrounds is also being potentially messed up by your modifications to make left/right non-zero. For example: cw = pw + left + right; Now that you put margins into left/right, the destination rect for fixed backgrounds will be too small. Style nitpick: Put spaces between arithmetic expressions, e.g., height() + marginTop() + marginBottom() and not height()+marginTop()+marginBottom().

Anatoli Papirovski

Comment 10 2008-04-14 21:28:58 PDT

Created attachment 20543 [details] patch (updated) Ok, I've implemented the requested changes and added some variables (rw, rh) to make it better formatted.

Anatoli Papirovski

Comment 11 2008-04-14 21:30:51 PDT

Will update tomorrow with test cases for background size

Anatoli Papirovski

Comment 12 2008-04-15 09:18:48 PDT

Created attachment 20557 [details] patch (updated 2) Ok, hopefully this is the final patch. I've modified my test case to also test background-origin for background-size on root element. Also, while I was testing that I noticed that WebKit's implementation of background-size: 50% auto;, that is percentage width and auto height, was broken and so I just quickly fixed it... was there a reason why it was that way, which I missed? (The formula that was there for calculation did not make any sense what-so-ever.) There is no separate ticket for that and I didn't think it was worth creating one for a change of one variable. No test case was needed for that one, as there is one in fast/backgrounds/size ... it's the number 18.

Dave Hyatt

Comment 13 2008-04-15 10:51:20 PDT

Comment on attachment 20557 [details] patch (updated 2) - h = bg->imageSize(style()->effectiveZoom()).height() * scaledWidth / bg->imageSize(style()->effectiveZoom()).width(); + h = bg->imageSize(style()->effectiveZoom()).height() * w / bg->imageSize(style()->effectiveZoom()).width(); This change fixes computation of height, but isn't the corresponding width case also broken? You could adjust the background-size computation so that it computes values of w and h first (for fixed/percent). Then handle the auto cases for each after that. (That enables you to fix the width case also.) So instead of if (width is fixed) else if (width is percent) else if (width is auto) if (height is fixed) else if (height is percent) else if (height is auto) you could do: if (width is fixed) else if (width is percent) if (height is fixed) else if (height is percent) if (width is auto && !height is auto) else if (!width is auto && height is auto) else if (width and height both auto)

Dave Hyatt

Comment 14 2008-04-15 11:02:40 PDT

Comment on attachment 20557 [details] patch (updated 2) Ah I see that the other case is not broken. I still think the cleanup is nice, but it's not necessary.

Dave Hyatt

Comment 15 2008-04-15 11:17:24 PDT

Fixed in r31912.

Gérard Talbot (no longer involved)

Comment 16 2008-06-20 14:36:16 PDT

Hello all, When using Safari 3.1.2 build 525.21 (XP Pro SP3), the lime + image vertically moves, changes its y-position. When I follow steps to reproduce, I do NOT get expected results. Can someone confirm my findings?

Gérard Talbot (no longer involved)

Comment 17 2009-06-14 13:46:22 PDT

When I try the provided testcase with the latest stable release Safari 4.0 build 530.17, I get expected results: as I resize vertically the browser window, the lime squared "+" image remains vertically in the middle of its containining block, which is the body. So, please ignore my previous comment. regards, Gérard

Note You need to log in before you can comment on or make changes to this bug.