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)

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: 22 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: