426629 - "table-layout: fixed" is not invoked if width changes from auto to fixed dynamically

Status: VERIFIED FIXED

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

Description

[Trevan]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008040112 Minefield/3.0pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008040112 Minefield/3.0pre

Setting table-layout before the element is placed into the dom causes it to be ignored.  Firefox 2 worked correctly while Firefox 3 is broken.  This functionality is used in Omniture and so their data grids have issues in Firefox 3.

Reproducible: Always

Steps to Reproduce:
1. Open up the url in Firefox 2; verify that the second column is cut-off with overflow: hidden
2. Open up the url in Firefox 3; verify that the second column ignores the width of the column (as set by the first row)
Actual Results:  
The second column overflows and changes the width of the table.

Expected Results:  
The second column should obey the width set and hide overflowing content.

Comment 1

[Trevan]

This regression occurred in December.  A trunk build on December 1 doesn't show the bug while January 1 does show it.

Comment 2

[Trevan]

Ok, It happens with this version:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007120505 Minefield/3.0b2pre

But not with this one:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007120405 Minefield/3.0b2pre

Comment 3

[Trevan]

I made a slight change to the test page.  It now sets table-layout twice in javascript.  Once before the table is inserted into the dom, and once after.  If you comment out the first occurrence, the table renders correctly.  But as long as the first one is in, the table renders incorrectly.

Comment 4

[Martijn Wargers (dead)]

The url can't be found anymore. Could you perhaps attach your testcase to the bug? Thanks.

Comment 5

[Trevan]

Attachment: Testcase

Comment 6

[yliang]

Attachment: This is another case to show the problem.

First table: The first column's width is 0.2in. The second column's width is 0.2in. The third column's width is 0.2in.
Second table: The first column's width is 0.6in.

Expect: 0.6in = (0.2in + 0.2in + 0.2in)
Result: 0.6in < (0.2in + 0.2in + 0.2in)

It works well with the IE and the Firefox 2.0. But the Firefox 3 changed the behavior of "table-layout:fixed;".

Comment 7

[Lucas Malor (mail: c6kfnkn2uc AT snkmail DOT c0m)]

The bug it's not related to layout fixed tables, but with automatic layout tables. If you remove any table-layout:fixed CSS and DOM settings in your examples, the problem happens anyway. This is a duplicate of Bug 409736

Comment 8

[Daniel Holbert [:dholbert]]

Not a dupe -- the original description & testcase 1 indicate that this specifically relates to inserting content into the DOM via JS. (I tested locally, and when I used a static testcase, I got expected behavior, but I got unexpected behavior with JS like in the testcase.)

If you're not absolutely 100% sure that a bug's a dupe, it's customary to just say "Dupe of bug XXX?" rather than simply marking as a dupe.

Also, please don't clear the "Blocks" list, when someone has gone to the trouble (Comment 2) of tracking down an exact regression range and finding the patch that triggered this behavior.

Comment 9

[Lucas Malor (mail: c6kfnkn2uc AT snkmail DOT c0m)]

Uops. You are right, I didn't checked well the JS code in testcase 1. Re-adding flags.

It seems to me there's another related bug, with a style property changed with JS but not rendered by Fx.

To DOM:Core?

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #9)
> To DOM:Core?

No.  Layout:Tables is correct.

Comment 11

[Bernd]

Attachment: testcase

Comment 12

[Bernd]

Attachment: testcase 1 fixed

The problem here is that if you specify fixed layout but when you append it to the dom the width is auto so the fixed layout is not invoked. Later you set the width and then you expect the fixed layout to be really invoked, this will not happen. And it never worked before see attachment 336652 [details]. The regression is due to a different execution of the script.

Only IE gets attachment 336652 [details] right. Webkit is as wrong as we are.

Comment 13

[Bernd]

Robert please reevaluate the blocking status. This should in my opinion not block, although it is a real bug.

Comment 14

[Trevan]

But the original test case worked in previous iterations of Firefox and then it was broken.  So there is a regression.

Also, I requested for it to be blocking as Omniture SearchCenter was broken with this regression as it doesn't figure out the width of the table until after it has been attached to the document.  All other browsers but FF3 work with it.  I figured out a hack to get around the issue so it isn't all that important now, but it'd be nice to remove the hack.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Should be simple to fix.  I'd recommend:

 Changing nsIFrame::SetStyleContext and DidSetStyleContext so the old style context is passed to DidSetStyleContext.

 Changing nsTableFrame::DidSetStyleContext so that it checks for changes in the conditions that cause table-layout to apply if 'table-layout' is 'fixed'.

Comment 16

[Bernd]

> All other browsers but FF3

Thats only due to the javascript execution and how it is scheduled, with a single layout break point (quering offsetHeight) one can probably cause the wrong behavior in other browsers too, this works by accident not by design. See the attached testcase.

> Omniture SearchCenter was broken
> with this regression as it doesn't figure out the width of the table until
> after it has been attached to the document.

May be I was not clear enough, I diagnosed the problem down to the cause, which is old bug in our code base, so there is no need to find a specific regression date. Further I provided a way to fix the website in the mean time:

Its not important to what width you set it before its only important that you set it to a fixed width before you apply the fixed layout. Later you can change the width to any width you want and the table will follow.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If this is a real bug that surfaced in FF3 I'd still like to block on it for 1.9.1, even if the bug is technically not a regression, especially since it doesn't seem hard to fix.

Comment 18

[Bernd]

Attachment: patch

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 338918 [details] [diff] [review]
patch

Not sure why you didn't request review, but this looks fine except for:

>diff --git a/layout/generic/nsIFrame.h b/layout/generic/nsIFrame.h
>   void SetStyleContext(nsStyleContext* aContext)
>   { 
>     if (aContext != mStyleContext) {
>+      nsStyleContext* oldStyleContext = nsnull;
>       if (mStyleContext)
>-        mStyleContext->Release();
>+        oldStyleContext = mStyleContext;        
>+      

This could just be
        nsStyleContext *oldStyleContext = mStyleContext;

>diff --git a/layout/tables/nsTableFrame.cpp b/layout/tables/nsTableFrame.cpp
>+NS_IMETHODIMP
>+nsTableFrame::DidSetStyleContext(nsStyleContext* aOldStyleContext)
>+{
>+   if (aOldStyleContext->GetStyleTable()->mLayoutStrategy != NS_STYLE_TABLE_LAYOUT_AUTO &&
>+       GetStyleTable()->mLayoutStrategy != NS_STYLE_TABLE_LAYOUT_AUTO) {
>+       
>+     const nsStyleCoord& oldWidth = aOldStyleContext->GetStylePosition()->mWidth;
>+     PRBool wasAuto = (oldWidth.GetUnit() == eStyleUnit_Auto) ||
>+                      (oldWidth.GetUnit() == eStyleUnit_Enumerated &&
>+                       oldWidth.GetIntValue() == NS_STYLE_WIDTH_MAX_CONTENT);
>+                         
>+     const nsStyleCoord& width = GetStylePosition()->mWidth;
>+     PRBool isAuto = (width.GetUnit() == eStyleUnit_Auto) ||
>+                     (width.GetUnit() == eStyleUnit_Enumerated &&
>+                      width.GetIntValue() == NS_STYLE_WIDTH_MAX_CONTENT);
>+     if( wasAuto != isAuto) {

I think, instead of writing this out, you should modify the current IsAutoLayout method into a static method that takes a style context argument, and then add a new IsAutoLayout with the same signature as the current one that just calls nsTableFrame::IsAutoLayout(GetStyleContext()).  Then the above code can just be:

  if (nsTableFrame::IsAutoLayout(aOldStyleContext) != IsAutoLayout()) {

Comment 20

[Bernd]

David:

>This could just be
>        nsStyleContext *oldStyleContext = mStyleContext;

Thats good, I have little to no experience with that addref stuff.

>Not sure why you didn't request review:

Because as you noticed its ugly and then I was wondering why I have the method at all. You made this fine and nifty function in bug 444928
so we would need the old style context only to see that we are not initializing and check then if 
IsAutoLayout() != (LayoutStrategy()->GetType() == nsITableLayoutStrategy::Auto))

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #20)
> >This could just be
> >        nsStyleContext *oldStyleContext = mStyleContext;
> 
> Thats good, I have little to no experience with that addref stuff.

Nothing to do with AddRef stuff, just that:
  foo = null;
  if (bar)
    foo = bar;
is the same as
  foo = bar


Two more issues, though:

(1) You should only set a layout strategy on the first-in-flow; you should leave it null on the later in-flows.

(2) You shouldn't delete the old strategy until you've already allocated the new one; in case of allocation failure you're better off leaving the old one.


Bug 322475 is likely to end up depending on the change to DidSetStyleContext.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #21)
> Bug 322475 is likely to end up depending on the change to DidSetStyleContext.

Actually, it won't.

Comment 23

[Bernd]

An alternative approach would be fixing bug 400776.

Comment 24

[Bernd]

Attachment: patch

I think I did incorporate all your comments.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, I was thinking you would incorporate the end of comment 20 as well, which would mean that the patch would only need to touch nsTableFrame.cpp.

Comment 26

[Bernd]

Attachment: revised patch

I used most of the patch for bug 258377, but here comes the minimized version.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 341886 [details] [diff] [review]
revised patch

>diff --git a/layout/tables/nsTableFrame.cpp b/layout/tables/nsTableFrame.cpp
>--- a/layout/tables/nsTableFrame.cpp
>+++ b/layout/tables/nsTableFrame.cpp
>@@ -2221,16 +2221,38 @@ nsTableFrame::GetCollapsedWidth(nsMargin
>             width += cellSpacingX;
>         }
>       }
>     }
>   }
>   return width;
> }
> 
>+NS_IMETHODIMP
>+nsTableFrame::DidSetStyleContext(void)

To merge with tip, this should be "/* virtual */ void", not NS_IMETHODIMP.  (And, thus, the "return NS_OK" should be "return", except the one at the end just removed.)

And you should write "()" rather than "(void)".  The latter is a habit from C, where () means "I didn't declare the parameters" and "(void)" means "no parameters".  In C++, they both mean "no parameters", and you should use "()".

>   /** @see nsIFrame::Destroy */
>   virtual void Destroy();
>+  
>+  /** @see nsIFrame::DidSetStyleContext */
>+  NS_IMETHOD DidSetStyleContext();
>+ 

I'd skip the double blank line afterwards.  Also change the NS_IMETHOD to "virtual void" to match tip.

r+sr=dbaron with that

Comment 28

[Bernd]

fix + reftest checked in

Comment 29

[Aakash Desai [:aakashd]]

Bernd, please add the link to the changeset for any bugs you've applied patches to.

For this bug, http://hg.mozilla.org/mozilla-central/rev/d16f06b361ad .

Comment 30

[Aakash Desai [:aakashd]]

I apologize for the second comment about this, but here is the changeset for 1.9.1 as well. 

http://hg.mozilla.org/releases/mozilla-1.9.1/log?rev=426629

Comment 31

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090406 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090406045355

and

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090331 Shiretoko/3.5b4pre (.NET CLR 3.5.30729) ID:20090331041754