Bug 10794 - [CSS1] Background-attachment: scroll should scroll within the containing block
: [CSS1] Background-attachment: scroll should scroll within the containing block
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: CSS
: 420+
: PC Mac OS X 10.4
: P2 Normal
Assigned To: Nobody
http://www.gtalbot.org/BrowserBugsSec...
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-09-09 18:40 PDT by Gérard Talbot
Modified: 2009-06-14 13:46 PDT (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Gérard Talbot 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.
Comment 1 Dave Hyatt 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.
Comment 2 Gérard Talbot 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 
Comment 3 Gérard Talbot 2006-09-09 22:24:30 PDT
I've added borders around <html> and <body> in the testcase (provided URL).
Comment 4 Dave Hyatt 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.
Comment 5 David Kilzer (:ddkilzer) 2006-09-10 08:05:29 PDT
Hixie, your comment on this bug is desired.
Comment 6 Ian 'Hixie' Hickson 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. :-)
Comment 7 Anatoli Papirovski 2008-04-14 20:01:05 PDT
Created attachment 20541 [details]
test case
Comment 8 Anatoli Papirovski 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.
Comment 9 Dave Hyatt 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().
Comment 10 Anatoli Papirovski 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.
Comment 11 Anatoli Papirovski 2008-04-14 21:30:51 PDT
Will update tomorrow with test cases for background size
Comment 12 Anatoli Papirovski 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.
Comment 13 Dave Hyatt 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)
Comment 14 Dave Hyatt 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.
Comment 15 Dave Hyatt 2008-04-15 11:17:24 PDT
Fixed in r31912.

Comment 16 Gérard Talbot 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?
Comment 17 Gérard Talbot 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