735579 - TH width not updated when changing the width of a cell in the first row that's not the first cell in that row in a fixed-layout table

Status: RESOLVED FIXED

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

Description

[TinyFox]

Attachment: ff10-table-size-test-case.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0.2) Gecko/20100101 Firefox/10.0.2
Build ID: 20120215223356

Steps to reproduce:

Given a table with 3 columns (TH), inside a div with fixed width and height. Assign width on the first TH, and then later on the second TH.


Actual results:

The width of the 3rd TH is not changed when a width is assigned to the second TH.


Expected results:

The width of the 3rd TH should be updated when a width is assigned to the second TH.

Comment 1

[Alice0775 White]

Regression window
Works:
http://hg.mozilla.org/mozilla-central/rev/60e86b847759
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20110929 Firefox/10.0a1 ID:20110929122038
Fails:
http://hg.mozilla.org/mozilla-central/rev/af3668a89015
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20110929 Firefox/10.0a1 ID:20110929141938
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=60e86b847759&tochange=af3668a89015

Last good: 34f184d2a6f8
First bad: 00f422b2cf36
Triggered by:
00f422b2cf36	Ehsan Akhgari — Bug 10209 - Part 6: Implement the CSS "containing block" concept correctly as a binary relation, as opposed to a unary relation; r=bzbarsky

Comment 2

[(no longer active)]

Boris, any wild theories on what may be going wrong here?

Comment 3

[TinyFox]

According to my observation, if you set the width of TH1 and TH2 consecutively (in the same event), TH3 width will be correct (so timing/sequence may be involved). Or, if you set the width of TH2 first and then TH1, it will be correct too.

Just a wild guess, but perhaps FF incorrectly determines to skip TH3 width synchronization due to some mechanism for performance enhancement?

Comment 4

[Boris Zbarsky [:bzbarsky]]

I have no wild theories for why this _used_ to work.  Nor for why it still works if the fixed-height div containing the table is removed (???).  The fact that removing that div makes things work is actually pretty worrisome

But the reason it's broken now is that FixedTableLayoutStrategy::ComputeColumnWidths has this loop at the end (with some error-checking removed):

    for (PRInt32 col = 0; col < colCount; ++col) {
        nsTableColFrame *colFrame = mTableFrame->GetColFrame(col);
        if (oldColWidths.ElementAt(col) != colFrame->GetFinalWidth()) {
            mTableFrame->DidResizeColumns();
        }
            break;
    }

Note the misplaced break statement.  This code initially landed in that state in bug 458924.

Moving the break inside the |if|, where it should be, fixes this bug.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Ah, I see why it works without the fixed-height div.  Without that, nsTableFrame::Reflow has aReflowState.mFlags.mVResize set to true, and hence calls SetGeometryDirty() which triggers reflows of all cells...

Presumably we ended up calling SetGeometryDirty in some other cases too, before.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Correctly handle changes to column widths in fixed-layout tables.

Comment 7

[Boris Zbarsky [:bzbarsky]]

TinyFox, thank you for the testcase!

Ehsan, I'm tempted to ask for branch approvals, since this change is pretty obviously correct...

Comment 8

[(no longer active)]

(In reply to Boris Zbarsky (:bz) from comment #7)
> Ehsan, I'm tempted to ask for branch approvals, since this change is pretty
> obviously correct...

Yeah indeed.  That would be fine by me.

Comment 9

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/a4cd76402136

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 606365 [details] [diff] [review]
Correctly handle changes to column widths in fixed-layout tables.

[Approval Request Comment]
Regression caused by (bug #): Bug 10209
User impact if declined: Incorrect dynamic updating of column/cell widths in
   fixed-width tables in some cases.
Testing completed (on m-c, etc.): Passes testcases in this bug and automated
   tests.
Risk to taking this patch (and alternatives if risky): Low risk.  The old code is
   obviously wrong; the new code will just avoid optimizing away (incorrectly)
   table relayouts.
String changes made by this patch: None.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/a4cd76402136

Comment 12

[Alex Keybl [:akeybl]]

Comment on attachment 606365 [details] [diff] [review]
Correctly handle changes to column widths in fixed-layout tables.

[Triage Comment]
Agreed that Aurora is the highest we should uplift given this isn't an absolutely critical regression, albeit recent (FF10).

Comment 13

[Boris Zbarsky [:bzbarsky]]

Note that this is not really a regression from bug 458924; it's just that that bug didn't fully fix the problem it was trying to fix.

Comment 14

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/3d220e21f6f7

Comment 17

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 6.1; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20100101 Firefox/13.0

Verified with Firefox 13 beta 2 on Windows 7, Ubuntu 11.10 and Mac OS X 10.6 that when using the STR from the Description the TH width is updated.