45486 - table cell sizes wrong when colspan and width both given

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect, P3)

Description

[Tommi Rintala]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; m17) Gecko/20000711
BuildID:
2000071108

Mozilla does a bad output when rendering tables with following properties:

- table width is given in
procentage or pixels
- table rows has more than 5 cells
- table cell width has been given in
procents or pixels
- table cell has colspan

Mozilla calculates the width of cells wrong, which can
be seen from the example url above.



Reproducible: Always
Steps to Reproduce:
1. Open a document having table described
2. 
3.


Actual Results:  Table cell widths get totally mixed.

Expected Results:  Parse the Document as
good (or better) as Opera

In the page (http://www.starsoft.fi/testi/mozilla.html) is a
shorter
demonstration of situation where the bug is visible.

If all cells have text, so that in some row of table
there is text
in each cell, results are better.

Comment 1

[Asa Dotzler [:asa]]

Confirmed with 071308 mozilla win32 build on NT4 Over to HTML Tables.  Nice bug
report!

Comment 2

[Marcelo Vanzin]

I think this might be the bug that's been annoying me for quite some time now. :-)

Some examples:

http://stats.distributed.net/rc5-64/psummary.php3?id=276065
My distributed.net stats page. Totally garbled.

http://www.rededc.com.br/noticia/index.html
A page from the site I work on. Show ok in IE and NS4, and almost there in NS6
PR1... not a good one to look at the HTML tho, it's a total mess.

I also noticed this same kind of behaviour when reading the Java 2 Documentation
(JavaDoc output).

Comment 3

[Marcelo Vanzin]

About the second link I posted: I modified the HTML in our internal server and
got better results from Mozilla when removing the DOCTYPE (it is set to
Transitional). I've seen a bug about this somewhere...

But still the rendering has some glitches not present in IE nor in NS4. I'll bee
looknig more closely at it...

Comment 4

[Keyser Sose]

WORKSFORME
Platform: PC
OS: Windows 98
Mozilla Build: 2001012205

Marking as such.

Comment 5

[Jarto Tarpio]

WORKSFORME? Excuse me but this bug is still very real with Mozilla/5.0 (Windows;
U; WinNT4.0; en-US; m18) Gecko/20010122

Look at the table on http://www.starsoft.fi/testi/mozilla.html and tell me if
the widths are reasonable. On this Mozilla I can see on the top row how the left
and wider cell is 25% and the narrower is 25%. Please tell me if it looks
different on Win98? And if not, reopen the bug.

Comment 6

[Stephan Niemz]

Reopening per above comment and changing summary.
Ksosez, maybe you've been a bit too quick. :-)

I can see this, too, with build 0130 on NT4.

Comment 7

[Stephan Niemz]

Attachment: simple test case

Comment 8

[Stephan Niemz]

Attachment: complex table (HTML)

Comment 9

[Stephan Niemz]

Attachment: complex table (Mozilla screenshot)

Comment 10

[Stephan Niemz]

Attachment: complex table (Opera screenshot)

Comment 11

[Bernd]

The correct implementation to render the short testcase right would require
solving a system of linear equations in BasicTableLayoutStrategy.cpp, this is
currently not implemented. Implementing this could cause significant performance
penalties, so if somebody comes up with a reasonable and fast algorithm, one
could fix this bug.

Comment 12

[Jarto Tarpio]

Attachment: Two tables with colspans and col%widths

Comment 13

[Jarto Tarpio]

Attachment: Screenshot of Mozilla 20010130 showing the two tables

Comment 14

[Jarto Tarpio]

Attachment: Screenshot of Opera showing the two tables

Comment 15

[Jarto Tarpio]

Attachment: Screenshot of Internet Explorer showing the two tables

Comment 16

[Jarto Tarpio]

Attachment: Screenshot of Netscape 4.5 showing the two tables

Comment 17

[Jarto Tarpio]

I added another testcase + four screenshots of different browsers rendering
them. No browser shows the tables right but IE, Opera and NS4.5 give acceptable
results. However, Mozilla's version of the first table looks absolutely horrendous.

The second table is interesting. If you add one row where the first column
doesn't use colspan, Mozilla shows the table as well as the other browsers.
Maybe a new algorithm is not needed for adequate results?

Comment 18

[karnaze (gone)]

Moving to m0.9.1

Comment 19

[Amarendra Hanumanula]

QA contact update

Comment 20

[karnaze (gone)]

Moving to m1.0.

Comment 21

[Bernd]

I have an idea .... (steeling the bug)

Comment 22

[Bernd]

Attachment: small patch :-)

Comment 23

[Bernd]

Attachment: patch new revision

Comment 24

[Bernd]

Attachment: revised complex testcase

Comment 25

[Bernd]

Attachment: introduced regression

Comment 26

[Bernd]

This patch:
- increases the  use of effective column number  ( this could be slighty more
efficient),s
- rewrites more efficient ( and less ugly)  the row sorting,
- includes the row sorting also for nested percent colspans, by this the inner
colspans will be computed first,
- ensures that the results of previously computed columns are honored for
percent colspans.

As a result of the patch several regression tests broke, mainly due to the
changed handling of effective column numbers. The affected tests are:

C:\moz_sour\mozilla\mozilla\layout\html\tests\table>my_grep
regression test c:\moz_sour\mozilla\mozilla\layout\html\tests\table\core\verify\
col_span2.rgd failed
regression test c:\moz_sour\mozilla\mozilla\layout\html\tests\table\core\verify\
misc.rgd failed
regression test c:\moz_sour\mozilla\mozilla\layout\html\tests\table\core\verify\
row_span.rgd failed
results1.txt:regression test c:\moz_sour\mozilla\mozilla\layout\html\tests\table
\bugs\verify\bug1220.rgd failed
results1.txt:regression test c:\moz_sour\mozilla\mozilla\layout\html\tests\table
\bugs\verify\bug1318.rgd failed
results2.txt:regression test c:\moz_sour\mozilla\mozilla\layout\html\tests\table
\bugs\verify\bug17138.rgd failed
results3.txt:regression test c:\moz_sour\mozilla\mozilla\layout\html\tests\table
\bugs\verify\bug20804.rgd failed
results4.txt:regression test c:\moz_sour\mozilla\mozilla\layout\html\tests\table
\bugs\verify\bug27993-2.rgd failed
results4.txt:regression test c:\moz_sour\mozilla\mozilla\layout\html\tests\table
\bugs\verify\bug29058-3.rgd failed
results4.txt:regression test c:\moz_sour\mozilla\mozilla\layout\html\tests\table
\bugs\verify\bug2947.rgd failed
results5.txt:regression test c:\moz_sour\mozilla\mozilla\layout\html\tests\table
\bugs\verify\bug41890.rgd failed
results5.txt:regression test c:\moz_sour\mozilla\mozilla\layout\html\tests\table
\bugs\verify\bug46623-2.rgd failed
results5.txt:regression test c:\moz_sour\mozilla\mozilla\layout\html\tests\table
\bugs\verify\bug46944.rgd failed
results5.txt:regression test c:\moz_sour\mozilla\mozilla\layout\html\tests\table
\bugs\verify\bug47163.rgd failed
results6.txt:regression test c:\moz_sour\mozilla\mozilla\layout\html\tests\table
\bugs\verify\bug67915-1.rgd failed
results6.txt:regression test c:\moz_sour\mozilla\mozilla\layout\html\tests\table
\bugs\verify\bug7113.rgd failed
results6.txt:regression test c:\moz_sour\mozilla\mozilla\layout\html\tests\table
\bugs\verify\bug7121-2.rgd failed
results6.txt:regression test c:\moz_sour\mozilla\mozilla\layout\html\tests\table
\bugs\verify\bug8858.rgd failed
results6.txt:regression test c:\moz_sour\mozilla\mozilla\layout\html\tests\table
\bugs\verify\bug8950.rgd failed

The most visible deviation from the previous rendering is visible in the latest
testcase, which is the 20th table from colspan2.html. IMHO, we can live with
this as the previous result was more the happy canceling of two errors then a 
predictable result.
With this patch we  render the provided testcases better than previously, come
close  to the IE6 rendering and sometimes even outperform them. 

Comment 27

[Bernd]

Attachment: new rendering of the complex testcase

Comment 28

[Bernd]

Attachment: new rendering of the regression test failure in colspan2.html

Comment 29

[Marc Attinasi]

Please add null-checks for the allocations - new PRInt32[numRows] (we cannot yet
guarantee memory allocations will succeed!).

Also, near the end of the patch I'm seeing what might be an extraneous
curly-brace, please check it out:

         }
       }
+      }


With those things, and karnaze's review, sr=attinasi

Comment 30

[karnaze (gone)]

Bernd, r=karnaze, but please add some comments.

Comment 31

[Bernd]

Attachment: revised patch "Schoener unsere Doerfer"

Comment 32

[karnaze (gone)]

r=karnaze

Comment 33

[Bernd]

fix checked in