47432 - different result with table in mozilla and NS 4/IE 5

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Ariel Shkedi]

BuildID:    20000080208

The code included renders differently in Mozilla and NS 4/IE 5

Reducing by 1 the two colspans will fix it - but the code as is is legal and
possibly shows an error.

Comment 1

[Ariel Shkedi]

Attachment: Code that displays layout error

Comment 2

[Rann Bar-On]

confirmed on 080308, win95...confirming bug.

Comment 3

[Nisheeth Ranjan]

Re-assigning bugs on Clayton's list opened between 7/29 and 8/4 to Kevin for
further triage.

Comment 4

[Kevin McCluskey (gone)]

Setting the width of the table to 732 prevents it from placing the last image 
on the second column. Setting table width to 731 or lower causes the last image 
to wrap.

It looks like its using the width of the colspan to determine the width of the 
rest of the table.

Comment 5

[Bernd Mielke]

Attachment: testcase without images

Comment 6

[Bernd Mielke]

Look on the previous testcase with Mozilla and Nav in parallel and look how they
fail. It seems that the redistribution of the excess space to the third column
is wrong. IMHO on the third table the width of the third column is equaly to the
desired content width of the column in the first row. (700-180=520).The bug
arises from AssignNonPctColWidths (look on the table dumps). Checked with
2000091905 under Win98.

Comment 7

[Bernd Mielke]

Attachment: another testcase with rows exchanged

Comment 8

[Bernd Mielke]

This bug arrises from a wrong algorithm inside ComputeNonPctColspanWidths.

Please have a look on the third column of the table frame dump:

AssignNonPctColWidths ex
***START TABLE DUMP*** 
mColWidths=1395 1395 495 

 col frame cache ->
0=00BC19C0 1=00BC1A28 2=00BC1A90 
 **START COL DUMP** colIndex=0 isAnonymous=0 constraint=0
  widths=0 0 -1 1395 3855 -1 -1 -1 -1  **END COL DUMP** 
 **START COL DUMP** colIndex=1 isAnonymous=0 constraint=0
  widths=0 0 -1 1395 3855 -1 -1 -1 -1  **END COL DUMP** 
 **START COL DUMP** colIndex=2 isAnonymous=0 constraint=0
  widths=120 120 -1 495 8400 -1 -1 -1 -1  **END COL DUMP**  ***END TABLE DUMP*** 

The algorithm sees in the first row a colspan=3. All the excess width should be 
distributed to the cells below. As the first two cells belong to another colspan 
they have a width of -1 and as a result *all* the excess goes to third column. 
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=15926 shows what 
happens if the rows are reversed. It seems: under these conditions the algorithm 
is correct.

A correct algorithm should take into account that a colspan can consist not only 
 of auto, percentage and fixed width cells but also of cells that belong to 
another colspan. 

Comment 9

[Chris Petersen]

Changing QA contact to Chrisd.

Comment 10

[Hixie (not reading bugmail)]

Netscape's standard compliance QA team reorganised itself once again, so taking 
remaining non-tables style bugs. Sorry about the spam. I tried to get this done 
directly at the database level, but apparently that is "not easy because of the 
shadow db", "plus it screws up the audit trail", so no can do...

Comment 11

[Bernd]

steeling this bug

Comment 12

[Bernd]

Attachment: patch

Comment 13

[Bernd]

The idea behind the patch is to sort the information from the rows in such a
manner that the smaller colspans are treated first. The patch passed the
regression tests and makes us look better than some competitor (M+%&!"t).

Comment 14

[rbs]

bug 38173 is proposing ns[Auto]UInt32Array which might be useful here.

Comment 15

[Bernd]

Attachment: testcase without dead columns (renders fine with NN4.7)

Comment 16

[Bernd]

Attachment: testcase with filled columns

Comment 17

[Bernd]

Attachment: patch with cosmetics

Comment 18

[Bernd]

Attachment: patch with cosmetics (ignore the previous)

Comment 19

[karnaze (gone)]

Bernd, r=karnaze if you add some comments indicating the reordering you are 
doing. Don't go overboard - maybe 2 lines of comments in a couple of places.

Comment 20

[Bernd]

my new comment will be:

//if more than one colspan originate in a column, resort the access to 
//the rows so that the inner colspans are handled first

Comment 21

[Marc Attinasi]

This looks good, but you might want to address the possible memory leak here:

+  PRInt32* rowIndices = new PRInt32[numRows];
+  if(!rowIndices)
+    return;


The previous allocation, numColSpans, will not be deleted in this unlikely error
condition. Like rbs suggested, an auto-array would solve the problem nicely.

sr=attinasi

Comment 22

[Bernd]

fix checked in

Comment 23

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Reopening.  The last part of the patch (the two calls to |delete[]|) was not
checked in, so this leaks.  (Also, shouldn't the |delete| you added in response
to review comments be |delete[]| instead?)

Comment 24

[Bernd]

Attachment: leak fix

Comment 25

[Marc Attinasi]

Yes, please check in the patch that cleans up! [s]r=attinasi

Comment 26

[karnaze (gone)]

r=karnaze

Comment 27

[Asa Dotzler [:asa]]

a=[e-mail] (on behalf of drivers) for checkin to 0.9

Comment 28

[Bernd]

fix checked in. David can you verify?

Comment 29

[Amarendra Hanumanula]

 when I look at the HTML testcases attached NS6.1 seems to work fine. But the 
same HMTL renders differently in NS4.77 and IE... But I think NS6.1 is rendering 
the HTML correctly where as NS4.77 and IE does not.. Can some have a look at 
this and correct me I am wrong..
Leaving bug as resolved..............

Comment 30

[Ariel Shkedi]

Verifying, (a little late I know).

Should it also be closed?