Closed
Bug 139524
Opened 22 years ago
Closed 22 years ago
broken handling of multilength in COL tag with span attribute
Categories
(Core :: Layout, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla1.2alpha
People
(Reporter: wolf-dietrich.moeller, Assigned: karnaze)
References
Details
(Keywords: memory-leak, regression, testcase, Whiteboard: [patch])
Attachments
(5 files, 11 obsolete files)
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
:
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>
Comment 1•22 years ago
|
||
Comment 2•22 years ago
|
||
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
Comment 5•22 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.
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
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•22 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
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.
Comment 10•22 years ago
|
||
nsbeta1-. Please clear the (-) if a large number of sites are affected by this regression or a top100 site.
Comment 11•22 years ago
|
||
Comment 12•22 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•22 years ago
|
||
Comment 14•22 years ago
|
||
Attachment #82386 -
Attachment is obsolete: true
Assignee | ||
Comment 15•22 years ago
|
||
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•22 years ago
|
||
->style system
Assignee: karnaze → dbaron
Status: ASSIGNED → NEW
Component: HTMLTables → Style System
QA Contact: amar → ian
Comment 17•22 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.
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•22 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.
Attachment #82927 -
Attachment is obsolete: true
Attachment #83765 -
Attachment is obsolete: true
Target Milestone: Future → mozilla1.1alpha
Comment 29•22 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•22 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•22 years ago
|
||
cc'ing myself
Whiteboard: [patch]
Comment 32•22 years ago
|
||
Add patch 83814 , the first testcase will work fine, but the second testcase 80985 still doesnot work.
Comment 33•22 years ago
|
||
*** Bug 159452 has been marked as a duplicate of this bug. ***
Comment 34•22 years ago
|
||
Comment 35•22 years ago
|
||
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•22 years ago
|
||
*** Bug 160676 has been marked as a duplicate of this bug. ***
Comment 37•22 years ago
|
||
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
Comment 38•22 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•22 years ago
|
||
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•22 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•22 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
Comment 44•22 years ago
|
||
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•22 years ago
|
||
Attachment #93978 -
Attachment is obsolete: true
Attachment #93995 -
Attachment is obsolete: true
Comment 46•22 years ago
|
||
I found that change to html.css does not affect this bug. So I just leave it unchanged.
Comment 47•22 years ago
|
||
Well.. if it does not affect this bug, what _does_ it affect? Useless rules should be removed (since they cost time and memory).
Comment 48•22 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•22 years ago
|
||
passes regression tests
Attachment #82741 -
Attachment is obsolete: true
Attachment #94621 -
Attachment is obsolete: true
Comment 50•22 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•22 years ago
|
||
yeah, the patch 0.23 seems quite good. one question, for there is a for loop added, does it hurt perf?
Assignee | ||
Updated•22 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 53•22 years ago
|
||
The loop is only entered when there is a col with a span - very seldom.
Comment 54•22 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•22 years ago
|
||
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•22 years ago
|
Attachment #99032 -
Flags: review+
Comment 56•22 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•22 years ago
|
||
karnaze has checked in the patch, mark it as fixed.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•