148994 - 'clear' misses floats that don't overlap the box (margins)

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect, P1)

Description

[Sönke Tesch]

In the attached testcase, <h3>-headers are floated to the left out of the <div>s
that contain them and the text of their paragraph. Although the <div>s have
clear:left set, the headers overlap each other because the <div>s are displayed
just one after another.

Comment 1

[Sönke Tesch]

Attachment: Overlapping, floated elements

Comment 2

[Sönke Tesch]

Forgot my Mozilla version: build 2002041711. SFT :)

Comment 3

[Dylan Foley]

It appears to me that this markup is being rendered correctly. The first floated
header does not cause it's parent div to stretch vertically(this is correct).
When you float the second header, it's vertical position is determined by it's
parent div, and the negative left margin allows it to overlap the first header.
IMHO this bug is invalid. 

Comment 4

[Sönke Tesch]

: When you float the second header, it's vertical position is determined by it's
: parent div, and the negative left margin allows it to overlap the first header.

Well, the <div>s have clear:left set, that should move their top below the
previous header. And that's the bug I see.

Comment 5

[Dylan Foley]

"Well, the <div>s have clear:left set, that should move their top below the
previous header. And that's the bug I see."

But clear will only force things down if there is something in the way. The
floats won't be cleared because they're not in the way. Try changing the float's
margin-left value to -100px and you'll see what I mean.
 

Comment 6

[Sönke Tesch]

Attachment: Testing clear:both in floating elements

Mmh, that seems to be a point. However I'm not sure if the CSS2 docs really
specify it this way.

Rule #2 (http://www.w3.org/TR/REC-CSS2/visuren.html#float-position) says:

  "If the current box is left-floating, and there are any left floating boxes 
   generated by elements earlier in the source document, then for each such 
   earlier box, [..] [the current box'] top must be lower than the bottom of 
   the earlier box."

So, it doesn't speak of where the floats must be defined (i.e. if floats are
independent of each other if they have different parents or not), it's just
"earlier in the source document" - any float.

In short: Looks to me like not a single floating element anywhere in a document
may overlap with another floating one.


Second, there seems to be another problem:

Rule #10, that comes into account if a floating element has itself its
clear-property set (http://www.w3.org/TR/REC-CSS2/visuren.html#flow-control):

  "The top outer edge of the float must be below the bottom outer edge of all 
   earlier [left-/right-] floating boxes".

This doesn't work either, second testcase attached. This one is exactly the
same as the first one, except that the floating <h3> now have clear:both set.

Might be tricky to solve..

Comment 7

[Eric A. Meyer (dead account)]

I believe that Sönke is correct about this being a bug.  The CSS float
rules do not, from what I've read, say that 'clear' is scoped by its parent
element.  It simply says that a cleared element should have the top of its outer
border edge placed just below the outer margin edge of any floated element that
comes earlier in the source.  To quote all of rule #10:

"The top outer edge of the float must be below the bottom outer edge of all
earlier left-floating boxes (in the case of 'clear: left'), or all earlier
right-floating boxes (in the case of 'clear: right'), or both ('clear: both')."
[http://www.w3.org/TR/REC-CSS2/visuren.html#propdef-clear]

Explorer (both Mac and Win) will honor 'clear' for any element in relation to
any float, no matter if it has another parent or not, and I believe they are
correct to do so.  We ought to be doing the same, unless I've missed some
scoping rules somewhere else in CSS2.

Comment 8

[Dylan Foley]

Attachment: Float testcase examples

"In the attached testcase, <h3>-headers are floated to the left out of the
<div>s
that contain them and the text of their paragraph."

The headers are floated left but NOT out of the <div>s that contain them. The
only reason they appear outside the <div>s is the use of negative margins.

The headers are properly floated to the left. In the examples given the floats
end up in the top left corners of their containing div's after clearing
anything in the way(there's actually nothing to be cleared in the examples
given.) THEN they are moved further to the left based on a left margin value of
-150px. Clear should not come into play at this stage.

The attachment shows a few more examples which will hopefully clarify what's
happening.

Comment 9

[Dylan Foley]

Just thought I would add, in case it is not yet clear, the part that you are
missing is 9.5.1 Rule #1:

"The left outer edge of a left-floating box may not be to the left of the left
edge of its containing block. An analogous rule holds for right-floating elements."

So when you float the second header, it is moved to the left edge of it's
containing block - not beyond - and so it never encounters the first one. The
first one gets placed first and it is placed beyond the left edge of the
containing block of the second one. Therefor simply floating the second one left
will not cause it to encounter the first header. It is only when the negative
margin is applied that the second header enters the space occupied by the first
float, this is no longer "floating."

Moving your boxes around with margin sizing has nothing to do with the
float/clear rules. So I still say this bug is invalid.

Comment 10

[Mats Palmgren (inactive)]

Attachment: Minimal testcase

Comment 11

[Mats Palmgren (inactive)]

I agree with Sönke and Eric - confirming bug.

Comment 12

[Sönke Tesch]

As I already wrote, Dylan brought up a good point. Setting a negative margin on
any other element will also lead to overlapping elements, so this behaviour is
in fact not that unexpected for floating elements.
However this IMHO conflicts with the clear-attribute.

So, by now I came to the conclusion that this is more a problem with the CSS
definition than with Mozilla's implementation.

May be set to WONTFIX.

Comment 13

[Boris Zbarsky [:bzbarsky]]

No, this looks like a bug.  Why would a left margin affect vertical positioning
of floats?

Comment 14

[Hixie (not reading bugmail)]

*** Bug 190751 has been marked as a duplicate of this bug. ***

Comment 15

[Hixie (not reading bugmail)]

This can be seen on:
   http://www.hixie.ch/tests/adhoc/css/box/float/006.xml
   http://www.hixie.ch/tests/adhoc/css/box/float/006.html
   http://www.hixie.ch/tests/adhoc/css/box/float/038.html
   http://bugzilla.mozilla.org/attachment.cgi?id=112743&action=view
   http://tests.vangorkum.com/css/float-clear-1.2.html

It is what causes our misrendering of:
   http://wp.netscape.com/fishcam/

----- Additional Bug 190751 Comment 2 From David Baron  2003-01-27 06:49 -----
nsBlockBandData::ClearFloaters is basically broken.  I think we should iterate
over the space manager's mFrameInfoMap looking for floaters to clear instead.

Comment 16

[Chris Casciano]

not that another testcase is really needed... but its made anyway:

http://placenamehere.com/wd-l/safari_big_clear.2.html

Comment 17

[Big John]

Attachment: Shows the bug with no neg margining or extra floats.

Comment 18

[Big John]

This bug does not require more than one float, nor does it require negative 
margining. My test case shows a float, followed by a static div, and that div is 
left margined so as be displayed right of the float. The div contains a cleared 
div, and Moz does not make that cleared div clear the float, apparently because 
they are not vertically aligned. 

None of these elements leave the containing body element.

IMO this is a crystal clear violation of the specs. See: http://bugzilla.
mozilla.org/attachment.cgi?id=132887&action=edit

Comment 19

[David Baron :dbaron:]

The way to fix this bug is by moving clearing from nsBlockBandData to
nsSpaceManager.

Comment 20

[Big John]

Since it's been over seven months with no activity
concerning the problem, can we assume that this 
spec violation will NOT be addressed any time soon?

That would be a shame, since IMO this is the single 
most glaring problem in the Gecko display engine.
It has neccesitated some really complicated page
structuring to get around when using some advanced
floating techniques. Neither Explorer of Opera has
any problem clearing all previous floats, so it's 
rather embarrassing for me to have to explain that
my 3-col float layout can't be made in a simple way 
due to a major spec violation in Gecko.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> can we assume that this spec violation will NOT be addressed any time soon?

It'll be addressed as soon as someone writes a patch....  I don't see people
clamoring to do that, even given that David pointed them at the right code.  :(

> since IMO this is the single most glaring problem in the Gecko display engine.

I think we have bigger problems just with float handling, never mind the rest of
layout.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

I'm looking at this, and I'm confused. In nsBlockReflowState::ClearFloats(), I'm
calling mStateManager->List(). I have two test cases, one which is the 'minimal
testcase,' and the other which is the same testcase, but without the negative
margin (this one works).

In the one that works, the space manager properly lists the first <div> (note:
the manager lists its frame as being Area(html), which I take to mean that it's
the space manager for the root. This would seem to indicate for me, that any
'top level' <div>s would show up in this manager). In 'minimal testcase,' the
space manager claims that it has no bands, and therefore no frames.

Is there another space manager lying around that the div disappears to? What's
going on?

Comment 23

[Gavin Kistner]

Is http://phrogz.net/tmp/min-height-hack.html a duplicate of this bug or not? I can't quite tell.

Comment 24

[Gavin Kistner]

Attachment: Negative margin/zero width test case

Shows two related cases: when the width of the float is zero, and when a
negative margin on the float makes its effective width zero.

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Ignore comment 22, I was more confused then than I am now. I think
nsSpaceManager::List() is doing something other than what I thought, but whatever.

I have a patch that moves clearing into nsSpaceManager, but I still have a
couple of questions about a couple of aspects of it. I'll attach the patch
sometime tomorrow for comments. I think the patch has some problems, but I also
think it is along the right path. 

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

This patch moves clearing into nsSpaceManager. All attachments seem to do what
is expected with this patch applied.

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 150006 [details] [diff] [review]
patch v1

dbaron, could you give this a look and tell me what you think of this patch?

Comment 28

[David Baron :dbaron:]

Comment on attachment 150006 [details] [diff] [review]
patch v1

This looks reasonable.	 |ShouldClearFrame| should either be |protected| or
should just be a static function (not a member function).  I think the latter,
since it has no need for |this|.

Comment 29

[David Baron :dbaron:]

Comment on attachment 150006 [details] [diff] [review]
patch v1

In addition to my previous comments:

>Index: base/src/nsSpaceManager.cpp
>+PRBool
>+nsSpaceManager::ShouldClearFrame(nsIFrame* aFrame, PRUint8 aBreakType)

As I said, just make this:
static PRBool
ShouldClearFrame(...

>+{
>+  PRBool result = PR_FALSE;
>+  const nsStyleDisplay* display = aFrame->GetStyleDisplay();
>+  if (NS_STYLE_CLEAR_LEFT_AND_RIGHT == aBreakType) {
>+    result = PR_TRUE;
>+  }
>+  else if (NS_STYLE_FLOAT_LEFT == display->mFloats) {
>+    if (NS_STYLE_CLEAR_LEFT == aBreakType) {
>+      result = PR_TRUE;
>+    }
>+  }
>+  else if (NS_STYLE_FLOAT_RIGHT == display->mFloats) {
>+    if (NS_STYLE_CLEAR_RIGHT == aBreakType) {
>+      result = PR_TRUE;
>+    }
>+  }
>+  return result;
>+}

If I were rewriting this function body, I'd make it:

{
  PRUint8 float = aFrame->GetStyleDisplay()->mFloats;
  PRBool result;
  switch (aBreakType) {
    case NS_STYLE_CLEAR_LEFT_AND_RIGHT:
      result = PR_TRUE;
      break;
    case NS_STYLE_CLEAR_LEFT:
      result = float == NS_STYLE_FLOAT_LEFT;
      break;
    case NS_STYLE_CLEAR_RIGHT:
      result = float == NS_STYLE_FLOAT_RIGHT;
      break;
    default:
      result = PR_FALSE;
  }
  return result;
}

I think I prefer this. :-)

>+nscoord
>+nsSpaceManager::ClearFloats(nscoord aY, PRUint8 aBreakType)


>+  FrameInfo *frame = mFrameInfoMap;
>+
>+  while (frame) {
...
>+    frame = frame->mNext;
>+  }

This wants to be a for loop:

  for (FrameInfo *frame = mFrameInfoMap; frame; frame = frame->mNext) {
     ...
  }


>Index: html/base/src/nsBlockReflowState.cpp
>   mSpaceManager->List(stdout);
> #endif
>+  
>   const nsMargin& bp = BorderPadding();
>-  nscoord newY = mBand.ClearFloats(aY - bp.top, aBreakType);
>+  nscoord newY = mSpaceManager->ClearFloats(aY - bp.top, aBreakType);
>+
>   mY = newY + bp.top;

I'd prefer not introducing new whitespace here -- I don't see logical
separation.

r=dbaron with those comments

Comment 30

[David Baron :dbaron:]

(And assuming you've run through a bunch of float/clear tests other than this
bug, such as those on other bugs and those in the CSS1 test suite and
http://www.hixie.ch/tests/adhoc/css/box/float/clear/ .)

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #29)
> (From update of attachment 150006 [details] [diff] [review])
> In addition to my previous comments:
> 
> >Index: base/src/nsSpaceManager.cpp
> >+PRBool
> >+nsSpaceManager::ShouldClearFrame(nsIFrame* aFrame, PRUint8 aBreakType)
...
>     case NS_STYLE_CLEAR_LEFT_AND_RIGHT:
>       result = PR_TRUE;
>       break;

Is there a reason that I don't need to check the float display type? Is it
possible to get a non-float in mFrameInfoMap? Do I need to worry about this?

This is done anyway.

> 
> >+nscoord
> >+nsSpaceManager::ClearFloats(nscoord aY, PRUint8 aBreakType)
> 
> This wants to be a for loop:
> 
>   for (FrameInfo *frame = mFrameInfoMap; frame; frame = frame->mNext) {
>      ...
>   }

Done

> 
> 
> >Index: html/base/src/nsBlockReflowState.cpp
> >   mSpaceManager->List(stdout);
> > #endif
> >+  
> >   const nsMargin& bp = BorderPadding();
> >-  nscoord newY = mBand.ClearFloats(aY - bp.top, aBreakType);
> >+  nscoord newY = mSpaceManager->ClearFloats(aY - bp.top, aBreakType);
> >+
> >   mY = newY + bp.top;
> 
> I'd prefer not introducing new whitespace here -- I don't see logical
> separation.

Done.

Comment 32

[David Baron :dbaron:]

Could you attach the new patch?

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v2

Updated patch. Comments addressed.

Comment 34

[David Baron :dbaron:]

Comment on attachment 150044 [details] [diff] [review]
patch v2

>Index: base/src/nsSpaceManager.cpp
>+/* static */
>+PRBool
>+nsSpaceManager::ShouldClearFrame(nsIFrame* aFrame, PRUint8 aBreakType)

I was thinking of a static non-member function, i.e.,

static PRBool
ShouldClearFrame(...

here, and nothing in the header file.

With that change, r=dbaron.

Comment 35

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v3

Addresses all comments - dbaron, could you check this in, please?

Comment 36

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 150051 [details] [diff] [review]
patch v3

I have to remember to do the bugzilla thing, heh. dbaron, (as I said above)
this is the patch that addresses all of your comments. All it needs is sr= and
checkin.

A note: we still fail (at least)
http://www.hixie.ch/tests/adhoc/css/box/float/clear/004-demo.html  however, the
cause of this is outside the scope of this bug (clearing works correctly,
something else is messing up).

Comment 37

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 150051 [details] [diff] [review]
patch v3

Can someone check this in for me, please?

Comment 38

[David Baron :dbaron:]

Will do, shortly.

Comment 39

[David Baron :dbaron:]

Fix checked in to trunk, 2004-06-08 12:18 -0700.

Comment 40

[Boris Zbarsky [:bzbarsky]]

*** Bug 247213 has been marked as a duplicate of this bug. ***

Comment 41

[Mike Kaply [:mkaply]]

OK, strange question.

We were encountering a hang in nsBlockReflowState when printing certain pages on
OS/2.

This fix fixed that problem. Does anyone have any clue why?

Thanks

Comment 42

[u81239]

What about putting this fix on the Firefox aviary branch as well?

Comment 43

[Asa Dotzler [:asa]]

Attachment: possible regression testcase

beginning of a testcase (I didn't weed out the CSS yet).

Comment 44

[David Baron :dbaron:]

Comment on attachment 157418 [details]
possible regression testcase

The trunk behavior looks correct to me -- the .key-point:after { clear: both; }
is the cause.  Since there's no .key-point { float: right; }, this applies to
pretty much anything in the document.

Comment 45

[Mike Kaply [:mkaply]]

This patch needs to go on the 1.7 branch and aviary.

Comment 46

[OstGote!]

(In reply to comment #44)
> (From update of attachment 157418 [details])
> The trunk behavior looks correct to me --
So I guess in Bug 259275 is the code faulty, not Mozilla, right?

Comment 47

[Boris Zbarsky [:bzbarsky]]

That's correct.

Comment 48

[Boris Zbarsky [:bzbarsky]]

*** Bug 146779 has been marked as a duplicate of this bug. ***

Comment 49

[Boris Zbarsky [:bzbarsky]]

*** Bug 182091 has been marked as a duplicate of this bug. ***

Comment 50

[Pascal Chevrel:pascalc]

Could this patch land onto 1.7.x branches ? Thanks to this I am able to easily
reproduce table-based layout like this one:
http://chevrel.org/ie7/tableless.htm

Works in the trunk, works in opera, works in Safari. But if it doesn't work for
firefox 1.0, I don't expect this solution to be popular :(

Comment 51

[Doug Wright]

Nominating to get attention for aviary branch landing (#45). I know its not
really a blocker as such.

Comment 52

[Boris Zbarsky [:bzbarsky]]

If you think this needs to go into branches, don't nominate; request approvals
on the patch.

Comment 53

[Doug Wright]

I only nominated because I can't request approvals (or I'm blind and don't see
where to do so).

Comment 54

[Boris Zbarsky [:bzbarsky]]

Click the "edit" link for the relevant patch, then toggle the right dropdowns.

Comment 55

[Doug Wright]

I don't have an 'edit' link - only view (click on patch name) and Diff (in
'Actions' column).

Apologies for bugspam to everyone else btw.

Comment 56

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 150051 [details] [diff] [review]
patch v3

Before I ask for branch approval on this, it'd be nice to know if this patch
applies directly to those branches.

Comment 57

[Pascal Chevrel:pascalc]

Blake, could you resquest for approval for aviary? Asa just announced that
FF1.0RC is about to be released, it might be + and + difficult to see it land in
the aviary branch with the release date getting near

Comment 58

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 150051 [details] [diff] [review]
patch v3

This patch applies to the 1.7 branch, looking for approval.

Comment 59

[Asa Dotzler [:asa]]

We're at the very end of the road here. What kind of confidence do we have that
this patch doesn't regress anything on the branch? What kind of testing has been
done? 

Comment 60

[Asa Dotzler [:asa]]

Comment on attachment 150051 [details] [diff] [review]
patch v3

a=asa (seconded by chofmann and dbaron) for checkin to the branches. Time is
very short for Firefox 1.0 so this needs to land quickly. Please add the
"fixed-aviary1.0" keyword when this has landed. Thanks.

Comment 61

[David Baron :dbaron:]

Fix checked in to MOZILLA_1_7_BRANCH, 2004-10-22 19:55 -0700.
Fix checked in to AVIARY_1_0_20040515_BRANCH, 2004-10-22 19:56 -0700.

Comment 62

[sairuh (rarely reading bugmail)]

Rafael, here is the layout bug I was talking about. because of this fix, several
pages on mozilla.org need to be updated so they render properly, notably:

http://www.mozilla.org/products/firefox/live-bookmarks.html
http://www.mozilla.org/products/firefox/tabbed-browsing.html
...essentially several feature pages under http://www.mozilla.org/products/firefox/
http://www.mozilla.org/mirrors.html

...and prolly other pages.

Comment 63

[David Baron :dbaron:]

Except for mirrors.html, those pages should be fixed.  mirrors.html is bug 259275.

Comment 64

[dmccammond_watts]

Ohter sites no longer render properly. See
http://www.netflix.com/MovieDisplay?movieid=60031268&trkid=135437