broken handling of multilength in COL tag with span attribute

RESOLVED FIXED in mozilla1.2alpha

Status

()

Core
Layout
P2
normal
RESOLVED FIXED
16 years ago
14 years ago

People

(Reporter: Wolf-Dietrich Moeller, Assigned: karnaze (gone))

Tracking

({mlk, regression, testcase})

Trunk
mozilla1.2alpha
x86
Windows NT
mlk, regression, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch])

Attachments

(5 attachments, 11 obsolete attachments)

379 bytes, text/html
Details
1.24 KB, text/html
Details
404 bytes, text/html
Details
261 bytes, text/html
Details
3.77 KB, patch
karnaze (gone)
: review+
kinmoz
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

16 years ago
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)
2.
3.

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:0.9.4.1) 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">
<HTML>
<HEAD>
<TITLE>testcase: col with multilength width and span</TITLE>
</HEAD>
<BODY>
<TABLE width="100%" summary="Navigation">
 <COL span="3" width="*">
 <TBODY>
  <TR>
   <TD bgcolor="#00FFFF">06</TD>
   <TD bgcolor="#FFFF00">05</TD>
   <TD bgcolor="#FF00FF">04</TD>
  </TR>
 </TBODY>
</TABLE>
</BODY>
</HTML>
Created attachment 80631 [details]
The testcase
confirmed on linux.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows NT → All
Hardware: PC → All

Comment 3

16 years ago
I thought I have seen this before...., I think the lookup of column widths in
http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsHTMLTableColElement.cpp#249
does not ake account of the span attribute. It cant a  bug 915 dupe, as it has
worked before.
Status: NEW → ASSIGNED
OS: All → Windows NT
Hardware: All → PC

Comment 4

16 years ago
Michael, could you help to narrow the regression time? Thanks
Keywords: regression

Comment 5

16 years ago
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

Comment 6

16 years ago
there has been a major change to content in this time
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=SeaMonkeyAll&branch=HEAD&branchtype=match&dir=mozilla%2Fcontent&file=&filetype=match&who=jst%25netscape.com&whotype=match&sortby=Date&hours=2&date=explicit&mindate=10%2F28%2F2001+00%3A00%3A00+&maxdate=10%2F31%2F2001+23%3A00%3A00&cvsroot=%2Fcvsroot

I think 6.2.3 should not be worse than 6.2.2
Keywords: nsbeta1

Comment 7

16 years ago
the code at
http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsHTMLTableColElement.cpp#315
does not get executed. Addding jst to CC as he changed that code.
(Assignee)

Comment 8

16 years ago
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

Comment 9

16 years ago
Created attachment 80985 [details]
Testcase for problem with SPAN attribute on COL elements.

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: nsbeta1 → nsbeta1-

Comment 11

16 years ago
Created attachment 82386 [details] [diff] [review]
hack

Comment 12

16 years ago
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 
http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsCSSFrameConstructor.cpp#3167
inside the TableProcessChild Routine. aChildItems is a FrameItems list. The
AddChild method at
http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsCSSFrameConstructor.cpp#569
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

Comment 13

16 years ago
Created attachment 82389 [details] [diff] [review]
hack v2

Comment 14

16 years ago
Created attachment 82390 [details]
another testcase
Attachment #82386 - Attachment is obsolete: true
(Assignee)

Comment 15

16 years ago
Created attachment 82741 [details] [diff] [review]
patch that corrects some flaws and reveals a style system problem

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
(Assignee)

Comment 16

16 years ago
->style system
Assignee: karnaze → dbaron
Status: ASSIGNED → NEW
Component: HTMLTables → Style System
QA Contact: amar → ian

Comment 17

16 years ago
David, could you please check that the regression is not a consequence of
http://bugzilla.mozilla.org/show_bug.cgi?id=56117#c46. The checkin would fit
mkaply 's time window. Thanks
Width is not an inherited property.  I think the style context construction here
seems wrong.
Created attachment 82927 [details] [diff] [review]
patch correcting style context parents
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
|ReParentStyleContext|.

Comment 21

16 years ago
>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
well).
(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
two.)
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.
Created attachment 83765 [details] [diff] [review]
more complete patch
Attachment #82927 - Attachment is obsolete: true
Created attachment 83814 [details] [diff] [review]
more complete patch, really this time
Attachment #83765 - Attachment is obsolete: true
Target Milestone: Future → mozilla1.1alpha

Comment 29

16 years ago
I am not sure that the inherit of the width can be removed.
   <COLGROUP span="40" width="20">
   </COLGROUP>
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?

Comment 30

16 years ago
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

Comment 31

16 years ago
cc'ing myself
Whiteboard: [patch]

Comment 32

16 years ago
Add patch 83814 , the first testcase will work fine,
but the second testcase 80985 still doesnot work.

Comment 33

16 years ago
*** Bug 159452 has been marked as a duplicate of this bug. ***

Comment 34

16 years ago
Created attachment 93667 [details]
testcase got from bug 159452

Comment 35

16 years ago
Created attachment 93682 [details] [diff] [review]
patch 0.1

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.

Comment 36

16 years ago
*** Bug 160676 has been marked as a duplicate of this bug. ***

Comment 37

16 years ago
Created attachment 93978 [details] [diff] [review]
patch 0.2


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

Updated

16 years ago
Keywords: review

Comment 38

16 years ago
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;

Comment 39

16 years ago
Created attachment 93995 [details] [diff] [review]
patch 0.21

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.

Comment 42

16 years ago
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.

Comment 43

16 years ago
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().
see
http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/nsTableColGroupFrame.cpp#459

   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
conditional?

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?

Comment 45

16 years ago
Created attachment 94621 [details] [diff] [review]
patch 0.22
Attachment #93978 - Attachment is obsolete: true
Attachment #93995 - Attachment is obsolete: true

Comment 46

16 years ago
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).

Updated

15 years ago
Blocks: 166758

Comment 48

15 years ago
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
(Assignee)

Comment 49

15 years ago
Created attachment 98850 [details] [diff] [review]
patch 0.23

passes regression tests
Attachment #82741 - Attachment is obsolete: true
Attachment #94621 - Attachment is obsolete: true

Comment 50

15 years ago
Comment on attachment 98850 [details] [diff] [review]
patch 0.23

r=bernd

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

Bernd
Attachment #98850 - Flags: review+

Comment 51

15 years ago
yeah, the patch 0.23 seems quite good.
one question, for there is a for loop added, does it hurt perf?

Comment 52

15 years ago
assign it to Chris.
Assignee: jerry.tan → karnaze
(Assignee)

Updated

15 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 53

15 years ago
The loop is only entered when there is a col with a span - very seldom.

Comment 54

15 years ago
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.



(Assignee)

Comment 55

15 years ago
Created attachment 99032 [details] [diff] [review]
patch adding in the changes to html.css in attachment 8381 [details] [diff] [review]

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
(Assignee)

Updated

15 years ago
Attachment #99032 - Flags: review+

Comment 56

15 years ago
Comment on attachment 99032 [details] [diff] [review]
patch adding in the changes to html.css in attachment 8381 [details] [diff] [review]

sr=kin@netscape.com
Attachment #99032 - Flags: superreview+

Comment 57

15 years ago
karnaze has checked in the patch, mark it as fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 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.