If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED

Status

()

Core
Layout: Tables
P4
major
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Trevan, Assigned: Bernd)

Tracking

({regression, testcase, verified1.9.1})

Trunk
regression, testcase, verified1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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.
(Reporter)

Comment 1

10 years ago
This regression occurred in December.  A trunk build on December 1 doesn't show the bug while January 1 does show it.
(Reporter)

Comment 2

10 years ago
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
Component: General → Layout
Product: Firefox → Core
QA Contact: general → layout
Version: unspecified → Trunk
(Reporter)

Comment 3

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

Updated

10 years ago
Component: Layout → Layout: Tables
QA Contact: layout → layout.tables
(Reporter)

Updated

9 years ago
Flags: wanted1.9.1?
(Reporter)

Updated

9 years ago
Flags: blocking1.9.1?
Blocks: 402567
The url can't be found anymore. Could you perhaps attach your testcase to the bug? Thanks.
(Reporter)

Comment 5

9 years ago
Created attachment 327604 [details]
Testcase

Comment 6

9 years ago
Created attachment 328096 [details]
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;".
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
No longer blocks: 402567
Status: UNCONFIRMED → RESOLVED
Last Resolved: 9 years ago
Flags: wanted1.9.1?
Flags: blocking1.9.1?
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → DUPLICATE
Duplicate of bug: 409736
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.
Blocks: 409736
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: 402567
No longer blocks: 409736
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?
Flags: wanted1.9.1?
Flags: blocking1.9.1?
(In reply to comment #9)
> To DOM:Core?

No.  Layout:Tables is correct.
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Flags: blocking1.9.1+
Keywords: regression, testcase
(Assignee)

Comment 11

9 years ago
Created attachment 336652 [details]
testcase
(Assignee)

Comment 12

9 years ago
Created attachment 336655 [details]
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.
(Assignee)

Comment 13

9 years ago
Robert please reevaluate the blocking status. This should in my opinion not block, although it is a real bug.
Flags: blocking1.9.1+ → blocking1.9.1?
Summary: "table-layout: fixed" functionality changed in v3 → "table-layout: fixed" is not invoked if width changes from auto to fixed dynamically
(Reporter)

Comment 14

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

Comment 16

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

Comment 18

9 years ago
Created attachment 338918 [details] [diff] [review]
patch
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()) {
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P4
(Assignee)

Comment 20

9 years ago
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))
(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.
(In reply to comment #21)
> Bug 322475 is likely to end up depending on the change to DidSetStyleContext.

Actually, it won't.
(Assignee)

Comment 23

9 years ago
An alternative approach would be fixing bug 400776.
(Assignee)

Comment 24

9 years ago
Created attachment 341064 [details] [diff] [review]
patch

I think I did incorporate all your comments.
Attachment #338918 - Attachment is obsolete: true
Attachment #341064 - Flags: superreview?(dbaron)
Attachment #341064 - Flags: review?(dbaron)
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.
(Assignee)

Comment 26

9 years ago
Created attachment 341886 [details] [diff] [review]
revised patch

I used most of the patch for bug 258377, but here comes the minimized version.
Attachment #341064 - Attachment is obsolete: true
Attachment #341886 - Flags: superreview?(dbaron)
Attachment #341886 - Flags: review?(dbaron)
Attachment #341064 - Flags: superreview?(dbaron)
Attachment #341064 - Flags: review?(dbaron)
(Assignee)

Updated

9 years ago
Assignee: nobody → bernd_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
Attachment #341886 - Flags: superreview?(dbaron)
Attachment #341886 - Flags: superreview+
Attachment #341886 - Flags: review?(dbaron)
Attachment #341886 - Flags: review+
(Assignee)

Comment 28

9 years ago
fix + reftest checked in
Status: NEW → RESOLVED
Last Resolved: 9 years ago9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Keywords: fixed1.9.1
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 .
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
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
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in before you can comment on or make changes to this bug.