The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 13

Status

()

Core
Layout: Tables
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: TinyFox, Assigned: bz)

Tracking

({regression})

10 Branch
mozilla14
x86_64
All
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox11 affected, firefox12 affected, firefox13 verified)

Details

(Whiteboard: [qa+])

Attachments

(2 attachments)

(Reporter)

Description

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

5 years ago
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
Blocks: 10209
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout: Tables
Ever confirmed: true
Keywords: regression
OS: Windows 7 → All
Product: Firefox → Core
QA Contact: untriaged → layout.tables

Updated

5 years ago
Attachment #605666 - Attachment mime type: text/plain → text/html
Boris, any wild theories on what may be going wrong here?
(Reporter)

Comment 3

5 years ago
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?
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.
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.
Created attachment 606365 [details] [diff] [review]
Correctly handle changes to column widths in fixed-layout tables.
Attachment #606365 - Flags: review?(ehsan)
TinyFox, thank you for the testcase!

Ehsan, I'm tempted to ask for branch approvals, since this change is pretty obviously correct...
Assignee: nobody → bzbarsky
Priority: -- → P1
Whiteboard: [need review]
Summary: TH width not updated → 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

Updated

5 years ago
Attachment #606365 - Flags: review?(ehsan) → review+
(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.
http://hg.mozilla.org/integration/mozilla-inbound/rev/a4cd76402136
Target Milestone: --- → mozilla14
Whiteboard: [need review]
Flags: in-testsuite+
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.
Attachment #606365 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/a4cd76402136
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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).
Attachment #606365 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 458924
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.
http://hg.mozilla.org/releases/mozilla-aurora/rev/3d220e21f6f7
status-firefox11: --- → affected
status-firefox12: --- → affected
status-firefox13: --- → fixed

Updated

5 years ago
Duplicate of this bug: 741370
Duplicate of this bug: 729114
Whiteboard: [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.
status-firefox13: fixed → verified
You need to log in before you can comment on or make changes to this bug.