Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

Comment 4

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

Comment 5

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

Comment 6

5 years ago
Created attachment 606365 [details] [diff] [review]
Correctly handle changes to column widths in fixed-layout tables.
Attachment #606365 - Flags: review?(ehsan)
(Assignee)

Comment 7

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

Updated

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

Comment 9

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/a4cd76402136
Target Milestone: --- → mozilla14
(Assignee)

Updated

5 years ago
Whiteboard: [need review]
(Assignee)

Updated

5 years ago
Flags: in-testsuite+
(Assignee)

Comment 10

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

Comment 13

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

Comment 14

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

Updated

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