458924 - Changes to COLs in a COLGROUP are not reflected properly in a table with style table-layout:fixed

Status: VERIFIED FIXED

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

Description

[Mike Walsh]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.3) Gecko/2008092417 Firefox/3.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.3) Gecko/2008092417 Firefox/3.0.3

When an existing COL element in a COLGROUP is replace with a new COL of a different width, the affected TDs' clientWidths are not updated properly.  This only occurs when the table's table-layout style property is set to fixed.

Reproducible: Always

Steps to Reproduce:
1.Create a table whose columns are defined in a COLGROUP.  Make sure each COL has its width defined.  The table should have the style table-layout:fixed.
2.Create a new COL element programatically and set its width property to the newly desired width.
3.Call replaceChild on the colGroup to replace the existing COL with the new one.
Actual Results:  
The border of the TDs in the updated column seem to update properly but the clientWidth does not.  This leaves the background of the cell, as well as the offset of the cell's nextSibling unchanged.

Expected Results:  
The TDs in the updated column should be displayed with the newly set width.

When the table is focusable (i.e., has a tabindex), and doesn't have -moz-outline:none set in its style, then clicking the table after the COL has been changed forces the table to redraw properly.

Comment 1

[Mike Walsh]

Attachment: A testcase

Comment 2

[Ria Klaassen (not reading all bugmail)]

This was caused by bug 300030.

Comment 3

[Mike Walsh]

This bug is not present in Firefox 2.0.0.17 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.17) Gecko/20080829 Firefox/2.0.0.17).  Not sure if that helps.

I'm still pretty new to bug reporting.  Does comment #2 mean this is actually being worked on as part of bug 300030 even though the status of that bug is resolved/fixed?  Or is this considered a completely separate issue caused by the fix for the other bug?

Comment 4

[Bernd]

the later,  the fix fro bug 300030 included a rewrite of the width distribution for fixed and auto layout tables.

Comment 5

[Bernd]

Attachment: reduced testcase

Comment 6

[Bernd]

we dont call DidResizeColumns() as we do for basic table layout strategy. This seems odd.

 http://mxr.mozilla.org/mozilla-central/source/layout/tables/BasicTableLayoutStrategy.cpp#1020

Comment 7

[Mike Moening]

Bernd,

Your reduced test case does not appear to re-produce the problem on FF 3.0.3 in both XP and Win 2000.

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.3) Gecko/2008092417 Firefox/3.0.3

Comment 8

[Bernd]

What about trunk? That's the thing that we care about for the next release.

Comment 9

[Mike Moening]

Bernd,
I haven't built FF on my machine since 3.0. Before the switch to hg.

Can you test this on a truck build?
Or point me to a nightly I can install?

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to comment #9)
> Or point me to a nightly I can install?

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/

Pick any of the win32 builds there.

Comment 11

[Mike Walsh]

Both testcases fail in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081009 Minefield/3.1b2pre

Comment 12

[Bernd]

Attachment: patch

Comment 13

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

Comment on attachment 343830 [details] [diff] [review]
patch

>+    // store the old column widths
>+    nsTArray<PRInt32> oldColWidths;

This should be nscoord, not PRInt32.

Could you add a comment here that we wouldn't need to store this in an array if we used a different one of the widths as the temporary?  (In other words, if we only set the final width of the column once, we could just call GetFinalWidth right before the SetFinalWidth call and call DidResizeColumns based on that.  DidResizeColumns is designed so it can be called once for each column without much penalty.)

>         nsTableColFrame *colFrame = mTableFrame->GetColFrame(col);
>         if (!colFrame) {
>             NS_ERROR("column frames out of sync with cell map");
>             continue;
>         }
>+        oldColWidths.AppendElement(colFrame->GetFinalWidth());

Could you append a 0 to oldColWidths in the !colFrame case so the indices don't get out of alignment for the loop below?  (It doesn't matter what you append, since you'll never read that entry back out.)

>+    for (PRInt32 col = 0; col < colCount; ++col) {
>+      nsTableColFrame *colFrame = mTableFrame->GetColFrame(col);
>+      if (!colFrame) {
>+        NS_ERROR("column frames out of sync with cell map");
>+        continue;
>+      }
>+      if (oldColWidths.ElementAt(col) != colFrame->GetFinalWidth()) {
>+        mTableFrame->DidResizeColumns();
>+        break;
>+      }
>+    }

Please use 4-space indent to match the rest of the file.

r+sr=dbaron with that.

Comment 14

[Bernd]

Attachment: revised patch for checkin

Comment 15

[Bernd]

Attachment: mq patch with user and checkin comment

Comment 16

[Bernd]

Attachment: mq patch with user and checkin comment

Comment 17

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/a5e50d053bc0

Comment 18

[Bernd]

I will checkin a reftests when the tree is less under pressure

Comment 19

[Aakash Desai [:aakashd]]

verified FIXED using the attached testcases on builds:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090721
Minefield/3.6a1pre ID:20090721044139

and

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.1pre) Gecko/20090720
Shiretoko/3.5.1pre ID:20090720042942