Closed Bug 139524 Opened 19 years ago Closed 19 years ago

broken handling of multilength in COL tag with span attribute


(Core :: Layout, defect, P2)

Windows NT





(Reporter: wolf-dietrich.moeller, Assigned: karnaze)



(Keywords: memory-leak, regression, testcase, Whiteboard: [patch])


(5 files, 11 obsolete files)

379 bytes, text/html
1.24 KB, text/html
404 bytes, text/html
261 bytes, text/html
3.77 KB, patch
: review+
: superreview+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; rv:1.0rc1) Gecko/20020417
BuildID:    2002041711

The handling of multilength in COL tags with span attribute is broken. A table
with 3 columns and width=100% is rendered okay with
<COL width="*"><COL width="*"><COL width="*">
but wrong with
<COL span="3" width="*">.

Reproducible: Always
Steps to Reproduce
1.Display the test page (code given below)

Actual Results:  The leftmost column gets too small a width. The following cols
get their width evenly distributed as expected.

Expected Results:  All columns with exactly the same width.

Further comments: For more complex tables the errors show up in a more
"random" manner, e.g. 2nd and 3rd col of a longer span disturbed.

Other browsers and history:
correct display in:
- Mozilla/5.0 (Windows; U; WinNT4.0; ; rv: Gecko/20020314 Netscape6/6.2.2
- Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:0.9.7+) Gecko/20020107 (as far as
I remember, no longer on my machine)
- MSIE 5.5
I first noticed the error in
AGENT=Mozilla/5.0 (Windows; U; WinNT4.0; ; rv:0.9.9) Gecko/20020311
 and it is still there in rv:1.0rc1 (see above).

Example page code:
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<TITLE>testcase: col with multilength width and span</TITLE>
<TABLE width="100%" summary="Navigation">
 <COL span="3" width="*">
   <TD bgcolor="#00FFFF">06</TD>
   <TD bgcolor="#FFFF00">05</TD>
   <TD bgcolor="#FF00FF">04</TD>
Attached file The testcase
confirmed on linux.
Ever confirmed: true
OS: Windows NT → All
Hardware: PC → All
I thought I have seen this before...., I think the lookup of column widths in
does not ake account of the span attribute. It cant a  bug 915 dupe, as it has
worked before.
OS: All → Windows NT
Hardware: All → PC
Michael, could you help to narrow the regression time? Thanks
Keywords: regression
The best I can do is between:

October 28, 2001 at 4PM PST and October 31, 2001 at 8AM PST

Unfortunately it appears the OS/2 build was broke on the 29 and 30, so we got no 
nightly builds.
Keywords: testcase
the code at
does not get executed. Addding jst to CC as he changed that code.
nsCSSFrameConstructor::ConstructTableColFrame looks like it is doing the right 
thing. The extra 2 cols are given a pseudo style context with a parent style 
context equal to the original col's style context. They should be inheriting the 
width, but maybe they aren't. I wonder if this could be a style system problem.
Priority: -- → P2
Target Milestone: --- → Future
Testcase (made before I saw this bug report) attached for bug in handling SPAN
attribute on COL elements. If this is not the same bug, let me know and I'll
submit a separate report.
nsbeta1-. Please clear the (-) if a large number of sites are affected by this
regression or a top100 site.
Keywords: nsbeta1nsbeta1-
Attached patch hack (obsolete) — Splinter Review
The attached hack fixes the problems in both testcases. But I am not sure that
the patch is complete. The main problem is the broken handling of the new cols at 

-      lastCol->SetNextSibling(newCol);
-      lastCol = newCol;

This is wrong. It leaks. aNewFrame will be added to the aChildItems List at
inside the TableProcessChild Routine. aChildItems is a FrameItems list. The
AddChild method at
updates the member variable of the FrameItem list lastChild. The previous
manipulation of the aNewFrame will be overwritten when the aNewFrame is appended
to the list. When this is overwritten we lose the access to the items added to
aNewFrame via the SetNextSibling method.
I don't understand why we need to Reparent the stylecontext when the col is a
child of a colgroup. 
I don't oversee what kind of childrens a TableColFrame can have, so it is
possible that the end of the ConstructTableColFrame routine should be integrated
into the loop above. 
Keywords: mlk
Attached patch hack v2 (obsolete) — Splinter Review
Attached file another testcase
Attachment #82386 - Attachment is obsolete: true
The patch contains a function called DumpForBug139524 which dumps info
indicating that the cols have the right parent style contexts but are not
inheriting the width as they should be. I was using attachment #1 [details] [diff] [review] as the test
case. I'm marking the previous patch as obsolete, because it is not correct to
reuse the style contexts for the spanned cols.
Attachment #82389 - Attachment is obsolete: true
->style system
Assignee: karnaze → dbaron
Component: HTMLTables → Style System
QA Contact: amar → ian
David, could you please check that the regression is not a consequence of The checkin would fit
mkaply 's time window. Thanks
Width is not an inherited property.  I think the style context construction here
seems wrong.
StyleSetImpl::GetStyleContext already has code that causes siblings to get the
same style contexts when they match the same rules.  Since these frames are for
the same content node and they have the same parent frame, they should
automatically have the same style contexts so we can just make it so.  Note that
we have to get the style context from the frame because we've called
>Width is not an inherited property.  I think the style context construction here
>seems wrong.

Wait, width is being _explicitly_ set to inherit - so it should inherit, right? 

Also, I am totally confused on how you assume that the style context for the
pseudo does not have to be resolved. What is the point of having the pseudo
anyway then? Can you please explain your thinking for me?
Oh, I was thinking :-moz-table-column was like all the other table pseudos and
was used when we created anonymous frames.  Do we use it for that too?  Is it
possible to create anonymous col frames?  I'll have to check the spec.

And why are we applying those crazy rules to |col| too?
Anyway, why do we need that :table-column pseudo when we can just use the same
style context over again?  If we get rid of it there won't be a need to update
html.css every time we change what CSS properties can apply to table columns. 
Furthermore, we're doing something that's consistent with what
ReResolveStyleContext will do -- if we really want to do things the current way
we should make it so ReResolveStyleContext can duplicate the structure.

Furthermore, the old code was wrong in two ways, although I don't think they
affect this testcase -- it ignored the first ReParentStyleContext when choosing
the parent for the second, and it did a useless ReParentStyleContext for the
second.  I also don't think the first ReParent is correct -- it looks like it
always reparents to the same frame when in fact it should sometimes reparent to
a higher one (which would then have to be reflected in ReResolveStyleContext as
(Never mind the fact that ReParentStyleContext itself is rather broken, since
none of the recent fixes to ReResolveStyleContext have been applied to
ReParentStyleContext, and we probably need to share a lot more code between the
Also note that 'width: inherit' is handled as an explicit eStyleUnit_Inherit
value in the style data, which must then be implemented by the frames, since the
parent's computed width can't be known until reflow.  There could be something
wrong with nsStyleCoord nsTableColFrame::GetStyleWidth() (which implies that
this correct behavior is incorrect) or with some other table code.
er, I should say, it's sometimes handled that way, and *should* be handled that
way for proportional widths, in general, although it may not make a difference
for tables.
Attached patch more complete patch (obsolete) — Splinter Review
Attachment #82927 - Attachment is obsolete: true
Attachment #83765 - Attachment is obsolete: true
Target Milestone: Future → mozilla1.1alpha
I am not sure that the inherit of the width can be removed.
   <COLGROUP span="40" width="20">
But I would need to dig further, which will not happen before the weekend. David
could you please verify that my analysis of the broken sibling handling is wrong?
it looks that my concerns about the colgroup are invalid. But attachment 83814 [details] [diff] [review]
does not fix the testcase at attachment 80985 [details] because the sibling logic is flawed.
Target Milestone: mozilla1.1alpha → mozilla1.2alpha
cc'ing myself
Whiteboard: [patch]
Add patch 83814 , the first testcase will work fine,
but the second testcase 80985 still doesnot work.

*** Bug 159452 has been marked as a duplicate of this bug. ***
Attached patch patch 0.1 (obsolete) — Splinter Review
With the latest testcase, trunk will generate four TableColFrame object
named them as A, A1, B, B1.
A,A1 are generated by the same <col>, because that <col> set span as 2.
In the process of construction, A->setNextSibling(A1), add A to mChildlist.
then B->SetNextSibling(B1), add B to childlist( call A->SetNextSibling(B).)

During reflow, colgroup get first child as A, then call A->GetNextSibling(), it
will retunr B. 
That is the problem.
it should return A1, according to the spec.
*** Bug 160676 has been marked as a duplicate of this bug. ***
Attached patch patch 0.2 (obsolete) — Splinter Review
The problem of patch 0.1: when met <colgroup><col..><col..>, it will arrange
relationship of ColFrame uncorrectly.

So make another patch, works fine with all testcases of this bug.
Attachment #93682 - Attachment is obsolete: true
Keywords: review
Jerry, could you get rid of the SetNextSibling routine?
I think the only correct way, is to use the addchild routine.

-      lastCol->SetNextSibling(newCol);
-      lastCol = newCol;
+      aChildItems.AddChild(aNewFrame);
+      aNewFrame = newCol;
Attached patch patch 0.21 (obsolete) — Splinter Review
mChildList.AddChild(newCol) will call SetNextSibling(newCol), so just remove
SetNextSibling before AddChild
Re comment 38:  why do you say that?  InitAndRestoreFrame should set the parent,
and then we set the correct sibling with SetNextSibling, no?
Ignore my previous comment.
see my comment #12 at the least the beginning, I believe the mixture of AddChild
and the direct use of the member function (SetNextSibling) is wrong. But I think
even the patch v2.1 is not correct. We should use the AddChild method there, you
can have more than two cols in a colgroup.
Colgroup can has more than one col, but
according to the reflow logic, 
Only one col added into childlist of colgroup is enough.
because it always get the first child, reflow it, then call child->GetNextSibling().

   if (childFrame && !childIsCaption && !isPseudoParent) { 
-    aChildItems.AddChild(childFrame);
+    if (NS_STYLE_DISPLAY_TABLE_COLUMN !=styleDisplay->mDisplay ||
!aChildItems.lastChild) {
+      aChildItems.AddChild(childFrame);
+    }

Is there a reason to have two nested |if| statements instead of just a single

Other than that, this seems pretty reasonable to me (with my vast one-hour
experience in the table frame code.  ;) ).

Is there a reason you're leaving all those "inherit"s in html.css?
Attached patch patch 0.22 (obsolete) — Splinter Review
Attachment #93978 - Attachment is obsolete: true
Attachment #93995 - Attachment is obsolete: true
I found that change to html.css does not affect this bug.
So I just leave it unchanged.
Well.. if it does not affect this bug, what _does_ it affect?  Useless rules
should be removed (since they cost time and memory).
Blocks: 166758
change component to layout
and assign it to me.

I will do some research about the css rules.
Assignee: dbaron → jerry.tan
Component: Style System → Layout
Attached patch patch 0.23 (obsolete) — Splinter Review
passes regression tests
Attachment #82741 - Attachment is obsolete: true
Attachment #94621 - Attachment is obsolete: true
Comment on attachment 98850 [details] [diff] [review]
patch 0.23


Thanks Chris for fixing the sibling issue, thats exactly what I had in mind.

Attachment #98850 - Flags: review+
yeah, the patch 0.23 seems quite good.
one question, for there is a for loop added, does it hurt perf?
assign it to Chris.
Assignee: jerry.tan → karnaze
The loop is only entered when there is a col with a span - very seldom.
yes,it wont hurt perf much, 
I still think combining SetNextSibing and AddChild for col whose span > 1
  is efficient, but code will look ugly.

The changes to html.css have no affect on the bug, but are good nevertheless.
Attachment #83814 - Attachment is obsolete: true
Attachment #98850 - Attachment is obsolete: true
Attachment #99032 - Flags: review+
Comment on attachment 99032 [details] [diff] [review]
patch adding in the changes to html.css in attachment 8381 [details] [diff] [review]
Attachment #99032 - Flags: superreview+
karnaze has checked in the patch, mark it as fixed.
Closed: 19 years ago
Resolution: --- → FIXED
Blocks: 233463
No longer blocks: 233463
You need to log in before you can comment on or make changes to this bug.