Last Comment Bug 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
: TH width not updated when changing the width of a cell in the first row that'...
Status: RESOLVED FIXED
[qa+]
: regression
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: 10 Branch
: x86_64 All
: P1 normal (vote)
: mozilla14
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
: 729114 741370 (view as bug list)
Depends on:
Blocks: 10209 458924
  Show dependency treegraph
 
Reported: 2012-03-13 23:52 PDT by TinyFox
Modified: 2012-05-08 08:24 PDT (History)
11 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
affected
verified


Attachments
ff10-table-size-test-case.html (1013 bytes, text/html)
2012-03-13 23:52 PDT, TinyFox
no flags Details
Correctly handle changes to column widths in fixed-layout tables. (3.67 KB, patch)
2012-03-15 15:14 PDT, Boris Zbarsky [:bz]
ehsan: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description TinyFox 2012-03-13 23:52:06 PDT
Created attachment 605666 [details]
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 2012-03-14 01:06:35 PDT
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 :Ehsan Akhgari 2012-03-14 20:12:21 PDT
Boris, any wild theories on what may be going wrong here?
Comment 3 TinyFox 2012-03-14 21:38:57 PDT
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 [:bz] 2012-03-15 15:09:16 PDT
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 [:bz] 2012-03-15 15:12:16 PDT
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 [:bz] 2012-03-15 15:14:26 PDT
Created attachment 606365 [details] [diff] [review]
Correctly handle changes to column widths in fixed-layout tables.
Comment 7 Boris Zbarsky [:bz] 2012-03-15 15:15:57 PDT
TinyFox, thank you for the testcase!

Ehsan, I'm tempted to ask for branch approvals, since this change is pretty obviously correct...
Comment 8 :Ehsan Akhgari 2012-03-16 12:50:17 PDT
(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 10 Boris Zbarsky [:bz] 2012-03-16 13:01:39 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-17 17:10:29 PDT
https://hg.mozilla.org/mozilla-central/rev/a4cd76402136
Comment 12 Alex Keybl [:akeybl] 2012-03-20 13:26:13 PDT
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 [:bz] 2012-03-20 14:11:34 PDT
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 15 Daniel.S 2012-04-02 10:45:06 PDT
*** Bug 741370 has been marked as a duplicate of this bug. ***
Comment 16 Boris Zbarsky [:bz] 2012-04-23 18:10:36 PDT
*** Bug 729114 has been marked as a duplicate of this bug. ***
Comment 17 Simona B [:simonab ] -PTO- back Sept 5th 2012-05-08 08:24:19 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.