Closed Bug 139524 Opened 23 years ago Closed 23 years ago

broken handling of multilength in COL tag with span attribute

Categories

(Core :: Layout, defect, P2)

Product:
Core
Component:
Layout
Platform:
x86
Windows NT
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.2alpha

People

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

References

Details

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

Attachments

(5 files, 11 obsolete files)

The testcase
379 bytes, text/html
Details
Testcase for problem with SPAN attribute on COL elements.
1.24 KB, text/html
Details
another testcase
404 bytes, text/html
Details
testcase got from bug 159452
261 bytes, text/html
Details
patch adding in the changes to html.css in attachment 8381
3.77 KB, patch
karnaze
: review+
kinmoz
: 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) 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>
Attached file The testcase —
confirmed on linux.
Status: UNCONFIRMED → NEW
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 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
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 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.
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: nsbeta1 → nsbeta1-
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 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
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
Status: ASSIGNED → NEW
Component: HTMLTables → Style System
QA Contact: amar → ian
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.
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|.
>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.
Target Milestone: Future → mozilla1.1alpha
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?
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
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?
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(). 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?
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 r=bernd Thanks Chris for fixing the sibling issue, thats exactly what I had in mind. Bernd
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
Status: NEW → ASSIGNED
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] sr=kin@netscape.com
Attachment #99032 - Flags: superreview+
karnaze has checked in the patch, mark it as fixed.
Status: ASSIGNED → RESOLVED
Closed: 23 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.

Attachment

General

Creator:
Created:
Updated:
Size: