98159 - erroneous computed background-color value on table elements

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect, P2)

Description

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Run the attached test case both with 20010904 build and moz0.9.2 (or ns6.1) :

background-color for | table    | tbody       |  tr          | 
td---------------------+----------+-------------+--------------+----------------
20010904 build       | #ffffff  | #ffffff     | #ffffff      | #ffcccc
---------------------+----------+-------------+--------------+----------------
mozilla 0.9.2        | #ffffff  | #ffff66     | #ffff66      | #ffcccc
---------------------+----------+-------------+--------------+----------------
EXPECTED RESULT      | #ffff66  | transparent | transparent  | #ffcccc

Comment 1

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: test case showing bug

Comment 2

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

This bug blocks CSSization of Composer because the background-color button is
unable to reflect the real background color for table elements.

Comment 3

[Boris Zbarsky [:bzbarsky]]

getting #ffffff instead of "transparent" is bug 97771

But we should still not be getting #ffffff for the table...

Comment 4

[karnaze (gone)]

Temporarily moving to future until a milestone can be assigned. 

Comment 5

[Boris Zbarsky [:bzbarsky]]

This happens because the primary frame for <table> is the outer frame.  The
outer frame seems to always have computed background of transparent. Possible fixes:

1)  Use the inner frame instead of the outer frame for background computation only
2)  Use the inner frame instead of the outer frame for all computed style
3)  Make the outer frame inherit things like background so that we can compute
    style on the outer frame at all times

Bernd?  Karnaze?  Thoughts?  I bet computed dimensions would be wrong if we did
#2.  #3 seems like a lot of work with unknown layout ramifications.  Should I
just  do #1 in the computed style code?

Comment 6

[Bernd]

#3 is wrong it will color the space between the inner table frame and the
caption frame.
#2 is wrong also due to the caption frame, I think.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to fix

Use the inner frame for background and borders.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Bernd, would you review?

Comment 9

[Bernd]

Just a first nit: 
+            frame->GetParentStyleContextProvider(presContext,
+                                                 &frame,
+                                                 relationship);
scares me, shouldn't you pass another Frameholder. just search for other users
of GetParentStyleContextProvider. Boris you will be far better (faster) to ask
dbaron for review. I am slow. 
Did you run the table regression tests? 
Does that patch also fix the transparent frames assertion?

Comment 10

[Bernd]

Boris, I think I have to apologize for misleading information. Outertable frames
dont paint, but they can inherit. Probably your patch should use only margins as
they can not be inherited.

Comment 11

[karnaze (gone)]

Yes, we should just put background:inherit within table-outer in html.css 
(recall that table-outer style inherits from table style, just the reverse of 
the frame hierarchy), since the outer table doesn't print backgrounds. The only 
thing that the outer table can never inherit is margin. 

Boris, sorry for the delay in responding to this - I often miss questions in 
bugs, because I have so many bugs.

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK.... so let me see if I understand correctly:

1)  We don't change computed style code at all
2)  we add

  background: inherit;
  border: inherit;

to html.css with a comment explaining why it's there
3)  We run the table regression tests to make sure nothing broke.

Is that correct?

Bernd, I'm not sure what you mean when you mention margins... I'm not very
familiar with the frame model for tables, to be truthful.  :(

Comment 13

[karnaze (gone)]

That is correct. For this bug, the regression tests should only be used to see 
if there are bbox mismatches, just in case we are overlooking something 
regarding borders. Don't pay attention to frame state differences. We can't 
really catch background problems in the tests, so just bring up a few pages to 
make sure backgrounds look ok. 

Bernd was saying that if we need to get "margin" from the primary frame (outer 
table), then code will be necessary, because we can't allow the outer table to 
inherit "margin". But I don't think that is necessary for this bug.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Regression test output (the post-change version)

Ran regression tests on that "inherit".  The only "new" failure was:

+regression test verify/40129.rgd failed

I can't figure out whether the failure is actually a bbox failure or a
framestate failure, so attaching output here in the hopes that someone can
actually make sense of it.

Comment 15

[karnaze (gone)]

Sorry, but the attachement has no new lines in it, so I couldn't read it. If you 
find a bbox difference on a test, view the test to see if it is a bogus message 
(we do get a few of those).

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Just the text for the test in question

The attachment had unix newlines... sorry about that.

This is a plaintext version of just the test in question.  It _does_ seem to be
a bbox failure.  I've looked at it in Mozilla with and without my change and it
looks unchanged to the naked eye.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to fix more simply

Looked at testcases in bug 40129.  Looked at test by hand before and after
patch.	I see no differences in behavior...

Comment 18

[karnaze (gone)]

Comment on attachment 64631 [details] [diff] [review]
Patch to fix more simply

r=karnaze

Comment 19

[Marc Attinasi]

Comment on attachment 64631 [details] [diff] [review]
Patch to fix more simply

sr=attinasi, but please add a comment in the CSS file explaining why it is
needed, and why it is OK to always inherit the background and border.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment added and checked in