Last Comment Bug 509681 - Gradients as body background should fill the browser window
: Gradients as body background should fill the browser window
Status: VERIFIED FIXED
: verified1.9.2
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla1.9.3a1
Assigned To: Michael Ventnor
:
: Jet Villegas (:jet)
Mentors:
http://www.schubert-it.com/about/
Depends on:
Blocks: 113577 479220
  Show dependency treegraph
 
Reported: 2009-08-11 03:24 PDT by Manfred Schubert
Modified: 2009-09-04 04:33 PDT (History)
14 users (show)
hskupin: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta1-fixed


Attachments
Patch (3.05 KB, patch)
2009-08-11 17:33 PDT, Michael Ventnor
roc: review+
Details | Diff | Splinter Review
Patch (2.42 KB, patch)
2009-08-12 03:26 PDT, Michael Ventnor
roc: review-
Details | Diff | Splinter Review
test case (637 bytes, text/html)
2009-08-12 17:21 PDT, philippe (part-time)
no flags Details
screenshot of testcase (180.25 KB, image/png)
2009-08-12 17:22 PDT, philippe (part-time)
no flags Details
Now with tests (4.36 KB, patch)
2009-08-12 21:04 PDT, Michael Ventnor
dbaron: approval1.9.2+
Details | Diff | Splinter Review
Rebased (checked into m-c) (3.06 KB, patch)
2009-08-21 17:25 PDT, Michael Ventnor
no flags Details | Diff | Splinter Review

Description Manfred Schubert 2009-08-11 03:24:38 PDT
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10_5_8; de-de) AppleWebKit/530.19.2 (KHTML, like Gecko) Version/4.0.2 Safari/530.19
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9.2a1) Gecko/20090806 Namoroka/3.6a1

When a gradient is set as background for the body, then it should be at least as large as the browser window.

Reproducible: Always

Steps to Reproduce:
1. Make a gradient the background of the body.
2. Make the browser window larger than the body text.
Actual Results:  
Ugly white area appears at the bottom of the browser window.

Expected Results:  
The gradient should fill the whole browser window.

Background colors and background images fill the whole browser window. Therefore background gradients should do the same. They do in Safari.
Comment 1 Ria Klaassen (not reading all bugmail) 2009-08-11 07:33:12 PDT
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a2pre) Gecko/20090810 Minefield/3.6a2pre

Looks indeed weird.
Comment 2 Michael Ventnor 2009-08-11 17:05:15 PDT
Without no-repeat, the gradients repeat for the length of the window. bgOriginRect is being reported wrong, or is this expected? I suggest using the root element rather than the body.
Comment 3 Michael Ventnor 2009-08-11 17:21:08 PDT
I think this is purely because of a difference between what Gecko and Webkit consider the "body". If you set the gradient rule on the root element, I think it should work out fine.
Comment 4 Markus Stange [:mstange] 2009-08-11 17:23:19 PDT
For HTML pages, "body" has always been magic. When the html element doesn't have a background, the body's background must propagate to the html element.
Comment 5 David Baron :dbaron: ⌚️UTC-10 2009-08-11 17:23:49 PDT
Backgrounds on body (when none is on the root) vs. on the root should behave indistinguishably.  If moving it to the root makes a difference, that's a bug.  See the third and fourth paragraphs of http://www.w3.org/TR/CSS21/colors.html#background .
Comment 6 Michael Ventnor 2009-08-11 17:30:09 PDT
OK, never mind then. I think I found the problem anyway.
Comment 7 Michael Ventnor 2009-08-11 17:33:15 PDT
Created attachment 393949 [details] [diff] [review]
Patch

Some code was not being run when it should.
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-08-11 17:33:18 PDT
Could this just be bug 507939?
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-08-11 17:33:40 PDT
I suppose if Michael sees it, then no.
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2009-08-11 18:12:59 PDT
You should define the behavior of gradients in terms of CSS3's background positioning and painting areas.  Read the comment at the top of the method and determine precisely where you want any deviation to fit; just moving code like this is going to break other things.  Specifically, the patch you propose here, which I assume you have not run against layout/reftests/backgrounds reftests, will incorrectly place percent-positioned scaled background images if I read it right -- there is at least one reftest which checks this.
Comment 11 Michael Ventnor 2009-08-11 18:20:38 PDT
Comment on attachment 393949 [details] [diff] [review]
Patch

I suppose roc could do this if dbaron doesn't get to it first.
Comment 12 Michael Ventnor 2009-08-11 18:21:15 PDT
Comment on attachment 393949 [details] [diff] [review]
Patch

Didn't see Jeff's comment.
Comment 13 Michael Ventnor 2009-08-11 18:24:40 PDT
(In reply to comment #10)
> You should define the behavior of gradients in terms of CSS3's background
> positioning and painting areas.  Read the comment at the top of the method and
> determine precisely where you want any deviation to fit; just moving code like
> this is going to break other things.  Specifically, the patch you propose here,
> which I assume you have not run against layout/reftests/backgrounds reftests,
> will incorrectly place percent-positioned scaled background images if I read it
> right -- there is at least one reftest which checks this.

The code that I move is completely independent from all the code above it. All it does is make sure gradients get the right bgPositioningRect, if it does break things like you say then I wouldn't know why. I'll re-run the reftests to double-check, but I don't see how this code move could break since the only interaction is gradients reading the new bgPositioningRect.
Comment 14 Jeff Walden [:Waldo] (remove +bmo to email) 2009-08-11 18:34:22 PDT
Oh, hm, I misread the patch, that's a different hunk from what I thought it was.  You may actually be safe; I'd have to look closer than I actually did.
Comment 15 Michael Ventnor 2009-08-11 19:55:36 PDT
OK, this presents an interesting problem. If you set an image URL then do -moz-background-size 100% 100%, you get the exact same problem here with images. I'm not really sure what to do.
Comment 16 Jeff Walden [:Waldo] (remove +bmo to email) 2009-08-12 00:28:41 PDT
Not true re image URLs and background-size: 100% 100%.  Percent background-sizes are relative to the dimensions of the background positioning area, which is the element itself.  If you want the percentages to be determined relative to the entire window, you have to have fixed background-attachment to change the background positioning area to be the entire window.
Comment 17 Michael Ventnor 2009-08-12 01:31:41 PDT
Oh, so our current behaviour is correct if the bg is not fixed?
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-08-12 02:04:00 PDT
For backgrounds that have been propagated to the viewport, the background positioning area should be the whole window. It's as if the background was really on the viewport, not on the root or body elements.
Comment 19 Michael Ventnor 2009-08-12 03:26:05 PDT
Created attachment 394003 [details] [diff] [review]
Patch

This works right.
Comment 20 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-08-12 04:14:03 PDT
It seems to me that if the root element has a margin then this patch is going to draw the gradient background with the size of the canvas frame, but positioned at an offset to the canvas frame, which seems wrong.

It also seems to me that our current code is correct insofar as the gradient's intrinsic size should be the background positioning area.

I don't know what the correct behaviour is here. I think this is a spec issue. The spec needs to clarify what the background positioning area is for backgrounds that have been propagated to the viewport. I think a reasonable option would be to do the same as for background-attachment:fixed and make the background positioning area the initial containing block. A slightly better option might be to make the background positioning area be the entire area that can be scrolled into view by the viewport scrolling mechanism (or the initial containing block if there is no such mechanism).
Comment 21 fantasai 2009-08-12 10:10:16 PDT
What part of
http://dev.w3.org/csswg/css3-background/#special-backgrounds
  # The background of the root element becomes the background of the canvas
  # and its background painting area extends to cover the entire canvas,
  # although any images are sized and positioned relative to the root element
  # as if they were painted for that element alone. (In other words, the
  # background positioning area is determined as for the root element.)
is unclear?

I can't spec the behavior you want because of
http://www.w3.org/TR/CSS21/colors.html#background
  # The background of the root element becomes the background of the canvas
  # and covers the entire canvas, anchored (for 'background-position') 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.
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-08-12 13:07:43 PDT
I just missed that, sorry.

That makes this bug INVALID, doesn't it?
Comment 23 Michael Ventnor 2009-08-12 16:17:23 PDT
Webkit does fill the entire viewport when a gradient is set on the body. I'm pretty sure it will stretch to the entire page if part of the page is offscreen. I'm not sure what it does if a margin is specified on the root element.
Comment 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-08-12 16:31:22 PDT
That may be so but they don't necessarily get this right.

Try a test which gives the root element border, padding and margins, and try using a background image with background-position:0 0 and background-position:100% 100%. That should be positioned within the root element borders, according to the spec fantasai quoted in comment #21. If Webkit gets that right, then they must give gradient backgrounds special treatment that doesn't use the background positioning area to size or position the gradient.
Comment 25 philippe (part-time) 2009-08-12 17:15:26 PDT
(In reply to comment #24)

WebKit propagates the gradient background to the viewport (the same as background-color). But I don't think Gecko gets the gradient correctly.

two tests, background on <body>:
with gradient
http://dev.l-c-n.com/_temp/background-gradient-root2.html
with background-image
http://dev.l-c-n.com/_temp/background-gradient-root.html
Comment 26 philippe (part-time) 2009-08-12 17:21:13 PDT
Created attachment 394168 [details]
test case

root element with border/padding/margin
body with gradient background
Comment 27 philippe (part-time) 2009-08-12 17:22:42 PDT
Created attachment 394169 [details]
screenshot of testcase

On the left, Minefield 20090812 nightly, on the right WebKit latest nightly.
Comment 28 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-08-12 17:47:14 PDT
Your gradient testcases are using background-attachment:fixed, which forces the background positioning area to be the initial containing block, which is not the case we're dealing with in this bug.
Comment 29 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-08-12 17:55:11 PDT
Sorry, actually, the original bug report here *is* for background-attachment:fixed. Webkit behaves the same way we do for background-attachment:normal. So all this discussion was pointless and we clearly do have a bug...
Comment 30 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-08-12 17:57:42 PDT
Comment on attachment 393949 [details] [diff] [review]
Patch

This patch was exactly right. Since the intrinsic size of the gradient depends on the background positioning area, we need to finish computing the background positioning area before we set the gradient intrinsic size.

We do need tests here though.
Comment 31 Michael Ventnor 2009-08-12 21:04:34 PDT
Created attachment 394213 [details] [diff] [review]
Now with tests
Comment 32 Michael Ventnor 2009-08-21 17:25:33 PDT
Created attachment 395999 [details] [diff] [review]
Rebased (checked into m-c)

Updated to tip.
Comment 33 Michael Ventnor 2009-08-21 17:26:29 PDT
Dao, would you be able to check this in at any time?

This'll probably also need approval1.9.2 if the blocking flag isn't set soon...
Comment 34 David Baron :dbaron: ⌚️UTC-10 2009-08-22 08:03:51 PDT
I can land it.  Am I correct that the commit message should say that it's fixing the area used for background-position:fixed gradients?  (I don't think it has anything to do with body anymore.)
Comment 35 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-08-22 08:55:28 PDT
Yes.
Comment 36 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-08-22 08:56:03 PDT
It's actually fixing the size of the gradient image, specifically.
Comment 37 David Baron :dbaron: ⌚️UTC-10 2009-08-22 11:20:22 PDT
http://hg.mozilla.org/mozilla-central/rev/909161bf21e5

Not sure if that makes this bug fixed, though.
Comment 38 David Baron :dbaron: ⌚️UTC-10 2009-08-22 11:21:11 PDT
Comment on attachment 394213 [details] [diff] [review]
Now with tests

And a1.9.2=dbaron (I suspect you'll have to use this version for the 1.9.2 landing)
Comment 39 Michael Ventnor 2009-08-22 18:52:49 PDT
Comment on attachment 394213 [details] [diff] [review]
Now with tests

Indeed I do. Would you or someone else be able to check it in?
Comment 40 aja+bugzilla 2009-08-23 01:17:08 PDT
Just an FYI for crossbrowser testing, since I ran into it...either webkit or opera (the latter IIRC) incorrectly defaults 2nd background-size parm to be same as 1st parm, instead of auto. 
e.g.:
background-size: 100% computes to 100% 100% instead of to 100% auto;
Comment 41 Jeff Walden [:Waldo] (remove +bmo to email) 2009-08-23 03:16:23 PDT
Last I checked both incorrectly treat b-s:X as b-s:X X rather than b-s:X auto.  I filed a WebKit bug on the matter, and presumably they'll be fixing it Real Soon Now as they've removed their -webkit- prefix from the property.

https://bugs.webkit.org/show_bug.cgi?id=28440

No word on Opera as they don't really have a way to give concise technical feedback of this nature.
Comment 43 Henrik Skupin (:whimboo) 2009-09-04 04:33:23 PDT
Verified fixed on trunk and 1.9.2 with builds on OS X, Linux, and Windows like:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20090902 Minefield/3.7a1pre ID:20090902030655

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a2pre) Gecko/20090903 Namoroka/3.6a2pre ID:20090903033550

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