Closed Bug 4510 Opened 25 years ago Closed 20 years ago

table, row, column, *-group backgrounds not working correctly (<col>, <colgroup>, table, style, background)

Categories

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

Product:
Core
Component:
Layout: Tables
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: dbaron, Assigned: fantasai.bugs)

References

(
URL
)

Details

(Keywords: css2, testcase, Whiteboard: [awd:tbl])

Attachments

(4 files, 17 obsolete files)

temporary fix
9.79 KB, patch
karnaze
: review+
attinasi
: superreview+
Details | Diff | Splinter Review
screenshot
20.69 KB, image/png
Details
screenshot of painting inside the cellspacing
36.89 KB, image/png
Details
final patch (?)
135.62 KB, patch
dbaron
: superreview+
Details | Diff | Splinter Review 
You have a number of serious problems with backgrounds on table elements (CSS2,
section 17.5.1: http://www.w3.org/TR/REC-CSS2/tables.html#table-layers ).

My three tests:

http://www.fas.harvard.edu/~dbaron/csstest/sec170501
http://www.fas.harvard.edu/~dbaron/csstest/sec170501a
http://www.fas.harvard.edu/~dbaron/csstest/sec170501b

show the following problems:

1) Backgrounds on table-column and table-column-group elements don't work.

2) Transparency on table-cell elements causes the table-row and table-row-group
backgrounds to be ignored.  This is because of you current inherit hackery in
ua.css.  I assume if the tables were created in XML, the table-row and
table-row-group backgrounds wouldn't work either.

3) Backgrounds are not ignored (as they should be) on empty table-cell elements.

If you want table backgrounds to work correctly, you need to paint them in the 6
layers described in the spec (table, table-column-group, table-column,
table-row-group (or header or footer), table-row, table-cell) and not use the
inherit stuff in ua.css.
Status: NEW → ASSIGNED
Target Milestone: M5
chrisd, is this Linux specific?
OS: Linux → All
Hardware: PC → All
It's not Linux specific.  It's the same on Linux and Windows.  I just reported
it on Linux.  Changing OS and Platform to All.
Target Milestone: M5 → M6
Moving to M6.
Moving to M8
ua.css, this bug, and and many other table style bugs will be easily fixed after
Peter adds a new style rule (in the code) which gets applied after the html
style sheet but before the css style sheets.
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
Fixed with latest checkin. Only works correctly in Standard Mode.
Status: RESOLVED → VERIFIED
Using 6/14 Apprunner, colors display an indicated on test cases. Verifying bug
fixed.
Background attribute of <tr> does not inherit to
 <td>. (Build 2000062020 Windows 2000)


<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<HTML lang=ja>
<HEAD>
   <STYLE TYPE="text/css"><!--
tr     {background:  #80ffff;
        color:       #000000}
td     {background:  #000000;
        color:       #ffffff}
--></STYLE>
</HEAD>
<BODY background=#ffffff>
<table>
<tr><td>a</td><td></td><td>c</td></tr>
<tr><td>a</td><td>b</td><td>c</td></tr>
</table>
</BODY>
</HTML>

For HTML above, background color becomes #fffff of body.
This should be color of <tr> #80ffff.

 >These "empty" cells are transparent, letting lower layers shine through.

(CSS2 17.5.1)

I'm reopening this bug, since I think this should not be only for standard mode. 
 Are there any pages that depend on table backgrounds not working?  We don't 
want quirks mode to be blatant emulation of Nav4 -- page authors hate Nav4.  We 
only want to emulate the quirks that pages depend on.

Also clearing M7 TM and removing peterl from cc: list, but adding Ian.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Target Milestone: M7 → ---
I think this should be fixed for beta3.  Assuming we decide to do it, it should 
be very easy to do.
Keywords: nsbeta3
Status: REOPENED → ASSIGNED
Keywords: testcase
One bug report == one issue is one of the golden rules of Bugzilla because it
enables independent tracking and prioritization of each issue. David, could you
please split this into separate bug reports, one per issue, because not all of
these issues are of equal priority. For example, we don't support styles at all
on columns or column groups (FUTUREd), so supporting backgrounds on them should
be FUTUREd as well. But you can make your case that the other bugs should be of
higher priority. Please justify to PDT user/developer impact, any blocking
effect on adoption within content, etc.
virtual attinasi
Whiteboard: [nsbeta3-]
Target Milestone: --- → Future
*** Bug 48013 has been marked as a duplicate of this bug. ***
see also bug 47563
See comment on bug 55264 for yet another reason why this should be fixed in
quirks mode too.

Regarding ekrock's comment above -- it *is* one issue.  It's just that when the
fix was checked in, it was for standard mode only (for no apparent reason).
Keywords: css2
Nominating for mozilla 0.9 to get on people's radar.  This is a trivial fix, and
I don't see any reason not to make it.  We're better off avoiding
quirks/standard mode differences that we don't need.
Keywords: mozilla0.9
NavQuirks behavior should be kept for <tr bgcolor="x">, though, since all 
versions of MSIE also render it that way. (And Opera, too.)

Do you want CSS2 behavior for /CSS/-applied backgrounds on <tr>?
QA contact update
QA Contact: chrisd → amar
Keywords: nsbeta3
Keywords: mozilla0.9 → mozilla0.9.2, mozilla1.0
Whiteboard: [nsbeta3-]
Target Milestone: Future → ---
nomination of mozilla0.9 conflicts w/ target milestone of future. clobbering 
both and nsbeta3-.  Can we please try to get closure on this bug?
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=20181 [bug 46268]
should fix this for everything except table rows (a necessary quirk).


what needs to be done on this one?! Hope to see this one fixed soon.
copy from 46268:

Just a short quote from the CSS2 spec:

17.6.1 The separated borders model

In this model, each cell has an individual border. The 'border-spacing' property
specifies the distance between the borders of adjacent cells. 


*This space is filled with the background of the table element*.


 Rows, columns, row groups, and column groups cannot have borders (i.e., user
agents must ignore the border properties for those elements).

I think what IE is doing is just right, they inherit the color into the table
cell. I think we should do the same. This would require from fantasai's patch
only the nsTableFrame.cpp part in order to paint the cellspacing with the table
backgrounds color.

I have to admit that the CSS2 spec is not conflict free in this matter and as
one can see by the David's posting
http://lists.w3.org/Archives/Public/www-style/1999Jul/0083.html and the answers,
the W3C is not able create a clear specification over two years
now. So it is hard to believe that this will change very soon.

The honest interpretation of the conflict is that for the collapsed border model
 as  default the space between cell's should be covered by table- , row- , col-
etc background, while for the separated border model it deviates from the
default model and only the table background is used for the space between the
cells. 
The WG is looking at this. I hope to have an answer for you on Monday.
Whiteboard: (WG bug 85)
*** Bug 87232 has been marked as a duplicate of this bug. ***
*** Bug 90247 has been marked as a duplicate of this bug. ***
We partly removed the table background quirks and now we have the regression
bugs, before going any further in this direction we should carefully analyze our
painting bugs because the majority of pages use still the quirk mode and
switching to the less tested strict mode can create a bunch of regressions.
Depends on: 91034
Depends on: 68998
Whiteboard: (WG bug 85) → [awd:tbl](WG bug 85)
Target Milestone: --- → mozilla1.0
Blocks: 55623
*** Bug 84986 has been marked as a duplicate of this bug. ***
CSS2 Errata [http://www.w3.org/Style/css2-updates/REC-CSS2-19980512-errata.html]
 |
 | [2001-08-27] At the end of the section add the following paragraph:
 |
 |   Note that if the table has 'border-collapse: separate', the background of
 |   the area given by the 'border-spacing' property is always the background
 |   of the table element. See 17.6.1"

*** Bug 113067 has been marked as a duplicate of this bug. ***
*** Bug 119442 has been marked as a duplicate of this bug. ***
*** Bug 119442 has been marked as a duplicate of this bug. ***
Keywords: mozilla0.9.9
We could easily make the strict mode finally compliant to the spec (errata), aka
not drawing the cellspacing background, by switching it to the quirks mode
rendering. Oh man I love that, sorry David and Hixie if you cant share my joy.
I second a switch to Quirks rendering for Standard mode. It's almost identical 
to IE and Opera. It won't break pages. Right now, our rendering is incorrect per 
CSS2, and I think it's pointless to keep it that way. As for making our 
rendering standards-compliant, the CSS2 spec is too ambiguous about how one 
would handle, for example

  TR {background: url(starburst.gif) no-repeat;}

IMO, Quirks is good enough for 1.0, it's easy to change, and it's better than 
what we have now. At the very least, we shouldn't introduce yet another 
incorrect rendering for page authors to handle.
Attached patch temporary fix — — Splinter Review 

    
    

    
  
Comment on attachment 69072 [details] [diff] [review]
temporary fix

r=karnaze. fantasia, please run enough visual tests to convince yourself the
patch is ok.

        Attachment #69072 -
        Flags: review+

    
    

    
  
I looked at the testcase in bug 46268, all three of dbaron's table background 
tests, and a testcase with image backgrounds. We render incorrectly per spec, of 
course, but aside from the column backgrounds, we're consistent with IE5.

    
    

    
  
Comment on attachment 69072 [details] [diff] [review]
temporary fix

sr=attinasi (egg-shields up)

        Attachment #69072 -
        Flags: superreview+

    
    

    
  
Marking nsbeta1+
Keywords: nsbeta1+
Blocks: 116212

    
    

    
  
fix checked in

    
    

    
  
->fantasai
Assignee: karnaze → fantasai
Status: ASSIGNED → NEW

    
    

    
  
fixed
Status: NEW → RESOLVED
Closed: 25 years ago → 23 years ago
Resolution: --- → FIXED

    
    

    
  
This bug isn't actually fixed yet. Reopening and moving to future.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla1.0 → Future

    
  
Status: REOPENED → ASSIGNED

    
    

    
  
*** Bug 116212 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 134261 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 137707 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 137779 has been marked as a duplicate of this bug. ***

    
    

    
  
Even if you don't solve this bug for every case, could you at least be as
standards compliant as IE5 and paint background colors for colgroup and col like
IE5 does?  I'm trying to classify data using foreground colors for rows and
background colors for columns.  

BTW:  There's a lot of discussion here about CSS2 being ambiguous.  But I think
this bug would be a bug for HTML4/CSS1 as well as HTML4/CSS2.  I think that
would make Netscape 4.79's behaviour a bug and why carry that into Mozilla 1?  I
don't think CSS2 is very ambiguous regarding table backgrounds. 

    
    

    
  
> Even if you don't solve this bug for every case, could you at least be as
> standards compliant as IE5 and paint background colors for colgroup and col
> like IE5 does?

Then we would be condoning and spreading IE5's incorrect behavior. IMO, IE5 is 
not any more standards compliant than we are here. Even if you interpreted 
according to the scant information in CSS1, background is applied to the element 
it is specified on, not on a collection of associated elements as IE5 does.

> I think this bug would be a bug for HTML4/CSS1 as well as HTML4/CSS2. 

CSS1 does not bother to specify anything about table rendering; intra-table 
elements' behavior is effectively outside the scope of that specification.

    
    

    
  
It's not a CSS problem, it's an HTML4 problem.

First, I am not implying that IE is a better browser overall.  I appreciate the
efforts of mozilla.org to supply a W3C standards-based browser and I'm just
trying to help the process.

>CSS1 does not bother to specify anything about table rendering; 

No, but HTML4 DOES specify how <col/> and <colgroup> should behave.  
Strictly speaking, Navigator4 and Mozilla1RC1 do not conform to HTML4 (and
therefore XHTML 1.0 I think) in regards to <col/> or <colgroup>.  Neither one
correctly displays the sample table in the HTML4.01 spec, and there isn't a
single STYLE attribute in that sample.  

My opinions:

As I see it there are three separate issues which may all be one bug or 3 bugs:
1: Mozilla doesn't attempt to paint the background of <COL> or <COLGROUP>
2: Mozilla doesn't properly apply HTML4 attributes to <COL> or <COLGROUP>
3: Mozilla doesn't inherit HTML4 attributes (including STYLE) from <COL> or
<COLGROUP> to <TH> or <TD>

Items 2 and 3 are clearly addressed in the W3C HTML4.01 specification.  
Item 1 is clearly addressed in the W3C CSS2 specification.

>IMO, IE5 is not any more standards compliant than we are here.

Yes, it is in this case.  IE5 attempts to do all 3 things I listed above.  It
correctly shows the sample table from the HTML 4.01 spec.  It may not do them
perfectly but IMO half a glass is better than an empty glass.

>Even if you interpreted according to the scant information in CSS1, 
>background is applied to the element it is specified on, 
>not on a collection of associated elements as IE5 does.

OK, so IE5 doesn't paint the <col/> background directly.  It allows <td> to
inherit the background properties from <col/>.  Yes, this is inconsistent with
CSS2.  And the background property is prohibited from being inherited in
HTML4.01.  But I suggest that since IE5 doesn't apply the background property
directly to <col/> it is reasonable to allow it to be inherited by an element
that it can apply to.  Therefore I think it is one reasonable implementation of
HTML4/CSS1.  If you disagree, so be it.  But tell me how to apply a background
or a foreground style to a <col>?  

>CSS1 does not bother to specify anything about table rendering; 
>intra-table elements' behavior is effectively outside the scope 
>of that specification.

Correct.  It is established first in HTML 4.01 and refined in CSS2, and Mozilla
is out of compliance with both of them in regards to <col/>.  IE seems to at
least comply with HTML4.01 in this instance.

Good Luck!

    
    

    
  
*** Bug 140084 has been marked as a duplicate of this bug. ***

    
    

    
  
Ok, I discovered bug 915 which covers issues #2 and #3 for HTML attributs. 
Sorry about venting that here.  Issue #1 remains part of this bug.

    
    

    
  
http://lists.w3.org/Archives/Public/www-style/2002May/0077.html


> Item 1 is clearly addressed in the W3C CSS2 specification.

It is not clearly adressed in the W3C CSS2 specification, and that is exactly 
the reason this bug is Futured--until it /is/ clearly addressed.

    
    

    
  
Nice, clear description of the problems.    

The collapsed border table model is much simpler, is the default behavior of
many browsers, and is (I guess) more commonly used than the separated borders model.
Therefore it seems to me that fixing the column backgrounds would be simple
enough and important enough to do for the collapsed borders model regardless of
the status of the separated borders model.

The background-color property doesn't have any of the the patterning issues that
you have illustrated in your description.  Therefore it seems that implementing
the background-color property for both table models would be simple enough and
important enough to do regardless of the status of the other background properties.

Regarding spanning cells, I think that if cell R1C2 spans across R2C2 then cell
R2C2 does not exist.  The row background for R2 should be transparent for where
R2C2 would be.  The row or column background for R1C2 would fill the entire R1C2
cell. 

BTW: I don't think I would often use backgrounds on both columns and rows in the
same table.  I think most people would be satisfied if column backgrounds worked
as long as there were no row backgrounds to conflict with.

You said: "In describing the effects of the background properties, however, the
spec is very ambiguous, making it impossible to write a defensibly correct
implementation."

No wonder moz 1.0 has taken so long.  ;)  Just kidding.  I would say: "In
describing the effects of the background properties, however, the spec is
somewhat ambiguous, making it possible to write multiple defensibly correct
implementations."

In short, something is better than nothing.

Again, Thank You!

    
    

    
  
The separated borders model is the default in all browsers I know of, and I
don't know of any "real" pages that use the collapsed borders model.

There is still a major ambiguity with 'background-color' -- whether or not to
break at the cell spacing.

    
    

    
  
charles allen wrote:
> In short, something is better than nothing.

from a conversation I had with Chris Karnaze - 
   : we're more likely to have pages break if we enable those backgrounds than
     if we disable them (since IE also applies color and we don't)
   : Imagine a table with a colored column background. In IE, the background and
     the color that goes with it both get applied. 
   : In Mozilla, currently, neither get applied.
   : So there's no contrast problems.
   : If, however, we apply the background and not the color, we might wind up
     with navy text on a forest green background or something like that.

In my mind, we should either be compatible with other browsers or undisputably 
correct in our deviations. Anything we change now would be neither.

> I would say: "In describing the effects 

Yes, I probably should have written it that way.

dbaron wrote:
> There is still a major ambiguity with 'background-color' -- whether or not to
> break at the cell spacing.

I thought the Errata cleared that up?


    
    

    
  
quote:
   : we're more likely to have pages break if we enable those backgrounds than
     if we disable them (since IE also applies color and we don't)
   : Imagine a table with a colored column background. In IE, the background and
     the color that goes with it both get applied. 
   : In Mozilla, currently, neither get applied.
   : So there's no contrast problems.

Not always true.  I have a table where I'm assigning background colors to
columns, and foreground colors to rows.  This is clearly legal in CSS2.  In IE,
both get assigned and I have no problem.  In Mozilla, only the foreground colors
are assigned.  If I did not carefully choose my table background color, I would
have contrast problems in Mozilla, but not IE.

Why not draw background colors for columns the same way quirks mode draws them
for rows?  This would be extending quirks mode to columns rather than inventing
a new method.  AFIK this is also the same way IE draws column background colors
so you are still not inventing a new method.  This would make you more
compatible with other browsers, AND more compatible with CSS2 at the same time!
 A win/win if you ask me.

BTW, you are correct that most browsers use separate border model.  I didn't
believe you, so I proved it to myself.  :)  However CSS2 specifies the collapsed
borders model should be the default.  Therefore I would expect it to be default
in a CSS2 spec browser.  Again, the collapsed borders model is much simpler and
I think would be easier to fix.  You should fix it and maybe make it the
default.  Then I would care less about the broken separated borders model.

    
    

    
  
> However CSS2 specifies the collapsed borders model should be the default.

See http://www.w3.org/Style/css2-updates/REC-CSS2-19980512-errata.html#s-17-6

    
    

    
  
*** Bug 149008 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 172824 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 175075 has been marked as a duplicate of this bug. ***

    
    

    
  
Test suite: http://fantasai.tripod.com/www-style/2002/table-backgrounds/tests/
It's a work in progress; only part A has been written.
URL: http://fantasai.tripod.com/www-style/...

    
    

    
  

      
      
      
      

        Attached patch
          
          
        preliminary work (obsolete)
        — 
          — Splinter Review
      

    


  
So you can see where I'm going with this. Mozilla with the patch passes Part A
of the test suite. I still have to run it through as yet unwritten tests for
exact image position wrt borders, empty cells, etc. Also, the code needs
polish. Of course, I'm not submitting for review yet, but if anyone notices
problems with the overall algorithm, it would help to know sooner rather than
later.

Many thanks to bz for answering silly questions to which any amateur programmer
would know the answers. He's saved me hours of frustration.

    
    

    
  
*** Bug 179086 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 180404 has been marked as a duplicate of this bug. ***

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
Not up for checkin just yet. Got some conceptual issues with borders. But the
code is more-or-less in its final form.

        Attachment #105030 -
        Attachment is obsolete: true

    
    

    
  
My first impression of the patch is somehow ambivalent. Probably I am only dumb.

@@ -140,8 +140,9 @@
                               const nsRect& aBorderArea,
                               const nsStyleBorder& aBorder,
                               const nsStylePadding& aPadding,
-                              nscoord aDX,
-                              nscoord aDY,
+                              nscoord aDX = 0,
+                              nscoord aDY = 0,
+                              const nsRect* aClipRect = nsnull,
                               PRBool aUsePrintSettings=PR_FALSE);
 
why we need this aDX = 0 , aDY = 0 ?

+class ColumnBGData;
+
+struct ColGroupBGData { ....

do we really need these classes, I would love to see them going away. A malloc
inside a paint routine seems to me like a potential performance hit. If we
really need those can't we compute them during the reflow? If we can compute
them during reflow, why dont we change the existing frame structures.

ColumnBGData::~ColumnBGData()
+{
+  ColGroupBGData* lastColGroup = nsnull;
+  for (PRInt32 i = 0; i < mNumCols; i++) {
+    if (mCols[i].mColGroup != lastColGroup) {
+      lastColGroup = mCols[i].mColGroup;
+      delete mCols[i].mColGroup;
+    }
+    mCols[i].mColGroup = nsnull;
+  }
+  delete [] mCols;
+}

isnt +  delete [] mCols; enough?

 if (bgData.Ok() && bgData.ColGroup(colIndex)->Ok()) {
+              nsCSSRendering::PaintBackgroundWithSC(aPresContext,
aRenderingContext,
+                bgData.ColGroup(colIndex)->mFrame, aDirtyRect,
+                bgData.ColGroup(colIndex)->mRect,
*bgData.ColGroup(colIndex)->mBackground,
+                zeroBorder, zeroPadding, 0, 0, &cellRect, PR_FALSE);
+            }
+            //Paint row group background
+            if (rgBG) {
+              nsCSSRendering::PaintBackgroundWithSC(aPresContext,
aRenderingContext,
+                this, aDirtyRect, rgRect, *rgBG, zeroBorder, zeroPadding, 0, 0,
+                &cellRect, PR_FALSE);
+            }
+            //Paint column background
+            if (bgData.Ok() && bgData.Col(colIndex)->Ok()) {
+              nsCSSRendering::PaintBackgroundWithSC(aPresContext,
aRenderingContext,
+                bgData.Col(colIndex)->mFrame, aDirtyRect,
bgData.Col(colIndex)->mRect,
+                *bgData.Col(colIndex)->mBackground, zeroBorder, zeroPadding, 0, 0,
+                &cellRect, PR_FALSE);
+            }

why do we paint first the colgroup then the row group then the col, I thought
the spec requires  colgroup col rowgroup

+    nscoord cellSpacingX = table->GetCellSpacingX();
 
-    x = borderPadding.left;
+    x = borderPadding.left + cellSpacingX;
     y = borderPadding.top;
 
     availSize.width  = aAvailWidth;
     if (NS_UNCONSTRAINEDSIZE != availSize.width) {
-      availSize.width -= borderPadding.left + borderPadding.right;
+      availSize.width -= borderPadding.left + borderPadding.right + (2 *
cellSpacingX);
     
This is pure horror for me, this must be wrong. The patch alters the
nsTableReflowState. Before we reflow all the children.

Probably there should be some comments on those critical places why it does not
affect reflow.



    
    

    
  
> My first impression of the patch is somehow ambivalent. Probably I am only
> dumb.

Probably you're not asking the right questions. What are you ambivalent toward?
What bothers you about it? What would you have prefered?

> why we need this aDX = 0 , aDY = 0 ?

The arguments don't seem to be used anywhere in the function, and IIRC every
function call I've run across (I had to run a search modify all calls passing in
the aUsePrintSettings arg) passes in zero. So, I think we should eliminate the
arguments. I'm defaulting them to zero for now so most calls won't have to pass
those zeros in, and I'll file a bug on taking them out later.

> do we really need these classes, I would love to see them going away. A malloc
> inside a paint routine seems to me like a potential performance hit.

I originally put them in to avoid a performance hit. This is also why all the
painting is done in nsTableRowGroupFrame::Paint rather than in the frames'
respective classes; there is no need to iterate over all the cells in the table
once for the column groups, once more for the columns, another time for the row
groups, yet another time for the rows, and once again for the cells themselves.
Alternatively the cells could paint everything by calling into parents, but I
don't think this is any better than what I'm doing right now. (That method would
involve more complicated mRect adjustments.)

The paint function is iterating over all the cells in the row group, which in
most cases means all of the cells in the table. Instead of recalculating all the
column style and frame info for every cell, I'm calculating everything once and
storing the info in an array. The storage doesn't cost very much in terms of
memory because its created and destroyed in one function call, existing for a
very short amount of time. However, it's possible that the time cost in
allocating and deallocating the memory might outweigh the savings from storing
the calculated values. I don't know what the performance cost is for that. If
you think that it's better to call the table frame's GetColFrame and get the
background, mRect, and the colgroup frame (and it's background and mRect) for
each cell, I can change it.

> If we really need those can't we compute them during the reflow?

We could, but they don't seem important enough to take up space when we're not
painting.

> isnt +  delete [] mCols; enough?

No, because each ColBGData holds a pointer to its parent ColGroupBGData. Suppose
I have a colgroup with two columns. If I delete the first ColBGData, it will
delete its ColGroupBGData. When I delete the second ColBGData, it will call
delete on its pointer to that same ColGroupBGData. Except it's already been
deleted, and the space might have been allocated to something else.

> why do we paint first the colgroup then the row group then the col, I thought
> the spec requires  colgroup col rowgroup

Yes, you're right. I'll fix that, along with the MOZ_COUNT_CTOR and GetStyleData
stuff you mentioned on IRC.

> This is pure horror for me, this must be wrong. The patch alters the
> nsTableReflowState. Before we reflow all the children.

nsTableReflowState is already altered before the children are reflowed by
subtracting the border...

> Probably there should be some comments on those critical places why it does not
> affect reflow.

Actually, it does affect reflow. But the end result should be the same as
before. CellspacingX is subtracted out of the width, but it's also no longer
added to x before reflowing the first cell.

I will run the layout regression tests, and that should point out any major
miscalculation errors.

btw, if you could answer the few questions I've commented in, that would be helpful.

    
    

    
  
*** Bug 190455 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 190942 has been marked as a duplicate of this bug. ***

    
  
Depends on: 195291

    
    

    
  
*** Bug 196889 has been marked as a duplicate of this bug. ***

    
    

    
  
Removing mozilla0.9.9+, mozilla1.0, and nsbeta+ keywords as they are all
obsolete by many moons.
Keywords: mozilla0.9.9+, 
          
            mozilla1.0, 
          
            nsbeta1+

    
    

    
  
Please never again remove keywords you don't know about!

Keywords: mozilla0.9.9+, 
          
            nsbeta1+

    
    

    
  
If the keywords do not have the meaning that the "Keywords" page has, then
perhaps someone needs to update that page eh???

    
    

    
  
Could you possibly explain for the other 30-some people who are watching this
bug, what does mozilla0.9.9+ mean if it does not mean, "drivers@mozilla.org
would like to see these bugs checked in during the [0.9.9] milestone freeze or
branch period"? And, what is the meaning of an nsbeta1+ keyword placed there
many versions ago?

Thank you.

    
    

    
  
There was a "temporary fix" for this bug that was checked in a while ago to make
us, if not correct, at least compatible with the other incorrect rendering engines
out there. I believe the keywords apply to that. This bug was then marked FIXED,
but I reopened it because it isn't really fixed.

    
    

    
  
roc, views are seriously broken on tables. I did the best I could, given that
they don't paint properly. Relevant testcases are:
http://fantasai.tripod.com/www-style/2002/table-backgrounds/tests/layers-opacity.html
http://fantasai.tripod.com/www-style/2002/table-backgrounds/tests/fixed-bg.html
That's not a background issue, though; content is also missing.
Target Milestone: Future → mozilla1.5alpha

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #106551 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        nsTableUnderlay.h (obsolete)
        — 
      

    


  

    
    

    
  

      
      
      
      

        Attached file
          
        nsTableUnderlay.cpp (obsolete)
        — 
      

    


  
ok, Bernd. Fire away!

(There are a few things I wasn't sure of; these are marked with XXX.)

    
    

    
  


  

        Attachment #125789 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        nsTableUnderlay.cpp (tweaked) (obsolete)
        — 
      

    


  

        Attachment #125790 -
        Attachment is obsolete: true

    
    

    
  
Ok, so. Explanations:

> what will the patch do and what not

The patch will fix every standards-mode table background issue I
know about except fixed backgrounds on columns/colgroups. Those
are broken...

Actually, now that I think about it, I might be able to make that
work, too. I'll give it a try. The backgrounds are painting--just
not properly.

So, fixed issues include:
 - positioning and tiling of background images
 - painting area (no cellspacing)
 - layering
 - views unwittingly giving cells wrong paint flags (bug 191567)

> which frames will change their size,

Rows, row groups, columns, and colgroups will no longer include
outer cellspacing in their dimensions.

> why did you implemented it the way you have choosen.

I implemented it this way because it seemed more efficient to
cache calculated data and make one pass through the table than
to have each cell calculate its own set of background data for
its row/rowgroup/column/colgroup. Implementing it as a separate
class makes it
  a) easier to understand
  b) easy to deal with views
  c) easy to reduce background painting to a single pass once
     the responsibility for painting borders is moved to the
     cells

> what tests has the already patch seen 

The patch passes all the tests at
  http://fantasai.tripod.com/www-style/2002/table-backgrounds/tests
with one problem: As aforementioned, views don't paint tables
properly so some cells are incompletely painted in the fixed
background and opacity tests. I also ran them in quirks mode;
I haven't broken anything. Although, I could also fix the way
<table> backgrounds leak in quirks border-collapse...

> risks 

Risk of pages breaking is pretty low; the tests I ran are quite
comprehensive.
Risk of crashes/leaks/other-stupid-mistakes is.. pretty high.
I'm a very inexperienced programmer; I wouldn't even rank myself
as high as amateur. In addition, I build on Linux, but that's all
I use it for, so the patched build hasn't seen extensive use.


I'm going to give Bugzilla a break and put up the code at
http://fantasai.tripod.com/Mozilla/bugs/4510
I'll attach the final versions for archiving when we're done.

dbaron -- you might want to check, at some point, to make sure
my interpretation is ok. After this goes in would probably be
easiest, since then you can grab a build and look at the test
cases.


    
    

    
  
> Actually, now that I think about it, I might be able to make [fixed bg on cols]
> work, too.

Looks like I can do that without much trouble. However, it means I have to cheat
a little; I'd have to ignore the bounds of the dirtyRect and paint the whole
column/columngroup. Do you want me to do this or should I add
col, colgroup {
  background-attachment: scroll !important;
}
to html.css instead?

    
  
Target Milestone: mozilla1.5alpha → mozilla1.5beta
Blocks: 203166

    
    

    
  
After reading the patch a first time ( 3hours) I believe the code will do the
job, however I am still concerned about the performance hit, why do we compute
for the whole table and not for the damage area, couldnt we do something similar
(call) as below
http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/nsTableFrame.cpp#6632
(oh, I thought I converted that into a function a long time ago). and even reuse
the damageArea afterwards for BC painting.

I will run the paint code after my vacation...


    
    

    
  
> why do we compute for the whole table and not for the damage area, couldnt we... 

Added coordinate checks to the loop.
If you want, I can make the intersect check in PaintCell into a separate inline
function and call the rest of the paint code conditionally based on its return
value.


    
    

    
  
The patch doesnt compile. 

While the GetView deCOM is easy, GetContinuousLeftBCBorderWidth is used in
nsTableUnderlay.cpp but not defined in nsTableFrame.h 
I think the patch needs to be updated to the tip, sorry for beeing so lame that
the patch started to bitrot.

    
    

    
  
> The patch doesnt compile.

Not surprising. It's based on the May 28th source.

> I think the patch needs to be updated to the tip

I'm trying. :) Mozilla's compiling right now. If I'm lucky, I'll get a working
Windows binary and I can start applying the patch. If not.. I guess I'll try to
imitate the Windows-Linux build environment I had on my old computer. Pulling
the source is working now that I've reinstalled cygwin, and given that, I should
be able to get it to work.

> sorry for being so lame that the patch started to bitrot.

It's ok. I just hope I'll be able to finish this before I leave this weekend...

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch (untested) (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #125788 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        nsTableUnderlay.h (untested) (obsolete)
        — 
      

    


  

        Attachment #125925 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        nsTableUnderlay.cpp (untested) (obsolete)
        — 
      

    


  

        Attachment #125928 -
        Attachment is obsolete: true

    
    

    
  
It compiles under mingw. More than that, I cannot say; I wasn't able to get a
working binary to test with under either Windows or Linux. If you can send me a
drop-in replacement of Gecko that I can use with a nightly, I'll test it...

    
    

    
  
*** Bug 214877 has been marked as a duplicate of this bug. ***

    
    

    
  
Apologies for duplicating this bug, I hadn't found it from a few searches.

However, could someone say what is happening or going to happen?  I got a bit
lost when people started using the code ;)

My basic question is: Will front end web developers be able to style with col?

Thanks :)  
I do try and persuade everyone to use Moz, which is becoming easier when they
realise they don't have to get all those ad-programs installed whilst browsing.

    
    

    
  
> Will front end web developers be able to style with col?

Yes, but see http://www.w3.org/TR/REC-CSS2/tables.html#q4

This bug will fix background rendering on columns in Standards layout mode.


    
    

    
  
> This bug will fix background rendering on columns in Standards layout mode.

That's great, I now have a related question about horizontal alignment, that you
might refer me to another bug for..

Moz seems to ignore horizontal alignment set by the width attribute of col, and
any CSS alignment class set on the col. E.g:
http://www.ukwindsurfing.com/results/2003/rankings_rb.asp

The CSS section refered to (http://www.w3.org/TR/REC-CSS2/tables.html#q4)
doesn't cover aligment - fair enough. But it doesn't give any control available
for controling columns without using classes set on each cell. In the example
above, the names are left aligned, and points are supposed to be right aligned.

The alignment attribute of col is in the XHTML DTD
(http://www.w3.org/TR/xhtml1/dtds.html#dtdentry_xhtml1-strict.dtd_col), has this
been intentionally left out, or would this be a separate bug?

Cheers, I hope I'm being useful, I've been using (& relying on) Moz since
version 1, but I'm new to the world of bugzilla. :)

-Alastair

    
    

    
  
Comment 98 is about bug 915, not this bug.  It's generally harmful to add
unrelated comments to bugs, especially long comments, since they make the real
issues in the bug harder to find.

    
    

    
  
Turning this over to Bernd for review/checkin/etc.
The two new files should be MPLed, of course.
And please look over comment 85.

    
    

    
  
Comment on attachment 129063 [details] [diff] [review]
Patch (untested)

*pokes Bernd* Bernd, will _you_ set the new target milestone? Obviously my
guesses are way off.

        Attachment #129063 -
        Flags: superreview?(bz-vacation)

        Attachment #129063 -
        Flags: review?(bernd_mozilla)

    
    

    
  

      
      
      
      

        Attached image
          
        screenshot 
        — 
      

    


  
the patch fails when a rowspan in the border collapsed table is scrolled into
the visible area.
furthermore the patch does not compile as 
-  PRInt32 numCols = GetColCount();
the numCols are used later in an assertion and the Makefile.in part of the
patch  is missing. There are some whitespace errors in the patch
http://www.johnkeiser.com/cgi-bin/jst-review-cgi.pl?patch_file=&patch_url=http%3A%2F%2Fbugzilla.mozilla.org%2Fattachment.cgi%3Fid%3D129063%26action%3Dview&patch_text=&reason_type=%21&reason_type=A&reason_type=B&reason_type=E&reason_type=F&reason_type=L&reason_type=N&reason_type=O&reason_type=P&reason_type=Q&reason_type=R&reason_type=S&reason_type=W&reason_type=X&reason_type=%7B

    
    

    
  
I've not digested all the changes yet, so a lot of these comments are
non-substantive.  I've also read only part of the patch.  So take these comments
with a boulder of salt...

Please use more context and use the -p option to diff (-p -u8 is a
good way to diff....).  That makes patches a lot more readable.

>Index: mozilla/content/shared/src/nsStyleStruct.cpp
>+#ifdef DEBUG
>+    if (mBorderStyle[NS_SIDE_TOP] & BORDER_COLOR_SPECIAL) {
>+      NS_WARNING("Clearing special border because BORDER_COLOR_DEFINED is not
set");
>+    }
>+#endif

Are there good reasons not to make that an assertion?  Are there ever cases
where it could happen that are technically correct?  Same for the other sides.

>Index: mozilla/layout/html/style/src/nsCSSRendering.h

>-                                    PRBool aUsePrintSettings=PR_FALSE);
>+                                    PRBool aUsePrintSettings=PR_FALSE,

Toss in spaces around the '=' sign, please?

>Index: mozilla/layout/html/document/src/quirk.css

>+/* make sure backgrounds are inherited in tables  -- see bug 4510*/
>+td, th, tr {
>+  background: inherit;
>+}

Do we really want this quirk?  What purpose does it serve?  In particular, what
would "tr" inherit from (that only comes into play on quirks-mode pages that
are setting backgrounds on <tbody> elements; I suspect we want to follow IE
there, and I doubt that IE has that quirk, but I could be wrong (no way I can
test)).

>Index: mozilla/layout/html/table/src/nsTableFrame.h

>+  /** Get left continuous border width
>+   */
>+  nscoord GetContinuousLeftBCBorderWidth(float  aPixelsToTwips,
>+                                         PRBool aGetInner) const;
>+

What does that mean, exactly?  A clearer explanation is in order here.  That
is, what is a "left continuous border"?  What does aGetInner do?

>-    unsigned : 19;                     // unused
>+    unsigned mLeftContBCBorder:8;
>+    unsigned : 11;                     // unused

I think I'd be happier if this struct were using PRUint32, so you knew you
actually _did_ have 32 bits to work with, but that can be a separate patch....

>Index: mozilla/layout/html/table/src/nsTableFrame.cpp

>-    x = borderPadding.left;
>+    x = borderPadding.left + cellSpacingX;
>     y = borderPadding.top;

Why the asymmetry here?  Comment it, if there is a good reason.

>-      availSize.width -= borderPadding.left + borderPadding.right;
>+      availSize.width -= borderPadding.left + borderPadding.right
>+                         + (2 * cellSpacingX);

Comment why you have to subtract off the cellspacing?  Same for the Y
direction?  Or perhaps comment where availSize is declared and say what size it
actually reflects?


    
    

    
  
> the patch fails when a rowspan in the border collapsed
> table is scrolled into the visible area.

Made some changes. See if that works.

> the numCols are used later in an assertion

Removed change. I think the compiler was complaining that the
variable wasn't used -- the assertion doesn't appear in
optimized builds, I assume. So therefore, the variable and
function call aren't, either. Should I put an #ifdef DEBUG on
it?

> and the Makefile.in part of the patch is missing

Ok, fixed.

> -p -u8 is a good way to diff

It shall be done. *updates diff4510 script to do that*

> Are there good reasons not to make that an assertion?  Are
> there ever cases where it could happen that are technically
> correct?

I can't think of any. I guess you want me to switch it over?

> Toss in spaces around the '=' sign, please?

Done.

> Do we really want this quirk? What purpose does it serve? 

I've wondered about that myself. I think it's only useful for
pages that use "background: transparent" on cells to make the
table background shine through.

> In particular, what would &quot;tr&quot; inherit from... 

table row group, of course.

> I doubt that IE has that quirk, but I could be wrong

IE does indeed have that quirk. (I think we're to be the only
layout engine that actually layers table backgrounds--although
I wouldn't be too surprised if Tasman's team did something like
that.)

> >+  /** Get left continuous border width
> What does that mean, exactly?  A clearer explanation is in
> order here.

Yes, you're quite right about that. I need to spec out the interaction
of table backgrounds with table borders.

> That is, what is a &quot;left continuous border&quot;? What does aGetInner do?

Commented in. Implementation moved inline into nsTableFrame.h

> I think I'd be happier if this struct were using PRUint32

Changed.

> Why the asymmetry here? Comment it, if there is a good reason.

What asymmetry? Between x and y? The cellspacing gets added to the y
coordinate during reflow; it's a running count and cellspacing gets
added before the child height in the loops. Comment added, anyway.

> Comment why you have to subtract off the cellspacing? 

availSize.width reflects the width available for the children. In
tables this means subracting out the cellspacing on the sides as well
as the border. (Before this change, cellspacing in the X direction was
treated as a shift in the table cell position within the row.)

(Updated patch is at http://fantasai.tripod.com/Mozilla/bugs/4510 ;
 No guarantee that it'll compile.)
Blocks: 216167

    
    

    
  
> No guarantee that it'll compile.
isnt that an euphimism for just the opposite. :-)
the patch at http://fantasai.tripod.com/Mozilla/bugs/4510/patch.txt
does not compile as a)

#define GET_TWIPS_TO_PIXELS(presContext,var) \
   float var; \
   (presContext)->GetScaledPixelsToTwips(&var); \
+
   var = 1.0f / var;
the additional line is not nessary.

(while you are on it, please remove the unessary blank lines from nsTableFrame.h)

In content/shared the assertion requires an argument what to test for.
I would rewrite that section with out the debug defines and loop over the four
sides.

>   for (PRUint8 Side = NS_SIDE_TOP; Side <= NS_SIDE_LEFT; Side++) {
>     if (!(mBorderStyle[Side] & BORDER_COLOR_DEFINED)) {
>       NS_ASSERTION(!(mBorderStyle[Side] & BORDER_COLOR_SPECIAL),
>                          "Clearing special border because BORDER_COLOR_DEFINED
is not set");
>       SetBorderToForeground(Side);
>       }
While this are peanuts, 
the patch draws into the cellspacing in separate mode when the table is not
completely inside the  window (see screenshot)

    
    

    
  
> painting inside the cellspacing

I've never seen /that/ before. Is it a regression from the previous patch?

> unnecessary blank lines

Stupid text editor. I'll fix that, don't worry. :)

    
    

    
  
this is not a regression it has been like this with attachment 129063 [details] [diff] [review], the trick
is the small window

    
    

    
  
*** Bug 218541 has been marked as a duplicate of this bug. ***

    
    

    
  
Resummarizing to what I think this bug currently covers.  If I'm wrong, please
resummarize again.
Summary: table element backgrounds not working correctly → table row, column, *-group backgrounds not shown

    
    

    
  
*** Bug 219404 has been marked as a duplicate of this bug. ***

    
    

    
  
We're fixing a bit more than that here. Things covered include:
  - paint table column/colgroup backgrounds
  - layer backgrounds properly
  - set up image positioning & repeating bounds correctly
  - cleaning up <table> background boundaries in Quirks (might as well)

I really ought to finish this off. If I don't post a (hopefully finalized)
patch by Monday, someone should send me a nice flame or something for being
so annoyingly slow... ~_~
Summary: table row, column, *-group backgrounds not shown → table, row, column, *-group backgrounds not working correctly

    
    

    
  
Patch updated. And it does compile. However, I don't know if it /works/ since I
still get linking errors...

    
    

    
  
*** Bug 221020 has been marked as a duplicate of this bug. ***

    
    

    
  
Re comment 104:

> I can't think of any. I guess you want me to switch it over?

Yes, please.

> IE does indeed have that quirk

OK.  Then it's ok to keep it as a quirk, I guess...

If you have an updated patch and are having trouble getting it to link, feel
free to mail it to me and I'll look..

    
    

    
  
I've put the files up at
  http://fantasai.tripod.com/Mozilla/bugs/4510/

I'll attach them for flagging once Bernd gives his ok, but attaching three files
every time I adjust whitespace would probably strain the CC list overmuch...

    
    

    
  
I cant apply the patch:
patch: **** malformed patch at line 790: +nsTableRowGroupFrame::GetContinuousBCB
orderWidth(float     aPixelsToTwips,

could you please create a patch with only diff -u, do you manually edit the
patch files ?

    
    

    
  
I'm patching with diff -p -u8. No, I didn't edit the patch by hand. I just reran
cvs diff and uploaded the result. I checked it, too, by running it in reverse
and then normally. Is it still causing problems?
QA Contact: amar → ian
Whiteboard: [awd:tbl](WG bug 85) → [awd:tbl]

    
    

    
  
*** Bug 224283 has been marked as a duplicate of this bug. ***

    
    

    
  
patch updated; fixed nsStyleStruct assertions.
The border-collapse assertions still go off when I build without the patch, so I
don't think I'm causing the problem. Also, it's in a function I don't call..

    
  
Flags: blocking1.6b+

    
  
Flags: blocking1.6b+

    
    

    
  
I tested the patch at http://fantasai.tripod.com/Mozilla/bugs/4510/ . It looks
good to me. And it is so much better what mozilla does currently. Thanks a lot.
Attach it to the bug. You should ask bz, roc and dbaron as the module owner to
look carefully at this. I think, I looked pretty hostile at the patch but this
can't be a substitute for a review.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #129063 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        nsTableUnderlay.h (obsolete)
        — 
      

    


  

        Attachment #129064 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        nsTableUnderlay.cpp (obsolete)
        — 
      

    


  

    
  

        Attachment #129065 -
        Attachment is obsolete: true

    
  

        Attachment #135353 -
        Flags: superreview?(bz-vacation)

        Attachment #135353 -
        Flags: review?(dbaron)

    
  

        Attachment #129063 -
        Flags: superreview?(bz-vacation)

        Attachment #129063 -
        Flags: review?(bernd_mozilla)

    
    

    
  
*** Bug 225780 has been marked as a duplicate of this bug. ***
Summary: table, row, column, *-group backgrounds not working correctly → table, row, column, *-group backgrounds not working correctly (<col>, <colgroup>, table, style, background)

    
    

    
  
So... is this going to get reviewed in time for 1.6beta or should I set the
target to January (1.7alpha)?

    
    

    
  
fantasai, I definitely won't get to it in time for beta (in the next 40 hours,
basically).  I'm sorry I dropped the ball on this so badly.  :(

On the other hand, I feel that this is alpha material in any case.  And I will
make sure that I've done a review well before 1.7a opens so you can land as soon
as it does...

    
    

    
  
> I'm sorry I dropped the ball on this so badly.  :(

Eh, don't worry too much about it. I doubt dbaron or roc would've found time,
either. And I've taken out several months myself, what with not being able to
build for so long...

> On the other hand, I feel that this is alpha material in any case.

Yeah, I'd prefer to have it checked in during an alpha cycle, too. :)

    
  
Blocks: 226426
Blocks: 227031
Blocks: 149378

    
  
Target Milestone: mozilla1.5beta → ---

    
    

    
  
Comment on attachment 135354 [details]
nsTableUnderlay.h

Why "nsTableUnderlay"?	Wouldn't "nsTableBackgroundPainter" be a better
reflection of what's going on?

You need a license comment here.

>#define BC_BORDER_TOP_HALF_COORD(p2t,px)    NSToCoordRound((px - px / 2) * p2t )

You need parens around "px" in that expression, both places (consider what
happens if I pass "2-1" as "px".

Same for all the other macros here.

>   * User is expected to loop through elements to be
>   * painted <em>in layout order</em> (row groups must
>   * be ordered properly), using Load/Unload functions

Perhaps you should clearly document which functions need to be called in which
order to paint a cell or whatever objects this paints?	An example would
probably not hurt either....

>   * TableBackgroundPainter will handle position
>   * translations

Meaning into the child's coordinate space?  If so, I think it would be better
to say that clearly.

>    TableBackgroundPainter(nsIPresContext*      aPresContext,
>                           nsIRenderingContext& aRenderingContext,

Why isn't that a pointer?  References to interfaces like that make me queasy.

>      * @param aDeflate        - adjustment for frame's rect

That's not helpful.  Explain why one would want to pass this and what it does.

>      * @param aSkipSelf   - pass this cell; i.e. paint only underlying layers

Why would one use this?

I would rather you avoided params with default values; people will have a
tendency to assume the default value is the correct one.  It's better to make
them explicit  so they have to think about how they are calling the function.

>    struct TableBackgroundData {
>      /** Data is valid */
>      PRBool Ok() const { return (PRBool)mBackground; }

Maybe call this IsOK() ?

>      /** Destructor; must call Clear first, however */

Why?  If this class always has a frame, it can get the prescontext off the
frame.	So there is no need to pass a prescontext to it, and hence no need for
Clear() that I see.  Unless mFrame can be null?

>      /** Calculate and set data values to represent aFrame */
>      void Set(nsIPresContext*      aPresContext,
>               nsIRenderingContext& aRenderingContext,
>               nsIFrame*            aFrame);

Does aFrame have to be non-null?  Is aPresContext really needed here?  Could
you use a pointer to the nsIRenderingContext instead?

>    nsIRenderingContext& mRenderingContext;

And here.

    
    

    
  
Comment on attachment 135354 [details]
nsTableUnderlay.h

>      PRBool Ok() const { return (PRBool)mBackground; }

Also, that's not cool in general -- casting a pointer to PRBool does not do
what you really want it to.  Use "return !!mBackground;" instead, I would say.

    
    

    
  
>      PRBool Ok() const { return (PRBool)mBackground; }

is uncool as bz said, because it generated random non-zero results on 32-bit
systems, and where pointers may be 64 bits but int is 32, it may even generate 0
(PR_FALSE) for a non-null pointer.

The !!p trick looks odd enough to people that I've always gone for an explicit
null comparison, e.g.:

>      PRBool Ok() const { return mBackground != nsnull; }

Note no need for (PRBool) casting.

/be

    
    

    
  
Comment on attachment 135355 [details]
nsTableUnderlay.cpp

Need a license.

>TableBackgroundPainter::TableBackgroundData::Clear(nsIPresContext* aPresContext)

Like I said, just get the prescontext off mFrame and be done with it.

>  mBorder = nsnull;
>  mBackground = nsnull;
>  mFrame = nsnull;
>  mBorderIsSynth = PR_FALSE;

Do you really need to reset all these?

>  //XXXfr should aRenderingContext be removed from IVFP's param list?

Probably.  File a bug on that, cc me and roc and dbaron.  We probably want to
deCOMtaminate that method at the same time too.

>inline PRBool
>TableBackgroundPainter::TableBackgroundData::ShouldSetBCBorder()

Any reason not to just inline this into the header?

>TableBackgroundPainter::TableBackgroundData::SetBCBorder(nsMargin& aBorder,
>  NS_PRECONDITION(aPainter, "null frame");

Change the text to make sense?

>  nsStyleBorder* styleBorder = new (aPainter->mPresContext) nsStyleBorder;
>  if (!styleBorder) return NS_ERROR_OUT_OF_MEMORY;
>  *styleBorder = aPainter->mZeroBorder;

Wouldn't 

  nsStyleBorder* styleBorder = 
     new (aPainter->mPresContext) nsStyleBorder(aPainter->mZeroBorder);
  if (!styleBorder) return NS_ERROR_OUT_OF_MEMORY;

be better?

>TableBackgroundPainter::TableBackgroundPainter(nsIPresContext*      aPresContext,

This could use a constructor counter.

>TableBackgroundPainter::Init(nsTableFrame*        aTableFrame,
>      NS_ASSERTION(colGroupList.FirstChild(), "table should have at least one colgroup");

Are you sure?  What if I'm in XHTML and I just have an <html:table> with
nothing inside?  Will this assert trigger?  What will mNumCols be?

Perhaps it would be better to bail early or skip stuff if mNumCols is 0, then
assert if mNumCols > 0 and there is no first child?

>      for (nsTableColGroupFrame* cgFrame = (nsTableColGroupFrame*)colGroupList.FirstChild();
>           cgFrame; cgFrame = (nsTableColGroupFrame*)cgFrame->GetNextSibling()) {

It's not immediately clear to me what this code is attempting to do.  A comment
would help a lot here.	I assume that you're storing the colgroup info for each
col or something?

In any case, comments about you're doing this here (and comments in the header
when you declare these data structs explaining what they are used for) would be
most helpful.

>          nsresult rv = cgData->SetBCBorder(border, this);
>          if (NS_FAILED(rv)) return rv;

Doesn't that return leak cgData?

>          mCols[colIndex].mCol.mRect.MoveBy(cgData->mRect.x, cgData->mRect.y);

So mCols[colIndex].mCol.mRect is the rect of the col in the coord system of the
table?	If so, why?  Also, this needs to be documented somewhere (eg the
header, either where you declare the ColData type or where you declare mCols or
whatever you deem appropriate).

In fact, it would be good to document the coord systems for all the various
members that end up having an nsRect as part of them.

>          mCols[colIndex].mColGroup = cgData;

So the idea is that cgData is allocated in Init() and deallocated in the
destructor of this class?  That is, the mCols elements have pointers to it, but
do not own it?	If so, document that clearly here and perhaps where mCols is
declared....

>              nsresult rv = mCols[colIndex].mCol.SetBCBorder(border, this);
>              if (NS_FAILED(rv)) return rv;

Document that this does NOT leak cgData, since there is now a pointer to it in
mCols[colIndex].

I have to admit that I'm not sure quite what's going on with the BC left border
(in particular, why you reset to lastLeftBorder every time in the inner loop. 
Please document that.

>TableBackgroundPainter::PaintTable(nsTableFrame*         aTableFrame,

More BC magic... why exactly do we only sometimes need to set the bcborder? 
The ShouldSetBCBorder impl should probably document why it uses the criterion
it uses.

>TableBackgroundPainter::TranslateColumns(nscoord aX,
>                                         nscoord aY)

This method needs some documentation in the header.

>  if (eOrigin_TableRowGroup != mOrigin) {

It helps to comment which coord space you are transforming to before doing the
coord transformations... you're transforming into the row group coord space
here, right?

>    mRenderingContext.Translate(mRowGroup.mRect.x, mRowGroup.mRect.y);
>    mDirtyRect.MoveBy(-mRowGroup.mRect.x, -mRowGroup.mRect.y);
>    if (mCols) {
>      TranslateColumns(-mRowGroup.mRect.x, -mRowGroup.mRect.y);

When you translate back in UnloadRowGroup(), you use
mRowGroup.mFrame->GetRect() as the rect.. are you guaranteed that the x,y
coords of that are the same as the x,y coords of mRowGroup.mRect at this point?
 If so, assert that here, please.

>TableBackgroundPainter::LoadRow(nsTableRowFrame* aFrame)
>  //else: No translation necessary since we're using row group's coord system

Erk.  First I hear of that, and I read the header.  That needs to be VERY
clearly documented somewhere.....

I'm not very knowledgeable about BC code, so I can't tell whether what you're
doing here with BC is right... if bernd is OK with it, I'm OK with it.

>TableBackgroundPainter::PaintCell(nsTableCellFrame* aCell,

>  nsRect cellRect;
>  cellRect = aCell->GetRect();

Why two lines?	Just put that on one line, please.

>  //Paint cell background in border-collapse unless told not to.

Why only in border-collapse?  Please document that somewhere....

I'm wondering about the Pass* functions.... if I decide to call PassRow(), I
need to make sure I don't LoadRow() that row, right?  Because it looks like if
I load a row and then call PassRow() on it, the row's background will be
painted.

Could we add some debug-only code that asserts that LoadRow() is not called
while another row is already loaded, and similarly for row groups, etc?

    
    

    
  
By the way, I just realized that you have some places where you call Clear()
when you don't want to call the destructor.  So it's OK to keep Clear(), but the
destructor can call it, which would allow callers who plan to just destroy to
not bother with having to call Clear().

    
    

    
  
Comment on attachment 135354 [details]
nsTableUnderlay.h

One more thing.  The various members here need documenting....	It's not clear
what role mOrigin plays, what mCols is used for, what mRow and mRowGroup are
used for, what mZeroBorder and mZeroPadding are supposed to do, etc.

    
    

    
  
Comment on attachment 135353 [details] [diff] [review]
patch

>Index: mozilla/layout/html/table/src/nsTableFrame.h

>+  /** Get width of table + colgroup + col collapse: elements that
>+   *  continue along the length of the whole left side.
>+   *  @param aPixelsToTwips - conversion factor
>+   *  @param aGetInner - get only inner half of border width
>+   */

It's not clear to me what this does, still... This is getting the width of
_which_ border?  The left one?	The left border of what element?  Why
"continuous"?

This would benefit greatly from an example, I think.

>+    PRUint32 mLeftContBCBorder:8;

Comment that this, like other BC code, rather arbitrarily limits the widths of
BC borders?

That said, why is this 8 bits?	I thought BC borders were currently limited to
6 bits?

I've gotten up to the beginning of nsTableFrame::Paint() for now; more later.

    
    

    
  
> Why isn't that a pointer?  References to interfaces like that make me queasy.
...
> Could you use a pointer to the nsIRenderingContext instead?

Because everything else refers to it that way and I'm copying them. Would you
rather have a pointer?

> Why? If this class always has a frame, it can get the prescontext off the
frame.

I didn't think of that..

If it's in the frame, then why is it being passed around the paint functions?

> Unless mFrame can be null?

hm.. Don't think it would have a generated borderstyle if it did.

> Any reason not to just inline this into the header?

I wanted to keep all the implementation logic in one file.
(It's not just returning a value; it's making a judgement.)

>>+  /** Get width of table + colgroup + col collapse: elements that
>>+   *  continue along the length of the whole left side.
>>+   *  @param aPixelsToTwips - conversion factor
>>+   *  @param aGetInner - get only inner half of border width
>>+   */
>
>It's not clear to me what this does, still... This is getting the width of
>_which_ border?  The left one?	The left border of what element?  Why
>"continuous"?

The left table border (nsTABLEFrame::GetContinuousLEFTBCBorder)--which is also
the leftmost column's left border, which is also the leftmost colgroup's left
border. It is taking the width of the "table + colgroup + col collapse" -- i.e.
the border's width before it collapses with the rowgroups/rows/cells. Continuous
because the table/colgroup/col elements necessarily extend across the entire
length of the table from top to bottom. (Rowgroup/row/cell borders do not: there
can be multiple rowgroup/row/cell borders along this edge of the table.)

> I thought BC borders were currently limited to 6 bits?

http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/nsTableCellFrame.h#483

Thanks for all the comments, bz. I'll deal with the rest of them as I fix the
problems they point out. :)

    
    

    
  
> Because everything else refers to it that way and I'm copying them.

In that case, I guess keep it as-is (if you're getting passed
nsIRenderingContext& yourself in the paint code; I've not looked at that yet).

> The left table border (nsTABLEFrame::GetContinuousLEFTBCBorder

Put all this in the relevant comments in the patch, please.  ;)

    
    

    
  
I almost forgot... The reason the prescontext is passed around and all that is
that there didn't use to be an accessor for it off the frame.  But there is one
now, and we're slowly moving to eliminating prescontexts from all sorts of
function signatures.

    
    

    
  
> Why "nsTableUnderlay"? Wouldn't "nsTableBackgroundPainter" be a better
> reflection of what's going on?

*thinks back* I put that there because the header includes border-related things
as well. Suppose you explain why bug 149378 is blocked by this. Should I change
it to nsTablePainter?

(In terms of things in the distant-and-likely-nonexistant-future, I've also
contemplated a redesign of BC handling that would find the caching features of
the painter most useful. ^^)

    
    

    
  
> Suppose you explain why bug 149378 is blocked by this.

Because the measurements there should be redone once this patch goes in, since
it will significantly change the control flow of table painting.

> Should I change it to nsTablePainter?

That may be a better description of what it does, yes.

    
    

    
  
Comment on attachment 135353 [details] [diff] [review]
patch

>Index: mozilla/layout/html/table/src/nsTableFrame.cpp
> nsTableFrame::Paint(nsIPresContext*      aPresContext,
>+      if (eCompatibility_NavQuirks != mode) {

You need a comment right before this check explaining why the check is made
(that is, what the behavior difference should be between the modes, and why
this behavior difference is desired).  It looks like we're just doing our old
painting in quirks mode and CSS2 painting in standards mode; is there a good
reason for this quirk?

>+        rv = painter.PaintTable(this, (nsTableRowGroupFrame*)rowGroups.ElementAt(0),
>+                                (nsTableRowGroupFrame*)rowGroups.ElementAt(numRowGroups-1));

What if numRowGroups is 0?  Eg. an XHTML <table> with no <tbody> and no rows...

> +          if (rg->GetView()) {

GetView() is slow.  This is why we have HasView().

> +              if (row->GetView()) {

Same.

>+              else if (aDirtyRect.YMost() > rgRect.y + rect.y) {

Why that test instead of an Intersects() test (with an appropriately translated
rect)?	Won't this trigger painting of rows/cells that come completely before
the dirty rect?

Also, why ">" instead of ">="?

>+                  rv = painter.PaintCell(cell, (PRBool)cell->GetView());

HasView()

> +          OrderRowGroups(rowGroups, numRowGroups);
> +          rv = painter.PaintTable(this, (nsTableRowGroupFrame*)rowGroups.ElementAt(0),
> +                                  (nsTableRowGroupFrame*)rowGroups.ElementAt(numRowGroups-1),

Again, what if numRowGroups == 0?

>+nsTableFrame::SetColumnDimensions(nsIPresContext* aPresContext,

Comment clearly in here why the cellSpacingY affects all these heights, please.
 Also comment why cellspacing affects origins.

>+    if (colGroupWidth) {
>+      colGroupWidth -= cellSpacingX;
>+    }

Comment that you are doing this to cancel out the extra cellSpacingX we counted
for the last column?

>@@ -5740,16 +5816,17 @@ nsTableFrame::CalcBCBorders(nsIPresConte

>+        CalcDominateBorder(this, cgFrame, colFrame, nsnull, nsnull,
>+                           nsnull, PR_TRUE, NS_SIDE_RIGHT, PR_TRUE, t2p, //XXXfr why true?

You added this code... why did you use true?  Add a clearer XXX comment if
needed.

>@@ -5878,16 +5992,28 @@ nsTableFrame::CalcBCBorders(nsIPresConte

>+                           nsnull, PR_TRUE, NS_SIDE_RIGHT, PR_TRUE, t2p, //XXXfr why true?

Same.

>+                           nsnull, PR_TRUE, NS_SIDE_RIGHT, PR_TRUE, t2p, //XXXfr why true?

Same.

I got up to the "@@ -6087,26 +6228,46 @@
nsTableFrame::CalcBCBorders(nsIPresConte" part.  More later.

    
    

    
  
Comment on attachment 135353 [details] [diff] [review]
patch

I'm not sure I can usefully review your CalcDominateBorder calls, btw... if you
could have bernd look them over, that would be great.


>@@ -6087,26 +6228,46 @@ nsTableFrame::CalcBCBorders(nsIPresConte
>+      if (!gotRowBorder && 1 == info.rowSpan) {
>+        //get continuous row/row group border

This could use a nice long comment explaining _why_ you're doing this, not just
what you're doing.  It's not obvious to me what "gotRowBorder" means at this
point, what info.rowSpan really means, and why you care about this particular
condition.

>Index: mozilla/layout/html/table/src/nsTableRowGroupFrame.h

>+  // border widths in pixels in the collapsing border model
>+  unsigned mRightContBorderWidth:8;
>+  unsigned mBottomContBorderWidth:8;
>+  unsigned mLeftContBorderWidth:8;

Any reason not to just make these PRUint8?

Also, I assume we have very few frames of this type around?  Otherwise adding
members to them is not so great...  If this is something that will be used only
rarely, perhaps it could be stored as a property of the frame.	If it's
something that needs to be accessed often, I guess we need to byte the bullet
and just store this?

>+inline nscoord
>+nsTableRowGroupFrame::GetContinuousBCBorderWidth(float     aPixelsToTwips,

Is there a good reason to inline this?	It seems like a nontrivial bit of code,
especially if it's called often.

>Index: mozilla/layout/html/table/src/nsTableRowGroupFrame.cpp

>+  if (NS_FRAME_PAINT_LAYER_BACKGROUND == aWhichLayer &&
>+      !(aFlags & (NS_PAINT_FLAG_TABLE_BG_PAINT | NS_PAINT_FLAG_TABLE_CELL_BG_PASS))) {

This could use a comment explaining what exactly this check means (as far as I
can tell, "Paint() called directly because we have a view" or something like
that.

>+        if (row->GetView()) {

HasView()

>+        else if (aDirtyRect.YMost() > rect.y){

Again, why this check as opposed to an intersects check?

>+            rv = painter.PaintCell(cell, (PRBool)cell->GetView());

HasView()

This whole chunk of code duplicates code in the inner parts of the loops in
nsTableFrame.  Is there really no way to push this code down into the painter? 
It could decide whether to run the outer part of the loop based on what the
origin is or something...  In fact, here you would just call PaintRowGroup() on
the painter, and the loop over the rows logic (as well as checking for views)
could live in the PaintRowGroup() method, no?

I suppose you're trying to have the painter present a more flexible interface,
but do we really envision using the extra flexibility?	Would there ever be a
case when one _would_ want to paint kids with views in this sort of situation?

>+void nsTableRowGroupFrame::SetContinuousBCBorderWidth(PRUint8 aForSide,

Will this never get called with NS_SIDE_RIGHT?	If so, add a debug-only case
with an NS_ERROR to that effect.

Also, as written this will trigger unhandled value warnings; you may want to
add an empty "default" to this switch (and add a return to the third case!).

>Index: mozilla/layout/html/table/src/nsTableRowFrame.h

>+  unsigned mRightContBorderWidth:8;
>+  unsigned mTopContBorderWidth:8;
>+  unsigned mLeftContBorderWidth:8;

This is increasing the size of nsTableRowFrame, and I _know_ we have lots of
those.... is there really no way not to store this state?  Perhaps we should
have a BC-only class for this like we do for table cells?

>+inline void
>+nsTableRowFrame::GetContinuousBCBorderWidth(float     aPixelsToTwips,

>+inline nscoord nsTableRowFrame::GetOuterTopContBCBorderWidth(float aPixelsToTwips)

Again, why inline the first of these?

Is there any reason these are two separate functions for table rows but
combined into a single function for table row groups?  It would be better to be
consistent, imo.

>Index: mozilla/layout/html/table/src/nsTableRowFrame.cpp
>+  if (NS_FRAME_PAINT_LAYER_BACKGROUND == aWhichLayer &&
>+      !(aFlags & (NS_PAINT_FLAG_TABLE_BG_PAINT | 
NS_PAINT_FLAG_TABLE_CELL_BG_PASS))) {

Same comment as for nsTableRowGroupFrame

>+        rv = painter.PaintCell(cell, (PRBool)cell->GetView());

HasView().

Once again, this would be better in the painter, imo...

>+void nsTableRowFrame::SetContinuousBCBorderWidth(PRUint8 aForSide,

Same comments as for table row groups.

>Index: mozilla/layout/html/table/src/nsTableColGroupFrame.h

>+  PRUint32 mTopContBorderWidth:8;
>+  PRUint32 mBottomContBorderWidth:8;

PRUint8.

>Index: mozilla/layout/html/table/src/nsTableColGroupFrame.cpp

>+void nsTableColGroupFrame::SetContinuousBCBorderWidth(PRUint8 aForSide,

Similar comments as for row groups.

>Index: mozilla/layout/html/table/src/nsTableColFrame.h

>+  // border width in pixels
>+  PRUint32 mLeftBorderWidth:  8;
>+  PRUint32 mRightBorderWidth: 8;
>+  PRUint32 mTopContBorderWidth:8;
>+  PRUint32 mRightContBorderWidth:8;
>+  PRUint32 mBottomContBorderWidth:8;

PRUint8.  I guess we don't have as many colframes as rowframes in really big
tables, so this may be OK....

>+inline nscoord
>+nsTableColFrame::GetContinuousBCBorderWidth(float     aPixelsToTwips,
>+                                            nsMargin& aBorder)

Again, I don't think this should be inlined.

>Index: mozilla/layout/html/table/src/nsTableColFrame.cpp

>+void nsTableColFrame::SetContinuousBCBorderWidth(PRUint8 aForSide,
>+                                                 nscoord aPixelValue)

Same comments as for row groups, etc.

>Index: mozilla/layout/html/table/src/nsTableCellFrame.cpp
>-  	        nsILookAndFeel* look = nsnull;

Hey, if you're touching this code anyway, make the thing an nsCOMPtr, eh?

>@@ -417,17 +418,17 @@ nsTableCellFrame::Paint(nsIPresContext* 
>-  PRBool paintChildren   = PR_TRUE;
>+  PRBool paintChildren = PR_TRUE;

Why make changes like that?  Just muddles up the CVS history....

>@@ -435,17 +436,17 @@ nsTableCellFrame::Paint(nsIPresContext* 

>-  
>+

Same.

>@@ -1363,18 +1364,19 @@ nsBCTableCellFrame::PaintUnderlay(nsIPre

>+  if (aVisibleBackground &&
>+      (!(aFlags & NS_PAINT_FLAG_TABLE_BG_PAINT) ||
>+        (aFlags & NS_PAINT_FLAG_TABLE_CELL_BG_PASS))) {

Again, need a comment as to what that conditional actually means...

>@@ -1389,11 +1391,11 @@ nsBCTableCellFrame::PaintUnderlay(nsIPre
>+  aFlags &= ~ (NS_PAINT_FLAG_TABLE_CELL_BG_PASS | NS_PAINT_FLAG_TABLE_BG_PAINT);

Why do table cells need to reset this flag?  It's not clear what that will do,
just by looking at this code.  Comment, please.

    
    

    
  
bernd, if you have thoughts of any kind on using a BC-only table row class that
stores the extra border state, please comment...  how well is that working out
for table cells?

    
    

    
  
Boris, thanks for your careful review, it shows that comment 121 was correct. BC
table cell frames work. I never touched them (Maybe thats the reason why they
work). I like the idea to hide the bc burden from ordinary rows in the separate
border case.

I will review the CalcDominateBorder calls.

Btw the empty table background case is bug 227123, and mozilla currently does
the right thing, so please no regressions....

    
    

    
  
s/CalcDominateBorder/CalcDominantBorder/g ?

/be

    
    

    
  
As a separate patch, sure.  This one is already pretty big....

    
    

    
  
I would like to propose to spin of the border collapse part into a separate
patch. It will otherwise jeopardize the 1.7a timetable for the whole patch.
The border collapse review issues are not new
(http://bugzilla.mozilla.org/show_bug.cgi?id=191731#c7) and when bz (comment
142) and dbaron ( http://bugzilla.mozilla.org/show_bug.cgi?id=225266#c8 ) try to
avoid to review this code it sends out a clear message. And it is really hard to
argue when looking at an undocumented function with 14 arguments like
CalcDominantBorder
(http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/nsTableFrame.cpp#5146)
 that it might cause comments like //XXXfr why true?
To summarize I would prefer if this function can be reorganized before any
further calls are added. I filed bug 229883 for this.

@@ -927,30 +931,31 @@ public:
 
+    PRUint32 mLeftContBCBorder:8;
+    PRUint32 : 11;                     // unused
   } mBits;

Why are mLeftContBCBorder not a member of the bcProperty like the other bc
widths on a table?

this would also eliminate the my objection against 

@@ -1101,16 +1106,25 @@ inline void nsTableFrame::SetBorderColla

+inline nscoord
+nsTableFrame::GetContinuousLeftBCBorderWidth(float  aPixelsToTwips,
+                                             PRBool aGetInner) const

why isn't there a Set method.

@@ -5782,27 +5859,53 @@ nsTableFrame::CalcBCBorders(nsIPresConte
+        mBits.mLeftContBCBorder = ownerWidth;

this should never happen, mBits should not accessed so directly.


Why are the widths for the table not reset together with the other table widths?
if (!tableBorderReset[NS_SIDE_TOP]) {
5738         propData->mTopBorderWidth = 0;
5739         tableBorderReset[NS_SIDE_TOP] = PR_TRUE;
5740       }

A continuous bc border is that the thinnest continuous line? If yes
+        //get row continuous borders
+        CalcDominateBorder(this, info.cg, info.leftCol, info.rg, rowFrame,
nsnull, PR_TRUE, NS_SIDE_LEFT,
+                           PR_FALSE, t2p, owner, ownerBStyle, ownerWidth,
ownerColor);
+        rowFrame->SetContinuousBCBorderWidth(NS_SIDE_LEFT, ownerWidth);
Why are cell frames here excluded from the computation? Imagine that they have
border style hidden.
I would expect more some minimum mechanism. My impression is that the exclusion
of frames at the CalcDominantBorder calls will create problems once a hidden
style is defined on any of those excluded frames.

    
    

    
  
> To summarize I would prefer if this function can be reorganized before any
> further calls are added. I filed bug 229883 for this.

If this is done (and it would be fine with me), I would say the calls should
still be added per this patch, just ifdefed out.  That way lxr searches will
show it when bug 229883 is being fixed (and they can be fixed to do the right
thing then).

    
    

    
  
> I would like to propose to spin of the border collapse part into a separate
> patch. It will otherwise jeopardize the 1.7a timetable for the whole patch.

You want me to extricate the border collapse parts so that we check in one
behavior (ignoring borders) and during the next release cycle (1.8a) change it
to another (accounting for borders)??


Right now, my main concern wrt release timetables is whether I'll be able to
handle all these comments in time for the freeze... :) (I won't have time to
work on it this week.)


> A continuous bc border is that the thinnest continuous line?

No, it is the result of collapsing all the continuous borders on that edge.
For vertical lines, this is table, colgroup, column.
For horizontal lines, this is table, rowgroup, row.

This is done so that backgrounds will line up along the edge.

Cell borders are not continuous on a line: they are segments along it.
Therefore they don't get counted. For example, setting a thick border
on the leftmost cell should not shift the row background over; this way
a striped background set on <tr> will line up across rows even if the
cells are assigned arbitrary border widths.

I really ought to write this up.
Blocks: 230133

    
    

    
  
For the record, the tests mentioned on this bug so far are currently at:
   http://fantasai.tripod.com/www-style/2002/table-backgrounds/tests/
   http://dbaron.org/css/test/sec170501
   http://dbaron.org/css/test/sec170501a
   http://dbaron.org/css/test/sec170501b

It would be great if we could get some CSS2.1-test-case-guideline-compliant 
versions of those tests for the CSS2.1 test suite. :-)
   http://www.w3.org/Style/CSS/Test/guidelines.html
Blocks: 232101
Blocks: 232108
Blocks: 191567

    
    

    
  

      
      
      
      

        Attached file
          
        nsTablePainter.h (not quite done) (obsolete)
        — 
      

    


  

    
    

    
  

      
      
      
      

        Attached file
          
        (not quite done) (obsolete)
        — 
      

    


  
Latest patch will be up at
http://fantasai.tripod.com/Mozilla/bugs/4510/patch.txt

I'm not quite done, but nsTablePainter.* are, minus some comments, mostly ready
for review. I'm still going over the nsTableFrame portions of the patch. I'm
extremely busy tomorrow, though, so I thought I'd post these for now in case
you (bz, bernd) wanted to look it over sooner rather than later.

Responses to some of the comments:


> Is there any reason these are two separate functions for table rows but
> combined into a single function for table row groups?  It would be better
> to be consistent, imo.

Return value is void now, so n/a.


 > Comment clearly in here why the cellSpacingY affects all these heights,
please.
 > Also comment why cellspacing affects origins.

 That should be documented in CSS2.1

 >+	   CalcDominateBorder(this, cgFrame, colFrame, nsnull, nsnull,
 >+			      nsnull, PR_TRUE, NS_SIDE_RIGHT, PR_TRUE, t2p,
//XXXfr why true?
 >
 > You added this code... why did you use true?

 I just guessed based on what I saw elsewhere. I don't know what it does,
 which is why I ask.

> Is there a good reason to inline this?  It seems like a nontrivial bit of
> code, especially if it's called often.

 It's only called once.

> This is increasing the size of nsTableRowFrame, and I _know_ we have lots
> of those....

 As discussed, I'll file a bug on moving that into a frame property.

> -  PRBool paintChildren   = PR_TRUE;
> +  PRBool paintChildren = PR_TRUE;
>
> Why make changes like that?  Just muddles up the CVS history....

neater code for the next person to read?

    
  

        Attachment #135354 -
        Attachment is obsolete: true

        Attachment #135355 -
        Attachment is obsolete: true

    
    

    
  
> That should be documented in CSS2.1

Then point to the relevant spec section here....  That at least tells people
where to look for the reasons for that code.
Blocks: 233532

    
    

    
  
@@ -5731,27 +5750,53 @@ nsTableFrame::CalcBCBorders(nsIPresConte
.
.
.
+CalcDominantBorder(this, cgFrame, colFrame, nsnull, nsnull,
+                   nsnull, PR_TRUE, NS_SIDE_RIGHT, PR_TRUE, t2p, //XXXfr why true?

so what does this code do IMHO,
it looks for the table, the colgroupframe and colframe It queries for all of
them right border. If the borders are the result of the rules attribute then
they should be ignored. Furthermore the owner should be the adjacent frame.
Afterwards only the owner width is used. If the request is going for the last
PR_TRUE then the answer is it doesnt matter, you need the owner only when you
store in the cellmap. If this request is for the first PR_TRUE then it really
matters. So we are at the top row and we query for the right side of these
frames, only for the rightmost edge we are certain that we hit an outer edge of
the table for all others we know that we are inside the table. So in once case
this should be PR_TRUE and in the majority of cases PR_FALSE. In the light of
bug 43178 I think my comment 147 makes sense because it would free you from
fixing this.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
newest nsTablePainter is up at
http://fantasai.tripod.com/Mozilla/bugs/4510
I'll attach it once bz says its good enough :)

    
  

        Attachment #135353 -
        Attachment is obsolete: true

        Attachment #140946 -
        Attachment is obsolete: true

        Attachment #140947 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 141121 [details] [diff] [review]
patch

forgot to mention: the only difference between the nsTablePainter files
previously attached and the ones I just put up is the comments and a couple
lines I swapped in PaintCell (moved the get colIndex part below the empty-cells
short-circuit).

        Attachment #141121 -
        Flags: superreview?(bzbarsky)

        Attachment #141121 -
        Flags: review?(bernd_mozilla)

    
    

    
  
Comments on the separate files:

> nsTablePainter.h

How about calling it mozTableBackgroundPainter.h?  ;)

>    /* aPass params indicate whether to paint the element or to just pass
through and

They're called aPassThrough. Change the comment accordingly.

>     * See Public versions for function descriptions

Why not just put these private methods down with the other private methods and
skip this part?

>    /** Paint table elements' backgrounds down through table cells
>      * (Cells themselves will only be painted in border collapse)

Maybe better said as "Paint background for the table frame and its children
down through the cells (Cells ... )

>    /** Paint table row group elements' backgrounds down through table cells
>      * (Cells themselves will only be painted in border collapse)

And then this could be "Paint background for the table row and its children
down....."

>    /** Paint table row elements' backgrounds down through table cells
>      * (Cells themselves will only be painted in border collapse)

Likewise.

Feel free to ignore these comments if you think what you wrote is clearer, of
course.  ;)

>      /** Calculate and set data all values to represent aFrame (which must be
non-null) */

"and set all data values", perhaps?

>      /** True if need to set border-collapse border; must call Set beforehand */
>      PRBool ShouldSetBCBorder();

Which Set?  SetFull()?  SetFrame()?  SetBCBorder()?  Some subset thereof?

>      private:
>        nsStyleBorder* mSynthBorder;

Weird indent; that should line up with the other members.

>    nsRect               mCellRect; //current cell

"current cell rect"

> nsTablePainter.cpp

Same naming issue (make it match the classname).

>TableBackgroundPainter::TableBackgroundData::SetBCBorder(nsMargin& aBorder,

>    mSynthBorder = new (aPainter->mPresContext)
>                        nsStyleBorder(aPainter->mZeroBorder);

So... mZeroBorder is just an optimization or something?  This is the only thing
it's used for, and you could as easily do

     mSynthBorder = new (aPainter->mPresContext)
                         nsStyleBorder(aPainter->mPresContext);

right?

>TableBackgroundPainter::TableBackgroundPainter(nsTableFrame*        aTableFrame,

>  mZeroBorder.SetBorderStyle(NS_SIDE_TOP, NS_STYLE_BORDER_STYLE_BLANK);

Use STYLE_NONE here.  dbaron just removed NS_STYLE_BORDER_STYLE_BLANK anyway.

>TableBackgroundPainter::PaintTableFrame(nsTableFrame*         aTableFrame,

>  tableData.mRect.MoveTo(0,0);

Add "// Put tableData.mRect in the table frame's coordinate system" before that
line.

>    if (aFirstRowGroup && aLastRowGroup && mNumCols > 0) {
>    //only handle non-degenerate tables; we need a more robust BC model
>    //to make degenerate tables' borders reasonable to deal with

Weird indent.

>      aLastRowGroup->GetContinuousBCBorderWidth(mP2t, tempBorder);
>      border.bottom = tempBorder.bottom;
>
>      nsTableRowFrame* rowFrame = aFirstRowGroup->GetFirstRow();
>      if (rowFrame) {

Why use the row for top and the rowgroup for bottom?  Is there a reason? If so,
comment it here.

>      border.left = aTableFrame->GetContinuousLeftBCBorderWidth(mP2t, PR_TRUE);
Likewise -- why use the table's left border but the col's right border above?

>      nsresult rv = tableData.SetBCBorder(border, this);
>      if (NS_FAILED(rv)) {
>        return rv;
>      }

Need to do "tableData.Destroy(mPresContext);" before returning, no?

>TableBackgroundPainter::QuirksPaintTable(nsTableFrame* aTableFrame,
>      if (NS_FAILED(rv)) return rv;
>      if (rgRect.Intersects(mDirtyRect) && !rg->HasView()
>          && NS_SUCCEEDED(rv) && isVisible) {

No need for the NS_SUCCEEDED check -- it can't fail.  ;)

>          if (NS_FAILED(rv)) return rv;
>          if (mDirtyRect.YMost() > rowRect.y && !row->HasView()
>              && NS_SUCCEEDED(rv) && isVisible) {

Again, no need for the NS_SUCCEEDED check.

Why checking mDirtyRect.YMost() > rowRect.y instead of checking whether the
rects intersect?  Due to rowspanning cells?  If so, please add a short comment
to that effect.

>TableBackgroundPainter::PaintTable(nsTableFrame* aTableFrame)

>    mCols = new ColData[mNumCols];

This allocates mCols.  Since ColData has no constructor, the mColGroup pointers
in all those are uninitialized.

>      /*Create data struct for column group*/
>      cgData = new TableBackgroundData;
>      if (!cgData) return NS_ERROR_OUT_OF_MEMORY;

Now some of the points in mCols are garbage and when you do the deletion loop
over mCols in the destructor you will probably crash.  I'd add a constructor to
ColData that inits the mColGroup pointer to 0.

This looks great, fantasai!  Thank you very much for the clear up-front
explanation of what this class is trying to do!  Once you make the changes I
just mentioned, these two files are good by me.

    
    

    
  
> How about calling it mozTableBackgroundPainter.h?  ;)

Scratch that.  mozTableBgPainter.h/cpp, please.  The other is just too long.

    
    

    
  
Comment on attachment 141121 [details] [diff] [review]
patch

>Index: mozilla/layout/html/table/src/nsTableCellFrame.cpp

>-NS_METHOD 
>+NS_METHOD
> nsTableCellFrame::Paint(nsIPresContext*      aPresContext,

NS_IMETHODIMP, as long as you're changing this.

Other than that, looks good to me.  sr=bzbarsky.

Don't forget to change Makefile.in if when you rename nsTablePainter.cpp

        Attachment #141121 -
        Flags: superreview?(bzbarsky) → superreview+

        Attachment #135353 -
        Flags: superreview?(bzbarsky)

        Attachment #135353 -
        Flags: review?(dbaron)

    
    

    
  
{ns/moz/}TableBackgroundPainter.{cpp,h} seems like fine names to me.  The length
isn't really a problem.
 
Here are comments on the patch, although I skipped
nsTableFrame::CalcBCBorders because it makes my head hurt, and I haven't
looked through the new files thoroughly yet.
 
nsStyleStruct.cpp:
  This should use a loop (probably the FOR_CSS_SIDES macro, which could
  be moved from nsCSSStruct.h to nsStyleConsts.h).  But since that
  requires changing files in directories you're not otherwise touching,
  and should probably be done with a bit of other cleanup, I filed bug
  233795 to remind myself to do it later.
 
nsCSSRendering.h/cpp:
  aClipRect should probably be called aBGClipRect, since aClipRect is a
  reasonably standard name meaning something else
 
nsTableFrame.h:
  Limiting BC borders to 63 pixels is wrong, and we shouldn't spread
  that elsewhere.  (Same for nsTableRowFrame.h, nsTableRowGroupFrame.h,
  nsTableColFrame.h, nsTableColGroupFrame.h.)
  We should at least have a typedef for a pixel size of a BC border so
  we can find all the places that use PRUint8 or :8.
 
  Shouldn't GetContinuousLeftBCBorderWidth use the
  BC_BORDER_LEFT_HALF_COORD macro for symmetry, and in case the macros
  change?
 
nsTableFrame.cpp:
 
  I dislike the NS_PAINT_FLAG_* business:
 
    NS_PAINT_FLAG_TABLE_CELL_BG_PASS is never set, so it can be removed.
 
    NS_PAINT_FLAG_TABLE_BG_PAINT can be replaced with a function on each
    inner table element class that checks if there's a view between that
    element (inclusive) and the table element (exclusive).  But that is
    probably better done in a later patch (since it would allow the aFlags
    parameter to be removed throughout layout, I think).
 
  The nsTableBackgroundPainter code within nsTableFrame.cpp probably
  shouldn't be conditioned on the table having visibility visible
  (although that currently means the NS_PAINT_FLAG_TABLE_BG_PAINT flag
  isn't set).  Instead, the nsTableBackgroundPainter code should be
  called unconditionally (which it looks like it will handle just fine).
  Likewise for the descendants with views that use it.
 
 
Your use of the terms "direct" and "table-called" in comments in three
places (nsTableRowGroupFrame::Paint, nsTableRowFrame::Paint,
nsBCTableCellFrame::PaintUnderlay) is really making up terminology where
we don't need extra terminology.  What's happening is that an internal
table element has a view (which, for table cells, can be caused by
relative positioning, opacity, etc.).  You should use the term "view"
somewhere in those comments and get rid of the terms "direct" and
"table-called".
 
In the code in nsTableRow(Group)?Frame that creates a
TableBackgroundPainter, do you want to set the flag tell the rows /
cells not to paint?  Likewise (as I said before), these calls probably
shouldn't be conditional on visibility.
 
nsTableColGroupFrame.cpp:
 
  nsTableColFrame guarantees not touching aBorder.left, so you don't
  need to do the fancy save-and-reassign dance in
  GetContinuousBCBorderWidth.
 
nsTableColFrame:
 
  GetContinuousBCBorderWidth can return void -- nobody uses the result.
 
nsTableCellFrame:
 
  I'm guessing Bernd wants you to remove a bunch of the
  PaintUnderlay-related changes, since they break
  NS_STYLE_TABLE_EMPTY_CELLS_SHOW_BACKGROUND, although removing the
  NS_PAINT_FLAG_TABLE_CELL_BG_PASS flag will allow you to remove the
  aPaintChildren argument.  The comments you added about the flags are
  backwards, but you should notice that when you remove the unneeded
  one.  Also, you're probably better in passing the value of
  GetStyleTableBorder()->mEmptyCells instead of a boolean and the const
  nsStyleTableBorder* (to get mEmptyCells again, just like for the
  boolean).
 
nsTableBackgroundPainter.cpp (just the parts I've read in order to read
the rest of the patch):
 
  In SetFull, is it really correct to ignore the border if visibility is
  hidden?  That seems wrong.
 
  TableBackgroundPainter::QuirksPaintTable incorrectly applies
  visibility on row groups to rows and cells and visibility on rows to
  cells.
 
  TableBackgroundPainter::QuirksPaintTable compares the frame's rect to
  the dirty area when it needs to use the overflow area (unioned with
  the rect, or something -- it's messy).
 
  "<dfn>passed</dfn>" seems like a bad term to use because one already
  passes arguments to functions and paints in multiple passes.  How
  about skipped (which conflicts only with aSkipSides)?  Or just "has
  view"?  Likewise aPassThrough / aPass should probably be renamed to
  aSkip or aHasView.
 
  The assertion at the beginning of PaintRow calls itself PaintRowGroup.
  Also, in both those assertions, "Quirks" isn't a caller, so you should
  probably say "must not call ... in quirks mode".
 
  Have you tested that moving a window over your testcases does the
  right thing?  Try multiple directions, and try movement that's exactly
  horizontal or exactly vertical?  I find the inconsistency in the ways
  you handle coordinate systems between cells/ rows / row groups /
  tables to be confusing, and confusion can easily lead to bugs.
  Perhaps it's better to be consistent with what the rest of the code
  does (always translate, i.e., get rid of mCellRect and call
  TranslateContext for rows as well)?  Perhaps it's OK the way it is,
  but this really seems like premature optimization (which is said to be
  the root of all evil).
 
Could you explain the logic of the changes (which I'm sure are
connected) to:
 * the nsTableReflowState constructor
 * nsTableFrame::SetColumnDimensions
 * nsTableRowFrame::ReflowChildren
 
 
Also, if you want to include added files in your diff, you can just add the
following lines to the end of (or anywhere within)
layout/html/table/src/CVS/Entries:
 
/nsTableBackgroundPainter.h/0/dummy timestamp//
/nsTableBackgroundPainter.cpp/0/dummy timestamp//
 
Once you do that, if you add -N to the options you pass to diff (i.e.,
diff -Npu8), they should show up in the diff.


    
    

    
  
As fantasai points out, NS_PAINT_FLAG_TABLE_CELL_BG_PASS is set in the painter,
so forget the bit about removing the flags.

However, this means nsTableCellFrame::Paint needs to set PaintChildren based on
it (just like it now does in one of the two implementations of PaintUnderlay) --
i.e., you need to pull that bit out of the BC version of PaintUnderlay.

    
    

    
  
 
> So... mZeroBorder is just an optimization or something?  This is the only 
thing 
> it's used for, and you could as easily do 
>     mSynthBorder = new (aPainter->mPresContext) 
>                         nsStyleBorder(aPainter->mPresContext); 
> right? 
 
Won't work; I need to give the border a width. 
STYLE_NONE makes it zero-width. And the default width is medium. 
 

    
    

    
  
(In reply to comment #162)

> Won't work; I need to give the border a width.

What do you mean?  Don't you explicitly assign some border widths immediately
after that?

    
    

    
  
STYLE_NONE means there's no border; i.e. the border width must calculate to zero
no matter what it's specified value.

    
    

    
  
Oh, hm... So do we need to restore STYLE_BLANK then?

    
    

    
  
I looked at the bc part between
http://bugzilla.mozilla.org/attachment.cgi?id=141121&action=diff#mozilla/layout/html/table/src/nsTableFrame.cpp_sec11
and 
http://bugzilla.mozilla.org/attachment.cgi?id=141121&action=diff#mozilla/layout/html/table/src/nsTableFrame.cpp_sec19
They look OK to me.

I am not too worried about NS_STYLE_TABLE_EMPTY_CELLS_SHOW_BACKGROUND changes
the only wrong place I already mentioned to fantasai. Just for the record
NS_STYLE_TABLE_EMPTY_CELLS_SHOW_BACKGROUND is only available in quirks mode. It
emulates the NN4 which has shown the background for empty cells but not the
border. NS_STYLE_TABLE_EMPTY_CELLS_SHOW_BACKGROUND is used as default, as soon
as empty-cells is either shown or hide this quirk doesn't apply.

>Here are comments on the patch, although I skipped
>nsTableFrame::CalcBCBorders because it makes my head hurt

You are of course involved in the offer I made to boris, if you rereview the
2300 lines of bc code like you did with the overflow stuff, I will bring the
code in accordance to your review comments. This seems to me necessary as
r=alexsavulov for this type of patch did not ensure  at least readability of the
code. I filed already bug 229883 and attached a patch to the bug, to get rid of
the large numbers of arguments to CalcDominantBorder. But I am weak at these
general style coding stuff.

    
    

    
  
I think STYLE_SOLID will work; PaintBackground doesn't paint the borders, it
just uses them for image positioning.

    
    

    
  
> STYLE_NONE means there's no border; i.e. the border width must calculate to 
> zero no matter what it's specified value.

It must _compute_ to zero, even, so this should happen during the cascade.

e.g.:

   div { border: none 2em red; }
   span { border: inherit; border-style: solid; }

   <div><span> test </span></div>

...should not have any border. (We currently do this wrong. It works in Opera.)

    
    

    
  
Ian, that is off-topic, and this is a long bug. But since you bring it up:
  http://lists.w3.org/Archives/Public/www-style/2004Feb/0160.html

Blocks: 171523

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
> Your use of the terms "direct" and "table-called" in comments in three

The significance of having a view is that it means ::Paint is not
called by an ancestor table element, which would have painted the
element's background for it. Therefore, I prefer to keep those
comments intact. I have, however, added a note about views in the
flag declarations.

> nsTableColFrame::GetContinuousBCBorderWidth can return void -- nobody uses
the result.

The painter does.

>  In SetFull, is it really correct to ignore the border if visibility is
>  hidden?  That seems wrong.

Why? If we're not painting the background, then we have no need for
the border.

> TableBackgroundPainter::QuirksPaintTable compares the frame's rect to
> the dirty area when it needs to use the overflow area

To the best of my knowledge, backgrounds never overflow their respective
elements' mRects.

> "<dfn>passed</dfn>" seems like a bad term to use.. aSkip

Skip to me implies that the entire section will be skipped
over. PassThrough emphasizes that we're going through the
section.

Changed "passed" to "passed through".

> Have you tested that moving a window over your testcases does the
> right thing?

Yes.

> Could you explain the logic of the changes (which I'm sure are
> connected) to:

Makes the row and column dimensions start at the first cell border
edge rather than the table edge. See "Background Boundaries" in
http://fantasai.tripod.com/www-style/2002/table-backgrounds/

    
  

        Attachment #141121 -
        Attachment is obsolete: true

    
  

        Attachment #141851 -
        Flags: superreview?(dbaron)

        Attachment #141851 -
        Flags: review?(bernd_mozilla)

    
    

    
  
I think you should revert the GetRowGroupFrame change or modify it to compile on
win with VC.

e:/moz_src/mozilla/layout/html/table/src/nsTableFrame.cpp(1233) : error C2724:
'GetRowGroupFrame' : 'static' should not be used on member functions defined at
file scope 
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/vccore/html/C2724.asp

    
    

    
  
The trick is that for static member functions you're supposed to say |static|
when you declare it but not when you define it.

    
    

    
  
somehow VC++ doesnt like the  access to the private member variables
mPresContext and mZeroBorder at the following snippet. My first guess is that
you either need a get function or change the arguments of this function.

nsresult
TableBackgroundPainter::TableBackgroundData::SetBCBorder(nsMargin& aBorder,
                                                         TableBackgroundPainter*
aPainter)
{
  NS_PRECONDITION(aPainter, "null painter");
  if (!mSynthBorder) {
    mSynthBorder = new (aPainter->mPresContext)
                        nsStyleBorder(aPainter->mZeroBorder);
    if (!mSynthBorder) return NS_ERROR_OUT_OF_MEMORY;

}
e:/moz_src/mozilla/layout/html/table/src/nsTablePainter.cpp(209) : error C2248:
"mPresContext" : cannot access mPresContext declared in class
"TableBackgroundPainter" 
        e:/moz_src/mozilla/layout/html/table/src/nsTablePainter.h(247) : see
declaration of 'mPresContext'
e:/moz_src/mozilla/layout/html/table/src/nsTablePainter.cpp(210) : error C2248:
"mZeroBorder" :  cannot access mZeroBorder declared in class
"TableBackgroundPainter" 
        e:/moz_src/mozilla/layout/html/table/src/nsTablePainter.h(263) : see
declaration of  'mZeroBorder'

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/vccore/html/C2248.asp

    
    

    
  
Since different versions of the C++ standard have different rules on member
access and nested classes, it's best to declare all nested classes or structs
this way:

class Container {

    // ...

    class Nested;
    friend class Nested;
    class Nested {
        // ...
    };

    // ...
};

so that the nested class is a friend of its containing class (which does nothing
in the current C++ standard, but did something in the 1998 version).

    
    

    
  
Even then you may need to make those members protected, not private.

    
    

    
  
Comment on attachment 141851 [details] [diff] [review]
patch

 I removed the static at
nsTableRowGroupFrame*
nsTableFrame::GetRowGroupFrame(nsIFrame* aFrame,
			       nsIAtom*  aFrameTypeIn)
further I added 

    struct TableBackgroundData;
    friend struct TableBackgroundData;
before the 
    MOZ_DECL_CTOR_COUNTER(TableBackgroundData)

With these changes I could compile it with VC++ 6.0.
Then I added a 

nsTableFrame* table = (nsTableFrame*)aTableFrame->GetFirstInFlow(); 
nsFrameList& colGroupList = table->GetColGroups();

as the colgroup list is only valid on the first table frame. I wonder 
if it would be better to incorporate this into the getcolgroups function

Further I replaced all PR_FALSE arguments in the PaintBackgroundWithSC calls
with PR_TRUE
otherwise we would paint the backgrounds during printing even if the user
deselected it 
in the page setup.

with those changes r+

        Attachment #141851 -
        Flags: review?(bernd_mozilla) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        final patch (?)
        — 
          — Splinter Review
      

    


  
Made all the changes Bernd mentioned, and switched some casts over to
NS_STATIC_CAST per dbaron's instructions. r/sr?

        Attachment #141851 -
        Attachment is obsolete: true
Blocks: 88260

    
    

    
  
So what's the status of the issues raised in comment 160?  I assume that last
patch addresses them?  If so, please let me know and I will try to sr it this
Saturday.   We seem to have ended up with three reviewers and two reviews that
need to happen.... and no one being sure what's going on.

    
    

    
  
Yeah, the second-to-last patch fixed them. See comment 170.

Further responses:

> {ns/moz/}TableBackgroundPainter.{cpp,h} 

Kept as nsTablePainter, per discussion on IRC.


> nsStyleStruct.cpp:

dbaron says he filed a bug already.
 
> aClipRect should probably be called aBGClipRect

Done.

>  Limiting BC borders to 63 pixels ... typedef for a pixel size of a BC border

Typedefed BCPixelSize.
 
> Shouldn't GetContinuousLeftBCBorderWidth use the BC_BORDER_LEFT_HALF_COORD macro

Don't see how this comment applies.
 
> The nsTableBackgroundPainter should be called unconditionally.

Fixed.
 
> In the code in nsTableRow(Group)?Frame that creates a
> TableBackgroundPainter, do you want to set the flag tell the rows /
> cells not to paint?

Fixed.
 
> don't need to do the fancy save-and-reassign dance in GetContinuousBCBorderWidth.

Fixed.

> break NS_STYLE_TABLE_EMPTY_CELLS_SHOW_BACKGROUND

Fixed.

>  TableBackgroundPainter::QuirksPaintTable incorrectly applies visibility

Fixed.

> say "must not call ... in quirks mode".

Fixed.

    
    

    
  
I'll still try to get to this tonight or tomorrow morning, if we want to land
this for 1.7b.  I personally feel that that's a reasonable course of action.  If
someone disagrees, please say so...

    
    

    
  
Boris, I disagree. Due to my limited time (and knowledge) resources, I can't
make sure that I have during the the freeze enough resources to iron out more
than regression. This code is large, at least IMHO, so my guess is we will have
more than just one critical regression.The patch is so clearly alpha material,
that I thought it might be possible to get it in in early beta, but I refuse to
go days before the freeze. If you could make the sr so that it goes into 1.8a I
would be already delighted.

    
    

    
  
Yeah, I can definitely make sure I sr in time for 1.8a (well before, in fact). 
I just hate missing the boat two milestones in a row now.... :(  But if you have
misgivings about this patch, then we shouldn't.

    
    

    
  
I agree with slipping to 1.8a. This is the kind of thing where we've been wrong
forever, and another few months of being wrong the same way is no big deal, and
far preferable to the chance of being wrong in a different way for just a few
months.

    
    

    
  
I think we should get this in for 1.7b.  We've got plenty of time before 1.7b
releases, never mind after it, to see if things go wrong.  If they go wrong
badly, we can back it out and reland for 1.8a.  If it's not serious, a few minor
regressions are definitely worth moving towards a process with which we have a
chance of attracting new contributors.

    
    

    
  
Comment on attachment 142639 [details] [diff] [review]
final patch (?)

Based on diffing diffs and rereading review comments above, sr=dbaron.	This is
a big patch, we've had a lot of review, and at some point we just need to stop
reviewing and check in.

        Attachment #142639 -
        Flags: superreview+

    
    

    
  
fix checked in, instead of flowers.

Happy Intl. Womens Day Fantasai!!
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 20 years ago
Resolution: --- → FIXED

    
    

    
  
Finally! :) Excellent work all contributors. 

    
    

    
  
Someone should probably go through the dependencies of this bug and see which
ones are now fixed.  I'm not optimistic, though -- many seem like bogus
dependencies.

    
    

    
  
I added a number of bugs as dependencies because there was not much point of
testing them until the patch landed...

I'll probably try to go through the deps tonight.

    
    

    
  
One other question -- what's now implemented for standards mode is a good bit
closer to quirks mode than the old version of standards mode background
painting.  Are there any reasons we still need the quirks?  If not, should we
file a bug to remove the mode differences when we open for 1.8a?

    
    

    
  
There was some discussion of that in comment 103, comment 104, comment 115.

I would be in favor of removing the quirk in 1.8a and seeing what happens,
myself.  The non-quirk behavior seems vastly more preferable in most situations
I can think of....

    
    

    
  
This commit have added a "may be used uninitialized" warning on brad TBox:

+layout/html/table/src/nsTableFrame.cpp:5679
+ `PRBool gotRowBorder' might be used uninitialized in this function


    
    

    
  
Hmm.. that warning looks correct to me.  We should init it (presumably to true,
given the following code?)

    
    

    
  
Hmmm.  My guess was that it should be initialized to false, but I'm not sure.

    
    

    
  
Although, actually, iter.IsNewRow() might be guaranteed to be true the first
time through the loop, which would mean it's a bogus warning.

    
    

    
  
iter.First calls SetNewGroup wich calls SetNewRow which sets mIsNewRow to true,
so the call to IsNewRow will for the first cell that is iterated over always be
true.
However I think given the difficulties to read the border collapse assembly
code, I will be more explicit that it is PR_FALSE at the start in the patch for
bug 229883, which I delayed to get this patch here up and running first.

    
    

    
  
Oh, so /this/ is why I had unusually much bugmail. :) Thank you very much,
everyone. *doffs hat and bows*

So, as for Quirks, I was looking over Tantek's shoulder while he pulled up one
of my tests at the Sofitel. I suspect MacIE doesn't do the transparent thing,
because it was definitely painting colgroups as colgroup backgrounds and not
inherited cell backgrounds. So I'm guessing we should be all right with getting
rid of the quirk, although saving it for 1.8a sounds fine to me. Filed bug
237078 on the issue.


Blocks: 237078

    
  

        Attachment #141121 -
        Flags: review?(bernd_mozilla)

    
    

    
  
(In reply to comment #152)
> > This is increasing the size of nsTableRowFrame, and I _know_ we have lots
> > of those....
> 
>  As discussed, I'll file a bug on moving that into a frame property.

Did that ever get filed?

    
  
No longer blocks: 233532

    
    

    
  
I think I found a bug in this fix.
See bug 267592
happens in mozilla and firefox

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: