106158 - [FLOAT] table floated right is incorrectly influencing a non-floated table specified after it

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Marc Attinasi]

This bug is a spinoff from bugscape bug 8601.

The problem is with a right-floated table that is overlapping a non-floated
table. I'm sure that there is a dup for this, but I could not find it, so
opening a new bug.

I'll attach the testcase...

Comment 1

[Marc Attinasi]

Attachment: reduced testcase from original bug (bugscape 8601)

Comment 2

[Marc Attinasi]

Original page is here: http://bugscape.netscape.com/show_bug.cgi?id=8601 (sorry,
internal only).

I have been debugging this and have found at least part of the problem. Going
from the testcase, the table following the floated table is being given an
available width of 0 by the nsReflowState::ComputeBlockAvailSpace method. In
there, there is some logic that considers a table 'clueless' about the
spaceManager (since tables do no flow around floaters) and just gives it the
BlockReflowState's availableSpace - which is 0. This is then used to reflow the
table, causing it the be reduced to its MES. By checking for this case, and
setting the availableWidth to UNCONSTRAINED instead of 0, the table is allowed
to take up the area that it wants, or that its containing cells wants to allow it.

This small change fixes the testcase and the original page, but I am not yet
convinced that it anything more than a band-aid over a faulty
nsReflowState::ComputeBlockAvailSpace algorithm... Basically, the floated table
is taking all of the available space (it is width=100%) but that does not mean
that the following table should be given no space at all - it should instead be
forced to the next line I think. Anyway, I'm still learning this stuff and
investigating, but makign some progress (if tentative).

Comment 3

[Marc Attinasi]

Index: nsBlockFrame.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/html/base/src/nsBlockFrame.cpp,v
retrieving revision 3.463
diff -u -r3.463 nsBlockFrame.cpp
--- nsBlockFrame.cpp    2001/10/16 03:53:08     3.463
+++ nsBlockFrame.cpp    2001/10/25 00:01:30
@@ -2958,6 +2976,13 @@
   nsRect availSpace;
   aState.ComputeBlockAvailSpace(frame, splitType, display, availSpace);

+  if (availSpace.IsEmpty()) {
+    NS_ASSERTION( !PR_TRUE, "given no room to reflow into - bug in BRS?");
+    // we are given no space to reflow into so we use an unconstrained width
and height
+    // in hopes of giving the block some reasonable size...
+    availSpace.width = availSpace.height = NS_UNCONSTRAINEDSIZE;
+  }
+

Comment 4

[Marc Attinasi]

There are two problems that I can see (so far): 

1) the available space from the spaceManger / blockBandData is coming back 0,
and we reflow the table into that which makes it really small. This is patched
by the little hack I attached prior.

2) when a table will not fit on the same line as a floater (left or right) we
are not moving it down to the next line. This is ther ellusive part for me - I
cannot figure out how to make the table land on the next line instead of either
overlapping the floater (when it is right-aligned) or expand the containing
block (when it is left-aligned).

Fixing (2) will fix a lot of floater bugs I think, but everything I have tried
fails thusfar.  CC'ing waterson who may take this to work on (since he has more
floater experience than I do).

Also, the 'original' testpage is fixed by just fixing (1) - it is here:
http://www.geocities.com/debugweyers/aoltips.html - but, only if the imges on
the line are already loaded (probably a bug in imageFrame that is is not causing
sufficient reflow when fully loaded...)

Comment 5

[Chris Waterson]

Attachment: testcase that works in 4.x but not in mozilla

Comment 6

[Chris Waterson]

Stealing from marc. Playing around with this a bit more: attachment 55684 [details] is a
test case that ``works'' in 4.x, but breaks in mozilla.

Comment 7

[Chris Waterson]

It looks to me like the problem is that the percentage width is not being
installed on the table's outer frame. Instead, it's on the table frame, which
the block can't see. This means that the table outer has no computed width, and
so the block attempts to constrain it since the in-flow table to overlaps the
right-floated table.

dbaron and I saw a similar problem last night when adding top margin to the
table: no matter how much to margin we added, the in-flow table remained
constrained. Again, the margin was added to the table frame, the block saw the
table outer frame as overlapping the float.

Comment 8

[karnaze (gone)]

The outer table handles/contains the margins for the table and the caption. If 
the reflow state of the outer table had a computed width, would that solve the 
problem? If the outer/inner table needs to change, please reassign this to me.

Comment 9

[Chris Waterson]

Switching the outer and inner style contexts in
nsCSSFrameConstructor::CreateTableFrame() ``fixes'' the problem.

Comment 10

[Chris Waterson]

Ok karnaze, I'm gonna pawn this off on you. Send it back if you need to.

Comment 11

[Chris Waterson]

Chris K., some more info.

What's happening is that in nsBlockReflowContext::ReflowBlock(), we're treating
the table outer frame as a block frame. When we create the reflow state in which
to flow the table's outer frame, we try to access some of the stylistic
properties (including margins and width) so that we can _provide_ a computed
width for the frame. Since these properties exist on the table's inner frame,
the block code assumes that there are no constraints or preferences for the
frame's width. IOW, it flows it at the minimum possible width because of the
overlap with the right-floated element.

If we could promote the margin and width information to the outer frame, then it
would be possible for the block code to detect that the table has a preference
for its width, and we could flow the table appropriately.

Comment 12

[karnaze (gone)]

Attachment: patch to have nested percent table return appropriate width

Comment 13

[karnaze (gone)]

The patch cleans up the outer table a bit and makes us compatible with IE (on 
the attachments) without regressing tables. It is odd that the cell block is 
reflowing its children with an available width of 0.

Comment 14

[Bernd]

Chris couldn't you add again a line 

childRS.mComputedWidth = PR_MAX(0, childRS.mComputedWidth);

You know I am paranoid

Comment 15

[Bernd]

Chris, 

why can we drop the childRS.mComputedHeight computation?
Is it secure to assume that childRS.availableWidth != NS_UNCONSTRAINEDSIZE?
if aChildFrame == captionframe then the if condition will be fullfilled?

 if ( ((aChildFrame == mInnerTableFrame) && 
   ((nsTableFrame*)aChildFrame)->GetMinWidth() <= childRS.availableWidth) ||   
      (aChildFrame != mInnerTableFrame)) {

Why  did we  assert before, if this happened?

Why we drop the margins?
 

Comment 16

[Alexandru Savulov]

Comment on attachment 55888 [details] [diff] [review]
patch to have nested percent table return appropriate width

r= alexsavulov

Comment 17

[Alexandru Savulov]

Comment on attachment 55888 [details] [diff] [review]
patch to have nested percent table return appropriate width

r= alexsavulov (forgot to set flag)

Comment 18

[karnaze (gone)]

Bernd, the diff is a bit confusing. I removed the code covering mComputedWidth 
and mComputedHeight (2nd and 3rd top level if statements) that was only there 
because boxes could contain tables. Now they can't so mComputedWidth on an outer 
table should only be 0 or NS_UNCONSTRAINEDSIZE. I had commented that this code 
should be removed. The change occurred in the 1st top level if statement in the 
patch and I made it so that captions would not be affected and yet fix the bug. 
I will add a max(0 ..) check, even though it wasn't in 1st if statement.

Comment 19

[Chris Waterson]

karnaze: as I said above, the problem is that the block code can't see the box
constraint properties (width, height, margin, etc.) because they're on the table
frame. The box code should do the right thing if you can propagate the stylistic
information for width and margin (at least) from the table frame to the table
outer frame. So...this feels like its wallpapering over the problem.

Don't we want those properties on the outer frame _anyway_? I mean the margin
should apply on the outer frame (not offset the inner frame within the outer frame).

Comment 20

[Bernd]

from Chris Watersons comment:

>The box code should do the right thing if you can propagate the stylistic
>information for width and margin (at least) from the table frame to the table
>outer frame

this would half the number of html-table bugs :-), may be this is to optimistic,
but at least the captions positioning bugs would go away. The way we center
tables by filling with the outer frame the available width and then compute the
margins we need to place the inner table frame and the caption inside this big
outer frame does not make me happy. See bug 87277 and bug 60365 which would be
fixed by following Watersons recommendation (due to bug 87663 and my limited
abilities I am not able to implemt this :-() 

Comment 21

[karnaze (gone)]

I first tried letting the outer table inherit the width and that caused a bunch 
of other problems. Keep in mind that the outer table is a combination of the 
inner table and the caption and it handles the margins for the inner and 
caption, so even if this had worked in this case, it would fail in cases 
involving captions. I suppose it is possible when constructing the outer table's 
reflow state to consider the inner and caption, but it may get a bit complex. 
So, I think it is better to just consider that an outer table is always auto 
width (but it can contain a non auto width inner and/or caption) and be able to 
deal with the abstraction.

I had to include special logic dealing with percent nested tables in the past 
and this patch just expands that logic. It is difficult to do this in the reflow 
state code, so the outer table fixes up the mComputedWidth of the inner table.

Bernd, I don't agree with your assessment. Something has to do the inner/caption 
margin and positioning and what better entity than the outer table frame. You 
should have seen the bugs when blocks did the inner and caption independently.

Comment 22

[Chris Waterson]

Sounds okay then. sr=waterson

Comment 23

[Bernd]

to bad, I am afraid Chris, that you are right. I did not see those bugs, because
I was just innocent when those bugs have been solved. So there wan't be a patch
that  fixes all these bugs at once. Looks like the tedious work continues.

Bernd

Comment 24

[Alexandru Savulov]

r= alexsavulov

Comment 25

[Kevin McCluskey (gone)]

Chris checked the patch into the trunk after it passed all table regression tests.
Adding edt0.9.4 keyword

Comment 26

[Kevin McCluskey (gone)]

patch was checked into the trunk on 2001-11-01 06:53:57

Peterson: Can you verify that it is fixed on the trunk? Thanks!

Comment 27

[Kevin McCluskey (gone)]

Resolving as fixed, since the patch was checked into the trunk

Comment 28

[Chris Petersen]

Verified fixed in the Nov 1 (2001-11-01-03) build. Tested under Windows ME.

Comment 29

[Judson Valeski]

please check into the 0.9.4 branch, and mark with the "fixed0.9.4" keyword once
it's in.

Comment 30

[karnaze (gone)]

-->m094 branch.

Comment 31

[Chris Petersen]

Verified on the 2002-01-14-23-0.9.4ec build.