Closed Bug 458924 Opened 12 years ago Closed 12 years ago

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

Categories

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

x86
All
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: mikew, Assigned: bernd_mozilla)

References

Details

(Keywords: regression, testcase, verified1.9.1)

Attachments

(5 files, 1 obsolete file)

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.
Attached file A testcase
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
This was caused by bug 300030.
Component: General → Layout: Tables
Keywords: regression
Product: Firefox → Core
QA Contact: general → layout.tables
Version: unspecified → Trunk
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?
the later,  the fix fro bug 300030 included a rewrite of the width distribution for fixed and auto layout tables.
Attached file reduced testcase
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
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
What about trunk? That's the thing that we care about for the next release.
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?
(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.
Both testcases fail in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081009 Minefield/3.1b2pre
Keywords: testcase
Attached patch patchSplinter Review
Assignee: nobody → bernd_mozilla
Attachment #343830 - Flags: superreview?(dbaron)
Attachment #343830 - Flags: review?(dbaron)
Attachment #343830 - Flags: superreview?(dbaron)
Attachment #343830 - Flags: superreview+
Attachment #343830 - Flags: review?(dbaron)
Attachment #343830 - Flags: review+
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.
Attachment #343830 - Attachment is patch: true
Flags: wanted1.9.1? → blocking1.9.1+
Priority: -- → P2
Whiteboard: [needs landing]
Attachment #350267 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/a5e50d053bc0
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
I will checkin a reftests when the tree is less under pressure
Flags: in-testsuite?
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.