33710 - background-color: inherit doesn't work correctly

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Pierre Saslawsky]

Troy: Remember the BodyFixupRule? We copy the background from the BODY to the 
HTML and then clear the BODY's background transparent.

You made that fix because of bug 2054, which has the following testcase:
  - http://www.student.oulu.fi/~sairwas/css-tests/BODY-margin.html

Problem: it creates a bug with the W3C test suite: 
 - Go to http://www.w3.org/Style/CSS/Test/current/sec71.htm (a simplified version 
will be attached).
 - Look at the 3rd paragraph "This paragraph should have a solid gray background 
(or a white grid), because in CSS1, inherit is an invalid keyword, and in CSS2, 
it will cause the gray background (not the white grid) to be inherited."

The bug occurs because we set the NS_STYLE_BG_COLOR_TRANSPARENT on the BODY after 
copying it to the HTML, then "P.two" inherits the background from the BODY, which 
causes it to inherit from that flag as well (nsCSSStyleRule.cpp, line 1722).

My question is: why do we make the BODY's background transparent? I can imagine 
it may hurt the performance to have the same background-image on the BODY and the 
HTML. If that's the case (or if for any other reason, we need to keep the BODY 
transparent), a solution would be to add a flag similar to NS_STYLE_BG_PROPOGATED 
but which would mean "the background was propagated, go get it in the parent".

Comment 1

[Pierre Saslawsky]

Attachment: simplified testcase

Comment 2

[troy]

We make the BODY's background transparent so that the BODY doesn't also render 
the background. Yes, it would be a performance issue, but the real reason is 
that the BODY would render the same background at a different offset than the 
HTML element rendered it and things would not line up.

It was decided that the BODY should not render the background.

I'm fine with adding something like NS_STYLE_BG_PROPOGATED. That sounds like it 
might involve overhead for the style system, so maybe we could leave the 
background specified on the BODY as well, but add a flag that says the 
background has been propagated to its parent and should be not be rendered.

Or we could use the flags to indicate the difference between "specified" and 
"actual" background. That's similar to the CSS model, where "specified" is what 
you would use when resolving style for child elements of the BODY and "actual" 
is what layout would use when rendering

This was Peter's idea and it's more a style issue than a layout issue so 
whatever you want to do is fine. As long as we don't have the BODY also render 
the background

Comment 3

[troy]

On a related note we have another similar issue that we need to solve before we 
ship. I mentioned it to you and Marc a while ago, but I didn't hear back.

Just as we "reverse propagate" the BODY's background to the HTNL element for 
HTML documents (only) for backwards compatibility reasons, the CSS2 spec also 
says that the document element's background should be rendered over the entire 
canvas.

I attempted to do that by resizing the document element's frame to cover the 
canvas, but that's wrong and it doens't work anyway if the content has something 
like a TABLE for the document element.

We need to do the same trick of reverse propagating the background from the 
document element to the "canvas" frame's style context. The canvas frame (we 
call it the root frame in the code today, but I will change it to be called 
canvas) is the parent frame of the document element's frame

Whatever solution we come up with we need to solve this problem as well

Any ideas on where the best place to do this is? We could do it in frame 
construction, but then the style system will need to be aware that it happened 
so when style changes you can do the proper thing, e.g., the specified BODY 
background changes and we need to reverse propagate it to the HTML element and 
to the canvas and then make sure the canvas is repainated

Comment 4

[Pierre Saslawsky]

CCd attinasi because it is related to bug 8524

Comment 5

[Marc Attinasi]

Yea, the other is bug Troy referred to is bug 11491.

Pierre, how is this related to bug 8524?

Comment 6

[Pierre Saslawsky]

These 3 bugs are related in the way that they rely on the BodyFixupRule that 
moves the background from the BODY to the HTML element.

Comment 7

[Marc Attinasi]

Bug 8524 had to do with the border color of elements with -moz-bg-inset and has 
been fixed.

Comment 8

[Pierre Saslawsky]

Fix checked in nsCSSStyleRule.cpp, nsHTMLBodyElement.cpp and nsStyleConsts.h.
Tested with the W3C and the RichInStyle.com test suites.

Comment 9

[Christine Hoffman]

Verified fixed.