Last Comment Bug 50630 - [FLOAT] float should be as high as previous line box
: [FLOAT] float should be as high as previous line box
Status: VERIFIED FIXED
[Hixie-P2][CSS1-5.5.25] (high profile...
: css1, testcase
Product: Core
Classification: Components
Component: Layout: Floats (show other bugs)
: Trunk
: All All
: P1 normal with 28 votes (vote)
: mozilla1.9.1a1
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
http://dbaron.org/css/test/sec414
: 161254 207743 236603 243972 249723 364268 369421 384838 406750 413352 423980 424973 461279 472381 492314 (view as bug list)
Depends on: 488725 reflow-refactor 439204 441683
Blocks: float 319603 104166 294306 360965 acid3
  Show dependency treegraph
 
Reported: 2000-08-28 20:07 PDT by Hixie (not reading bugmail)
Modified: 2009-05-11 01:22 PDT (History)
57 users (show)
roc: wanted1.9.1+
roc: wanted1.9.0.x-
roc: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test case for the vertical positioning of floats. (3.83 KB, text/html)
2001-02-28 07:55 PST, Tim Hunt
no flags Details
Shorter testcase where the 2 examples should NOT render the same (1.09 KB, text/html)
2002-03-17 02:12 PST, Stefan Huszics
no flags Details
another testcase (1.15 KB, text/html)
2002-09-20 14:17 PDT, Madhur Bhatia
no flags Details
Test case (almost) directly from CSS spec (539 bytes, text/html)
2003-02-02 23:58 PST, Nathan Kurz
no flags Details
fix (44.88 KB, patch)
2008-06-08 21:46 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Review
fix v2 (37.60 KB, patch)
2008-06-09 02:41 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
dbaron: review+
dbaron: superreview+
Details | Diff | Review
fix v3 (40.69 KB, patch)
2008-06-09 16:50 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
roc: review+
roc: superreview+
Details | Diff | Review
Floats should appear beside inline content and not below it (1.51 KB, application/xml)
2008-06-21 16:50 PDT, Alan Gresley
no flags Details
should whitespace before the float count? (453 bytes, text/html)
2008-06-22 00:19 PDT, Bruno Fassino
no flags Details

Description Hixie (not reading bugmail) 2000-08-28 20:07:30 PDT
STEPS TO REPRODUCE:
   1. Make the second of two blocks float, where the first block has only one
      line box.

ACTUAL RESULTS:
   The floater will be below the line box.

EXPECTED RESULTS:
   The floater should be above the line box.

This can be seen in the "This is short." part of the above URL, where the float
contains the following text:
: This paragraph is floated left, and since it must be placed as high as 
: possible without its top being higher than the top of any preceding line-
: boxes, the paragraph that says "This is short" should appear directly to the 
: right of the top border of this paragraph.
Comment 1 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2000-08-28 20:15:05 PDT
CSS3 may change this...
Comment 2 Hixie (not reading bugmail) 2000-08-28 22:44:27 PDT
That would be a good thing. Doing what the current spec asks for requires us to
reflow content that came a whole block before the float, which is silly.
Comment 3 Hixie (not reading bugmail) 2000-10-05 16:16:14 PDT
Actually I take that back. We should only move the float up that high if there
is enough room left on the line box. It requires us to reposition earlier
content but not reflow it.
Comment 4 Hixie (not reading bugmail) 2000-10-05 16:21:21 PDT
This also applies to the simpler case of a float occuring half way through a 
line, before that line has been fully layed out.

   <block> some text  <float/>  some more text </block>

...should lay out like this:

    some text some +------+
    more text.     |      |
                   +------+

...and not like this, as we are doing:

    some text some more
    text.          +------+
                   |      |
                   +------+

The CSS1 way (as described as the correct behaviour here) allows for some cool
effects. I would hope that CSS3 does _not_ change this.
Comment 5 Tim Hunt 2001-02-28 07:55:06 PST
Created attachment 26401 [details]
Test case for the vertical positioning of floats.
Comment 6 Tim Hunt 2001-02-28 08:09:31 PST
The above testcase has 4 paragraphs, each containing one float.

In the first two paragraphs, the float is the first thing in the paragraph. In
the second two paragraphs, the float appears after 12 words.

Paragraphs 1 and 3 have the float on the left. Paragraphs 2 and 4 have it on the
right.

The first two paragraphs work as expected.

In paragraphs 3 and 4, when the browser window is wide enough for the first 12
words of the paragraph and the float to fit side by side, then I think that the
top of the float should be level with the top of the paragraph.

I think this because of description of the float property in the CSS2 spec. 
<http://www.w3.org/TR/REC-CSS2/visuren.html#propdef-float>. Number 8 of "the
precise rules that govern the behavior of floats" states "A floating box must be
placed as high as possible", and rule 6 states "The outer top of an element's
floating box may not be higher than the top of any line-box containing a box
generated by an element earlier in the source document."

"
Comment 7 Chris Waterson 2001-03-02 09:24:43 PST
Pulling from buster's list.
Comment 8 Kevin McCluskey (gone) 2002-02-27 13:54:05 PST
Reassigning to Alex.
Comment 9 Stefan Huszics 2002-03-17 02:12:17 PST
Created attachment 74592 [details]
Shorter testcase where the 2 examples should NOT render the same

BTW... Opera 6.01 handles this correctly *nudge* ,)
Comment 10 Esben Mose Hansen 2002-03-22 09:18:59 PST
OK, I'll have a shot at this. Hope I can make it :-)
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2002-03-22 14:25:34 PST
This probably requires some pretty significant changes to do correctly.  See
also comment 2.
Comment 12 Alexandru Savulov 2002-03-25 15:12:23 PST
retaking
Comment 13 Alexandru Savulov 2002-03-27 18:09:28 PST
oh, I forgot to really retake
Comment 14 Greg K. 2002-07-11 19:52:53 PDT
Reconfirmed using FizzillaCFM/2002070913 to test the shorter testcase. Both
examples look the same. Setting All/All.
Comment 15 Kai Lahmann (is there, where MNG is) 2002-08-06 04:20:54 PDT
*** Bug 161254 has been marked as a duplicate of this bug. ***
Comment 16 Madhur Bhatia 2002-09-20 14:17:48 PDT
Created attachment 100017 [details]
another testcase


in mozilla, the image contained within the left / right floated paragraph is
displayed a line below the expected one.
The right floated paragraph also appears to be one line below the expected one.


opera shows this testcase correctly.
Comment 17 Nathan Kurz 2003-02-02 23:58:33 PST
Created attachment 113378 [details]
Test case (almost) directly from CSS spec

The CSS1 (http://www.w3.org/TR/REC-CSS1#floating-elements), 
    CSS2 (http://www.w3.org/TR/REC-CSS2/visuren.html#floats),
and CSS3 (http://www.w3.org/TR/css3-box/#floating) specs are all 
strikingly clear about how a float should be positioned relative the 
line it is contained in.  Here is the example they give, modified just
enough to have a valid image and describe what you should see.
Comment 18 Nathan Kurz 2003-02-03 02:15:40 PST
I could be wrong, but I think some of the comments for this bug make the problem
seem more complicated than it is.  At least for the test case in the previous
comment (referred to as the 'simpler case' in these comments, but it's not clear
to me there is another case), CSS1, CSS2, and CSS3 all agree on where the float
should be positioned.  If a float occurs in a line of text, it is placed (if
possible) at the same height as the text in which it occurs is placed, otherwise
(if blocked by another float) it is placed lower.  This way, things like margin
notes will appear alongside the same line as the text to which they pertain.

To my naive eye, this looks like a logic bug in existing code rather than
something that requires significant structural changes.  The code for
nsBlockReflowState::AddFloater
(http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsBlockReflowState.cpp#655)
has a comment about not conforming to the spec, but then seems to have all the
parts for handling the case correctly (based on the value of
aLineLayout.CanPlaceFloaterNow()).  The value in aLineLayout is set in
nsLineLayout::PlaceFrame
(http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsLineLayout.cpp#1612).

CanPlaceFloaterNow() is always false unless the float is the first thing on the
line.  My naive guess is that since the checks in PlaceFrame() are absolute
(psd->mX != psd->mLeftEdge) anything placed out of flow will always end up below
current line.  Could someone more knowledgeable about layout look at these two
functions and see if they are doing the right thing logically?  Or is the
problem that the text preceding the float has already been placed?  

I don't want to learn about layout :(
I just want it to work well enough for margin notes! :)


 





Comment 19 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-02-03 05:56:08 PST
The problem is that the text preceding the float has already been placed, to
some extent.  Fixing it shouldn't be that hard, though, now that I think of it,
since it's before the stage when we do horizontal alignment.

Taking bug.
Comment 20 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-02-03 07:05:40 PST
Actually, this is pretty messy, since nsBlockReflowState::FlowAndPlaceFloater
needs to be untangled into reflowing the floater and placing it in two separate
functions.
Comment 21 Chris Casciano 2003-03-19 11:57:34 PST
another manifestation of this bug:
http://placenamehere.com/Mozilla/lifloattest.html
Comment 22 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-05-20 19:21:24 PDT
(I wonder what nsLineLayout::UpdateFrames does, or would do...)
Comment 23 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-05-31 11:50:04 PDT
*** Bug 207743 has been marked as a duplicate of this bug. ***
Comment 24 Boris Zbarsky [:bz] (Out June 25-July 6) 2004-03-06 00:01:06 PST
*** Bug 236603 has been marked as a duplicate of this bug. ***
Comment 25 oliv 2004-04-13 09:21:27 PDT
IE has it wrong, but Konqueror (thus Safari) has it right. Is there still
someone working on this?
Comment 26 Boris Zbarsky [:bz] (Out June 25-July 6) 2004-05-18 14:50:28 PDT
*** Bug 243972 has been marked as a duplicate of this bug. ***
Comment 27 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-07-04 10:23:24 PDT
*** Bug 249723 has been marked as a duplicate of this bug. ***
Comment 28 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-06-16 15:38:06 PDT
See also the patch in bug 317278, which may be slightly related (although perhaps it's just a crash that we would have started hitting more often if we fixed this first).
Comment 29 Uri Bernstein (Google) 2006-12-19 12:55:20 PST
*** Bug 364268 has been marked as a duplicate of this bug. ***
Comment 30 Mats Palmgren (:mats) 2007-02-05 22:52:36 PST
*** Bug 369421 has been marked as a duplicate of this bug. ***
Comment 31 Gérard Talbot 2007-06-18 00:20:11 PDT
Hello all,

Is bug 384838 a duplicate of this bug? Otherwise do you know a bug number involving misalignment/mispositioning of floated, non-replaced inline element?
Just asking..
Comment 32 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-06-18 00:41:25 PDT
*** Bug 384838 has been marked as a duplicate of this bug. ***
Comment 33 Mats Palmgren (:mats) 2007-12-04 06:26:42 PST
*** Bug 406750 has been marked as a duplicate of this bug. ***
Comment 34 Leon Sorokin 2008-01-04 18:43:47 PST
what is the status of this bug? i just ran into it a few days ago as well.

thanks,
Leon
Comment 35 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-01-24 15:11:41 PST
*** Bug 413352 has been marked as a duplicate of this bug. ***
Comment 36 Mats Palmgren (:mats) 2008-03-19 20:00:45 PDT
*** Bug 423980 has been marked as a duplicate of this bug. ***
Comment 37 old9 2008-03-25 05:13:01 PDT
what's going on now with this one? will it be fixed in the firefox3 final?
Comment 38 Patrick Dark 2008-04-08 06:28:43 PDT
Bug 424973 (Acid3 blocker) should probably be marked as a duplicate of this bug while this bug should probably be marked as blocking bug 410460 (Acid3).
Comment 39 Scott R. Godin 2008-04-11 06:53:52 PDT
I've run into this bug TWICE in the last week on two completely separate websites, because I develop primarily in other more compliant browsers and then test in firefox and safari, and only then fix for MSIE. 

I cannot tell you how frustrating it is to discover that this bug has been known since the dawn of HTML 4.01 and is STILL NOT FIXED.

I had to resort to using positioned content rather than floated content, because with a restaurant menu, reversing the order of description and price is semantically incorrect and would look ridiculous with css off. 

please fix this bug before you release FF3, and if at ALL possible get it into the next release of FF2 already. it's over eight years old, this bug, and Konqueror and Opera and Safari all get this part right. it's only You and MSIE now, that (are) still (made of) fail.


Comment 40 :Aryeh Gregor (away until August 15) 2008-04-11 07:13:43 PDT
Gecko 1.9 (Fx3) is pretty much feature-frozen at this point, AFAIK.  If you want to request that this block Fx3 release, you can set the flag "blocking1.9?" and someone will either confirm or deny (probably deny).  Changes like this are never back-ported to previous releases like Fx2; Firefox is release-based, not continuous integration, and that means old releases are feature-frozen.

Please keep some perspective.  There are a lot of important things that need to be fixed and a lot of them are very old and there aren't enough people to fix them.  Everyone has their pet issue and some people are going to have to be disappointed if they want other people to do the work for them, because your priorities are not always going to agree with the developers'.  If you would really like to see this fixed soon, you could help by writing a patch.
Comment 41 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-06-06 09:20:27 PDT
*** Bug 424973 has been marked as a duplicate of this bug. ***
Comment 42 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-06-08 15:41:24 PDT
I don't think comment #20 is necessary.

Post-reflow-branch we know the width of a float before we reflow it. So here:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockReflowState.cpp#574
instead of just checking CanPlaceFloatNow() (which really should be renamed "HaveNonEmptyContentInLine" or something like that), we should go ahead if there's no non-empty content in the line OR if the float's width fits in the available width left in the line. The float placement and reflow code doesn't need to change.

The other thing we need to do is --- for left floats --- move the inline frames to the right to make room for the insertion of the float. The least invasive way to do this is to change nsLineLayout::UpdateBand to move the frames. When multiple left floats are being inserted before some already-laid-out inline content, it would be more efficient to move the frames just once in HorizontalAlignFrames, but leaving some frames with the wrong coordinates during inline reflow sounds a bit confusing and I suspect that moving them in UpdateBand will not be a significant performance issue.
Comment 43 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-06-08 17:02:58 PDT
I thought about checking whether there's enough available space on the line in FlowAndPlaceFloats and basically eliminating the "below current line floats" list, always placing floats when we encounter their placeholders. But that seems difficult since we can't yet know the height of the line box, so we really can't place the floats that don't fit on the current line.

In fact, the current code that allows FlowAndPlaceFloats to place floats below the current line when we haven't finished reflowing the current line is broken, because it doesn't take into account the space that will be required by the current line. I will attach a testcase that demonstrates this.

Basically, as soon as a float doesn't fit on the current line, we need to make that float --- and all subsequent floats, even if they do fit on the current line, because float order needs to be preserved --- go to the below-current-line-floats list.
Comment 44 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-06-08 21:46:34 PDT
Created attachment 324242 [details] [diff] [review]
fix

The parts to this patch:

-- Rename CanPlaceFloatNow() to LineIsEmpty()

-- Set LineIsEmpty to false when we place a frame with nonempty content (instead of checking whether the x-advance has changed). Fixes test 50630-2.html, which tests whether zero-width but nonempty content on a line prevents trimming of whitespace after it due to start-of-line (it should).

-- Add nsBlockFrame::ComputeFloatWidth to do just what it says...

-- In nsBlockReflowState::AddFloat, don't allow a float to be placed if we've already pushed one or more floats to the below-current-line list. That would allow float ordering to be violated. DO allow a float to be placed even if there is already nonempty content on the line, as long as there's enough space left on the line for the float.

-- In nsLineLayout::UpdateBand, the root span is moved to account for the new float. We need to update mX more carefully now that it might have advanced from the left edge. I've also moved UpdateFrames in here and made it work properly.

-- A couple of reftests needed to be updated to match this fix. 395331-1.xml assumed whitespace trimming after nonempty but zero-width content. 428423-1-ref.html assumed that the orange float would be placed on the next line.

-- Various reftests for this bug. 50360-1*.html test this bug under various combinations of directionality. 50630-3*.html tests that after moving a wide float to the next line, narrow floats that would fit on the current line are still pushed to the next line. 50630-4*.html tests that we don't place a float on a line where inline content has already taken up the necessary space. 50630-5*.html tests the quirk that floated tables should use an available width that takes into account floats impacting the line.

I could possibly break out the change to the LineIsEmpty name and behaviour into a separate bug, if you think that's worth doing. At the beginning it seemed necessary to fix this bug but now I don't think it is.
Comment 45 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-06-08 22:03:16 PDT
I filed bug 437920 on comment #43.
Comment 46 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-06-08 22:06:14 PDT
I skimmed the whole patch (except the tests):  it all makes sense to me except for the UpdateBand changes (which I think could use more comments as to what the parameters are, and perhaps more comments in general) and the changes to when we set LL_LINEISEMPTY (which I need to look into more closely, and compare with the spec).
Comment 47 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-06-09 02:41:58 PDT
Created attachment 324257 [details] [diff] [review]
fix v2

Same fix, but with the CanPlaceFloatNow/IsLineEmpty changes removed and UpdateBand reworked with more comments to hopefully be clearer.

I removed the leading whitespace test since it's no longer fixed by the patch, but I've left in the fix to the XUL box test that assumes non-leading whitespace will be trimmed, since it just shouldn't do that.
Comment 48 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-06-09 10:37:30 PDT
Comment on attachment 324257 [details] [diff] [review]
fix v2

Hrm.  I actually preferred the renaming of CanPlaceFloatNow -> IsLineEmpty.

Are you going to file the substantive changes that you'd made to when that was set as a different bug?  (What were they fixing?)

>-  if (NS_FRAME_IS_NOT_COMPLETE(aReflowStatus) && (NS_UNCONSTRAINEDSIZE == availHeight))
>+  if (NS_FRAME_IS_NOT_COMPLETE(aReflowStatus) &&
>+      (NS_UNCONSTRAINEDSIZE == availSpace.height))

If you're reformatting this, may as well drop the extra () around the second half.

> nsLineLayout::UpdateBand(nscoord aX, nscoord aY,

up to here, again...
Comment 49 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-06-09 11:09:03 PDT
(In reply to comment #48)
> (From update of attachment 324257 [details] [diff] [review])
> Hrm.  I actually preferred the renaming of CanPlaceFloatNow -> IsLineEmpty.

OK, but I don't think we should rename it unless we also change the behaviour so the name is not misleading :-).

> Are you going to file the substantive changes that you'd made to when that was
> set as a different bug?  (What were they fixing?)

Yeah I will. They don't have that much effect actually. In most places where we call CanPlaceFloatNow we're just trying to guarantee that we'll make progress by not putting a line-break at the start of the line before any content. The only bug I know for sure it fixes is the bug that was 50630-2.html in the first patch --- whitespace after zero-width but non-empty content at the start of a line is currently trimmed, but it shouldn't not be.
Comment 50 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-06-09 11:34:24 PDT
Comment on attachment 324257 [details] [diff] [review]
fix v2

It might be nice to make UpdateBand take an nsRect parameter instead of
four nscoords; that would let the parameter be named aAvailRect to show
that it's the available space (I was confused earlier when skimming
because I expected it to be the positions of the float).  (A followup
patch for this is fine too; it would also be good to get your other
rename landed.)


+    if (psd == mRootSpan) {
+      // Our mLeftEdge moved so we can't just change our right edge by deltaWidth
+      psd->mRightEdge = aX + aWidth;
+    } else {

Can you get rid of this if you move the rightEdge by deltaX at the same
time you move the left edge?

+    NS_ASSERTION(psd->mRightEdge >= psd->mLeftEdge,
+                 "We placed a float where there was no room!");

Are you sure you can't hit this with negative margins?

-           psd, psd->mRightEdge - deltaWidth, psd->mRightEdge);
+           psd, psd->mRightEdge - deltaRightEdge, psd->mRightEdge);

I don't think you want this change anymore.

I'd have written both of the while loops in UpdateBand as for loops, but
that's mostly personal preference; feel free to change if you want,
though.


Is the 395331-1.xml reftest change really needed?  Why?  It seems like
it ought to work as-is.

In 50630-3, could you specify a font-size, since the test sort of relies
on the text being reasonably small?

It might be worth having a second reference to 50430-4 (in addition to
the one you already have) that uses no floating at all.
Comment 51 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-06-09 14:38:33 PDT
(In reply to comment #50)
> (From update of attachment 324257 [details] [diff] [review])
> It might be nice to make UpdateBand take an nsRect parameter instead of
> four nscoords; that would let the parameter be named aAvailRect to show
> that it's the available space (I was confused earlier when skimming
> because I expected it to be the positions of the float).  (A followup
> patch for this is fine too; it would also be good to get your other
> rename landed.)

OK, I'll change that parameter in this patch.

> +    if (psd == mRootSpan) {
> +      // Our mLeftEdge moved so we can't just change our right edge by
> deltaWidth
> +      psd->mRightEdge = aX + aWidth;
> +    } else {
> 
> Can you get rid of this if you move the rightEdge by deltaX at the same
> time you move the left edge?

Yes, I'll do that.

> +    NS_ASSERTION(psd->mRightEdge >= psd->mLeftEdge,
> +                 "We placed a float where there was no room!");
> 
> Are you sure you can't hit this with negative margins?

We don't hit it in reftests. This should actually assert psd->mRightEdge >= psd->mX, and be guaranteed by the check in AddFloat that the width of the float is <= the available width (computed as psd->mRightEdge - psd->mX). Plus I'm assuming that the space manager can't decrease the available-space width by more than the width of the float. Did you have a specific example in mind?

> I'd have written both of the while loops in UpdateBand as for loops, but
> that's mostly personal preference; feel free to change if you want,
> though.

I should do that.

> Is the 395331-1.xml reftest change really needed?  Why?  It seems like
> it ought to work as-is.

Flexboxes are never treated as empty content, so the whitespace after the box is not trimmed even after the flexbox's children are removed.

> In 50630-3, could you specify a font-size, since the test sort of relies
> on the text being reasonably small?

OK.

> It might be worth having a second reference to 50430-4 (in addition to
> the one you already have) that uses no floating at all.

OK.
Comment 52 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-06-09 16:50:15 PDT
Created attachment 324369 [details] [diff] [review]
fix v3

Changes as requested. You probably should take a look though.
Comment 53 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-06-09 17:09:02 PDT
Seems ok.

I suppose with the line breaking stuff I wasn't actually sure what our behavior is when negative margins put us in situations where we can "cross the available width" at multiple points... but we're probably best off asserting that we always take the break preceding the first of those points, since that's the only sane thing for incremental layout.  So I guess the assertion is fine.
Comment 54 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-06-10 16:54:12 PDT
checked in.
Comment 55 old9 2008-06-10 18:29:25 PDT
so will this patch be applied to fx3 final?
Comment 56 Leon Sorokin 2008-06-10 19:26:01 PDT
great to see this finally get resolved, i'm also interested to know if this will make it into the final v3...or a subsequent update at least.
Comment 57 Ryan VanderMeulen [:RyanVM] 2008-06-10 19:29:06 PDT
It won't be in the final Firefox 3.0 release as that's in the late RC stages at this point. Going by the flags on this bug, no decision has been made about whether it will first appear in a Firefox 3.0.x, 3.x, or a later release.
Comment 58 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-06-10 19:33:30 PDT
We would not want to take this in Firefox 3.0.x. We will take this in Firefox 3.x since it's already on the trunk from which Firefox 3.x will be branched.
Comment 59 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-06-12 20:52:10 PDT
I filed bug 439004 with a patch to address the CanPlaceFloatNow->LineIsEmpty issue.
Comment 60 Alan Gresley 2008-06-21 16:50:08 PDT
Created attachment 326136 [details]
Floats should appear beside inline content and not below it

This bug with inline content seems to still be present in Gecko 1.9.1 alpre in this test case (see attachment).

Appears correct in Opera and maybe IE8b1. Bug is present in WebKit.
Comment 61 Alan Gresley 2008-06-21 16:55:42 PDT
Comment on attachment 326136 [details]
Floats should appear beside inline content and not below it

><?xml version="1.0" encoding="UTF-8"?>
><!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
><html lang="en" xmlns="http://www.w3.org/1999/xhtml" xml:lang="en-gb">
>
><head>
><title>Floats with width auto</title>
>
><style type="text/css">
>
>body {border:2px solid blue;}
>
>.floatleft {float: left; background: lime;}
>.floatright {float:right; background: yellow;}
>
>.floatleft1 {float: left; background: red;}
>.floatright2 {float: right; background: orange;}
>
>.clear {clear:both; border:2px solid silver;}
>
></style>
>
>
></head>
>
><body>
>
>	<div class="floatleft">
>		(inline content)
>		<div class="floatleft1">Float Left (should appear beside inline content)</div>
>	</div>
>
>	<div class="clear">Clear</div>
>
>	<div class="floatleft">
>		(inline content)
>		<div class="floatright2">Float Right (should appear beside inline content)</div>
>	</div>
>
>	<div class="clear">Clear</div>
>
>	<div class="floatright">
>		(inline content)
>		<div class="floatleft1">Float Left (should appear beside inline content)</div>
>	</div>
>
>	<div class="clear">Clear</div>
>
>	<div class="floatright">
>		(inline content)
>		<div class="floatright2">Float Right (should appear beside inline content)</div>
>	</div>
>
>	<div class="clear">Clear</div>
>
>
>	<p>Floats should only appear below inline content when there is not enought room within the containing block. In this test case it is the <code>body</code> element with a blue border.</p>
>
>
></body>
></html>
Comment 62 Bruno Fassino 2008-06-22 00:19:46 PDT
Created attachment 326158 [details]
should whitespace before the float count?

Alan's test case depends on the presence of white space before the float: if there is no white space between the previous word and the float, then it goes up.

And this behavior occurs even if there is no auto width involved. In the case I'm attaching there is something like:

<div style="font-family: Ahem; width: 10em">123456<div style="float: left; width: 4em">F</div></div>

and this fits exactly in a single line box. But as soon as there is white space before the float this goes down.
Comment 63 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-06-22 16:21:26 PDT
Alan, I think that's a different bug where we're not computing the preferred width of the outer float correctly. Please file it as a new bug.

Bruno, I don't think your testcase shows a bug. Your second box contains 11-em worth of content which just isn't going to fit in a 10-em box.
Comment 64 Bruno Fassino 2008-06-22 22:18:42 PDT
(In reply to comment #63)

> Alan, I think that's a different bug where we're not computing the 
> preferred width of the outer float correctly.

Roc, the width of the outer float looks correctly computed. But then the inner float drops _only if_ it is preceded by white space, as if  when placed it did not fit in the (apparently correct) width.
Unless you say that white space before the float (after the inline content) must be rendered, and so a bigger width for the outer float chosen (the one computed now doesn't take into account that white space.) 

In my case there are 11 ems if the white space is counted, and this again looks strange. If the float would not be there, surely that space would be ignored (trailing white space in a block.)
Comment 65 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-06-22 22:34:32 PDT
(In reply to comment #64)
> Unless you say that white space before the float (after the inline content)
> must be rendered, and so a bigger width for the outer float chosen (the one
> computed now doesn't take into account that white space.) 

Actually that whitespace doesn't need to be rendered so Alan's bug is a real bug.

> In my case there are 11 ems if the white space is counted, and this again looks
> strange. If the float would not be there, surely that space would be ignored
> (trailing white space in a block.)

Sorry, you are correct. But please file this bug (it's the same bug) as a new bug.
Comment 66 Alan Gresley 2008-06-23 01:49:16 PDT
(In reply to comment #65)

> Actually that whitespace doesn't need to be rendered so Alan's bug is a real
> bug.


I don't see how the can be considered a new bug, since the initial test cases didn't quite demonstrate the full problem. The difference in mine and Bruno's similar test case is that this inline content and float interaction is occurring within a floated container. Once this container is unfloated the bug is gone. In the specs 9.5 Floats is.

http://www.w3.org/TR/CSS21/visuren.html#floats

"A floated box is shifted to the left or right until its outer edge touches the containing block edge or the outer edge of another float. If there's a line box, the top of the floated box is aligned with the top of the current line box."

and point 8 in 9.5.1 Positioning the float: the 'float' property

http://www.w3.org/TR/CSS21/visuren.html#propdef-float

"A floating box must be placed as high as possible."

Why should there be difference if the containing block is a static inflow element or floated element?




Comment 67 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-06-23 02:06:30 PDT
I filed bug 441259 on that issue. It seems pretty easy to fix.

It should be a new bug because the testcases originall in this bug were fixed by the patch in this bug. If we reopened bugs every time a special case or edge case was found that the original fix didn't cover, we'd be opening and closing bugs all the time and it would create endless confusion.
Comment 68 Alan Gresley 2008-06-23 02:26:47 PDT
(In reply to comment #67)
> I filed bug 441259 on that issue. It seems pretty easy to fix.
> 
> It should be a new bug because the testcases originall in this bug were fixed
> by the patch in this bug. If we reopened bugs every time a special case or edge
> case was found that the original fix didn't cover, we'd be opening and closing
> bugs all the time and it would create endless confusion.


OK, It's a new bug but the problem I see with this is that this just adds another bug to the pool of bugs. How can an author search a database with 441259+ bugs. I believe that adding to this pool also make it confusing when searching?

Comment 69 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-06-23 04:14:49 PDT
It's easier this way. Trust us, we've been doing this for a while.
Comment 70 Gérard Talbot 2008-06-23 09:34:32 PDT
> How can an author search a database with
> 441259+ bugs.

By choosing the best (words) summary possible for each bug (this is best and very important; "A good summary should quickly and uniquely identify a bug report."), by adding proper keywords, by choosing proper component, etc.., you help others to be able to search and find such bug.

Bug writing guidelines
http://developer.mozilla.org/en/docs/Bug_writing_guidelines#Reporting_a_New_Bug

Bug Triagers' Guide
http://www.mozilla.org/quality/help/unconfirmed.html

A bug report that does not meet several criteria should NOT be confirmed.

Other public bug databases (KDE, webkit) have hundreds of thousands of bug entries and they do pretty much the same (guidelines, directives, recommendations, required criteria to fulfill).
Comment 71 Henrik Skupin (:whimboo) 2008-07-10 14:58:05 PDT
Having a look at the given URL (http://dbaron.org/css/test/sec414) the following builds places the floats at the correct position => verified:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a1pre) Gecko/2008070902 Minefield/3.1a1pre ID:2008070902

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a1pre) Gecko/2008070903 Minefield/3.1a1pre ID:2008070903
Comment 72 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-10-22 22:41:40 PDT
*** Bug 461279 has been marked as a duplicate of this bug. ***
Comment 73 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-01-06 14:43:03 PST
*** Bug 472381 has been marked as a duplicate of this bug. ***
Comment 74 philippe (part-time) 2009-05-11 01:22:14 PDT
*** Bug 492314 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.