Closed Bug 531371 Opened 14 years ago Closed 14 years ago

background image disappears when body{display:table} is used (common CSS hack for correct centering+scrolling in Firefox)

Categories

(Core :: Layout: Tables, defect)

Product:
Core
Component:
Layout: Tables
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
status1.9.2 --- beta5-fixed

People

(Reporter: jc.bugzilla.20.jc, Assigned: roc)

References

(
URL
)

Details

(Keywords: regression)

Attachments

(2 files)

test case
819 bytes, text/html
Details
fix
10.14 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; de; rv:1.9.2b4) Gecko/20091124 Firefox/3.6b4
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; de; rv:1.9.2b4) Gecko/20091124 Firefox/3.6b4

Firefox 3.6 Beta doesnt show background images on pages which use
body {
  background: url(image.jpg) no-repeat scroll center top;
  display: table;
}
to get a centered background image which is centered on the webpage.
(omiting display:table makes the background image centered as if there would be no scrolling)

Reproducible: Always

Steps to Reproduce:
1. open http://www.webdesignerwall.com/tutorials/how-to-css-large-background/ and compare it with FF3.5
2. open Firebug on this page (with FF3.6) and disable body {display:table}

Actual Results:  
1. FF3.6 doesnt show the background image, FF3.5 does
2. FF3.6 also shows the background image, but with the scrolling problem

Expected Results:  
display the backround image as in versions before

( OR: at least show the background image as there would be no display:table )

http://www.webdesignerwall.com/tutorials/how-to-css-large-background/ also explains why this CSS Hack is needed in Firefox.

--

I classified this bug as a major bug because it breaks existing webpages and requires the webmasters to use a CSS switch if this bug will be contained in FF3.6 final version.
Gecko/20090721 Minefield/3.6a1pre
http://hg.mozilla.org/mozilla-central/rev/f2a58ffcd00c

fails
Gecko/20090722 Minefield/3.6a1pre
http://hg.mozilla.org/mozilla-central/rev/02f8bf10f441


http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f2a58ffcd00c&tochange=02f8bf10f441

I suspect bug 505184.
Blocks: 505184
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Keywords: regression
OS: Linux → All
Attached file test case — 
There should be a thin lime strip at the top of the page.
It looks like we're not longer propagating body backgrounds to the canvas at all in this case, no?  I mean... the background color isn't painting either.
Status: UNCONFIRMED → NEW
Ever confirmed: true
And in fact bug 505184's use of DisplayBackgroundUnconditional would cause just such behavior.
Component: Layout: Images → Layout: Tables
QA Contact: layout.images → layout.tables
Assignee: nobody → roc
Attached patch fix — — Splinter Review 
I'm not sure what you meant about DisplayBackgroundUnconditional being the problem. It seems to me that the bug is that FindRootFrame (which needs a better name) doesn't look for the inner table frame when the body element's primary frame is a tableOuter frame. So this patch creates nsLayoutUtils::GetStyleFrame and uses it in FindRootFrame and in a couple of other places where we're explicitly checking for tableOuter right now.

Then again, I must be missing something since I don't see how this could have worked before.
Attachment #415055 - Flags: review?(bzbarsky)
I just realized that we don't properly handle the case where <body> is display:none. I guess we should still propagate the background in that case. But that requires more significant changes, because we can't just return a frame. Right now I have difficulty caring.
Whiteboard: [needs review]
> I'm not sure what you meant about DisplayBackgroundUnconditional being the
> problem.

I think I was just wrong about that.

> I don't see how this could have worked before.

It "worked" by not propagating the background to the viewport (e.g. the color doesn't cover the whole viewport if set) but just painting it on the table.  The new code skips painting via the table painter and puts in an nsDisplayBackground.  That calls nsCSSRendering::PaintBackground, which calls FindBackground, which calls FindElementBackground, which returns false, since the root element's background is transparent and this is the body.  Then aForFrame has no -moz-appearance, and in any case it's not for the root element, so we bail out of PaintBackground.
Comment on attachment 415055 [details] [diff] [review]
fix

Looks good.
Attachment #415055 - Flags: review?(bzbarsky) → review+
Whiteboard: [needs review] → [needs landing]
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Flags: blocking1.9.2+
Pushed http://hg.mozilla.org/mozilla-central/rev/4f6f09ec4447
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing]
You need to log in before you can comment on or make changes to this bug.