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

VERIFIED FIXED

Status

()

P2
normal
VERIFIED FIXED
10 years ago
7 years ago

People

(Reporter: mikew, Assigned: bernd_mozilla)

Tracking

({regression, testcase, verified1.9.1})

Trunk
x86
All
regression, testcase, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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.
(Reporter)

Comment 1

10 years ago
Created attachment 342107 [details]
A testcase

Updated

10 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
This was caused by bug 300030.
Blocks: 300030
Component: General → Layout: Tables
Keywords: regression
Product: Firefox → Core
QA Contact: general → layout.tables
Version: unspecified → Trunk
Flags: wanted1.9.1?
(Reporter)

Comment 3

10 years ago
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?
(Assignee)

Comment 4

10 years ago
the later,  the fix fro bug 300030 included a rewrite of the width distribution for fixed and auto layout tables.
(Assignee)

Comment 5

10 years ago
Created attachment 342287 [details]
reduced testcase
(Assignee)

Comment 6

10 years ago
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

10 years ago
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
(Assignee)

Comment 8

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

Comment 9

10 years ago
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.
(Reporter)

Comment 11

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

Updated

10 years ago
Keywords: testcase
(Assignee)

Comment 12

10 years ago
Created attachment 343830 [details] [diff] [review]
patch
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
(Assignee)

Comment 14

10 years ago
Created attachment 348407 [details] [diff] [review]
revised patch for checkin
Whiteboard: [needs landing]
(Assignee)

Comment 15

10 years ago
Created attachment 350267 [details] [diff] [review]
mq patch with user and checkin comment
(Assignee)

Updated

10 years ago
Attachment #350267 - Attachment is obsolete: true
(Assignee)

Comment 16

10 years ago
Created attachment 350269 [details] [diff] [review]
 mq patch with user and checkin comment
http://hg.mozilla.org/mozilla-central/rev/a5e50d053bc0
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
(Assignee)

Comment 18

10 years ago
I will checkin a reftests when the tree is less under pressure
Flags: in-testsuite?
Keywords: fixed1.9.1
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
Keywords: fixed1.9.1 → verified1.9.1
You need to log in before you can comment on or make changes to this bug.