Closed Bug 148994 Opened 21 years ago Closed 19 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.8alpha2

People

(Reporter: st, Assigned: mrbkap)

References

Details

(Keywords: fixed-aviary1.0, fixed1.7, testcase)

Attachments

(8 files, 2 obsolete files)

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.
Forgot my Mozilla version: build 2002041711. SFT :)
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. 
: 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.
"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.
 
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..
I believe that S&ouml;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.
"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.
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.
QA Contact: petersen → madhur
I agree with Sönke and Eric - confirming bug.
Severity: major → normal
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
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.
Severity: normal → minor
Priority: -- → P3
Target Milestone: --- → Future
No, this looks like a bug.  Why would a left margin affect vertical positioning
of floats?
Assignee: attinasi → float
Component: Layout → Layout: Floats
Priority: P3 → --
QA Contact: madhur → ian
Target Milestone: Future → ---
*** Bug 190751 has been marked as a duplicate of this bug. ***
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.
Severity: minor → normal
Summary: [FLOAT] Doesn't care for clear, floated elements overlap → 'clear' misses floats that don't overlap the box (margins)
Priority: -- → P3
Target Milestone: --- → Future
Priority: P3 → P1
not that another testcase is really needed... but its made anyway:

http://placenamehere.com/wd-l/safari_big_clear.2.html
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
The way to fix this bug is by moving clearing from nsBlockBandData to
nsSpaceManager.
OS: Windows 98 → All
Hardware: PC → All
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.
> 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.
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?
Is http://phrogz.net/tmp/min-height-hack.html a duplicate of this bug or not? I can't quite tell.
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.
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. 
Attached patch patch v1 (obsolete) — Splinter Review
This patch moves clearing into nsSpaceManager. All attachments seem to do what
is expected with this patch applied.
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?
Attachment #150006 - Flags: review?(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 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
(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/ .)
(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.
Could you attach the new patch?
Attached patch patch v2 (obsolete) — Splinter Review
Updated patch. Comments addressed.
Attachment #150006 - Attachment is obsolete: true
Attachment #150006 - Flags: review?(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.
Attachment #150044 - Flags: review+
Attached patch patch v3Splinter Review
Addresses all comments - dbaron, could you check this in, please?
Attachment #150044 - Attachment is obsolete: true
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).
Attachment #150051 - Flags: superreview?(dbaron)
Attachment #150051 - Flags: review?(dbaron)
Attachment #150051 - Flags: superreview?(dbaron)
Attachment #150051 - Flags: superreview+
Attachment #150051 - Flags: review?(dbaron)
Attachment #150051 - Flags: review+
Comment on attachment 150051 [details] [diff] [review]
patch v3

Can someone check this in for me, please?
Will do, shortly.
Assignee: core.layout.floats → mrbkap
Fix checked in to trunk, 2004-06-08 12:18 -0700.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Target Milestone: Future → mozilla1.8alpha2
*** Bug 247213 has been marked as a duplicate of this bug. ***
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
What about putting this fix on the Firefox aviary branch as well?
beginning of a testcase (I didn't weed out the CSS yet).
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.
This patch needs to go on the 1.7 branch and aviary.
(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?
That's correct.
*** Bug 146779 has been marked as a duplicate of this bug. ***
*** Bug 182091 has been marked as a duplicate of this bug. ***
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 :(
Nominating to get attention for aviary branch landing (#45). I know its not
really a blocker as such.
Flags: blocking-aviary1.0?
If you think this needs to go into branches, don't nominate; request approvals
on the patch.
I only nominated because I can't request approvals (or I'm blind and don't see
where to do so).
Click the "edit" link for the relevant patch, then toggle the right dropdowns.
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 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.
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
Flags: blocking-aviary1.0? → blocking-aviary1.0-
Comment on attachment 150051 [details] [diff] [review]
patch v3

This patch applies to the 1.7 branch, looking for approval.
Attachment #150051 - Flags: approval1.7.x?
Attachment #150051 - Flags: approval-aviary?
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 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.
Attachment #150051 - Flags: approval1.7.x?
Attachment #150051 - Flags: approval1.7.x+
Attachment #150051 - Flags: approval-aviary?
Attachment #150051 - Flags: approval-aviary+
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.
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.
Except for mirrors.html, those pages should be fixed.  mirrors.html is bug 259275.
Ohter sites no longer render properly. See
http://www.netflix.com/MovieDisplay?movieid=60031268&trkid=135437
Attachment #149946 - Attachment mime type: text/plain → text/html
You need to log in before you can comment on or make changes to this bug.