Closed Bug 509681 Opened 15 years ago Closed 15 years ago

Gradients as body background should fill the browser window

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: manfred, Assigned: ventnor.bugzilla)

References

()

Details

(Keywords: verified1.9.2)

Attachments

(4 files, 2 obsolete files)

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.
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a2pre) Gecko/20090810 Minefield/3.6a2pre

Looks indeed weird.
Component: General → Style System (CSS)
OS: Mac OS X → All
Product: Firefox → Core
QA Contact: general → style-system
Hardware: PowerPC → All
Version: unspecified → Trunk
Blocks: 479220
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
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.
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.
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 .
OK, never mind then. I think I found the problem anyway.
Attached patch Patch (obsolete) — Splinter Review
Some code was not being run when it should.
Assignee: nobody → ventnor.bugzilla
Attachment #393949 - Flags: review?(dbaron)
Could this just be bug 507939?
Assignee: ventnor.bugzilla → nobody
I suppose if Michael sees it, then no.
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 on attachment 393949 [details] [diff] [review]
Patch

I suppose roc could do this if dbaron doesn't get to it first.
Attachment #393949 - Flags: review?(roc)
Comment on attachment 393949 [details] [diff] [review]
Patch

Didn't see Jeff's comment.
Attachment #393949 - Flags: review?(roc)
Attachment #393949 - Flags: review?(dbaron)
(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.
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.
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.
Attachment #393949 - Attachment is obsolete: true
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.
Oh, so our current behaviour is correct if the bg is not fixed?
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.
Attached patch Patch (obsolete) — Splinter Review
This works right.
Assignee: nobody → ventnor.bugzilla
Attachment #394003 - Flags: review?(roc)
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).
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.
I just missed that, sorry.

That makes this bug INVALID, doesn't it?
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.
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.
(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
Attached file test case
root element with border/padding/margin
body with gradient background
Attached image screenshot of testcase
On the left, Minefield 20090812 nightly, on the right WebKit latest nightly.
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.
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 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.
Attached patch Now with testsSplinter Review
Attachment #393949 - Attachment is obsolete: true
Attachment #394003 - Attachment is obsolete: true
Keywords: checkin-needed
Blocks: 113577
Updated to tip.
Attachment #394213 - Attachment is obsolete: true
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...
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.)
It's actually fixing the size of the gradient image, specifically.
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)
Attachment #394213 - Flags: approval1.9.2+
Comment on attachment 394213 [details] [diff] [review]
Now with tests

Indeed I do. Would you or someone else be able to check it in?
Attachment #394213 - Attachment is obsolete: false
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [needs 1.9.2 checkin]
Attachment #395999 - Attachment description: Rebased → Rebased (checked into m-c)
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;
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.
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
Status: RESOLVED → VERIFIED
Flags: blocking1.9.2? → in-testsuite+
Keywords: verified1.9.2
Target Milestone: --- → mozilla1.9.3a1
You need to log in before you can comment on or make changes to this bug.