See https://bugzilla.mozilla.org/show_bug.cgi?id=50630#c60 and following. The problem is that when we decide whether a float fits on the line, we're looking at the current line width including the width of trailing whitespace that would be trimmed if it was the end of the line. We should exclude that width.

Comment on attachment 326270 [details] [diff] [review] fix This isn't quite what we want. The problem is, there might not be a break opportunity at the point where we're going to place the float. We really need to defer this test until we are at a break opportunity. But that's a harder change.

This problem goes beyond trimmable whitespace. Here's an example where we're placing the float too early; it looks like there's enough space when we encounter the float, but we can't break there so we run out of space later.

Actually delaying float placement until the next line break opportunity would be pretty hard since our inline layout interfaces do not support doing arbitrary things at the "next line break opportunity". I think if we're going to make this work without huge restructuring, we'll have to leverage multi-pass line layout. That probably means placing the float eagerly using a patch like the patch here, then detecting that the line overflowed and doing another pass where we don't place the float. I need to figure out exactly how that would work. This area is tricky since Webkit and Opera both have major bugs of their own. Webkit "solves" the problem by placing the float and then if there isn't enough room for the inline content, moving it all below the float (definitely wrong). Opera seems to allow line breaks at a float placeholder even in a nowrap context, which makes this bug go away completely, but I think is also clearly wrong.

(In reply to comment #4) [..] > Opera seems to allow line breaks at a float placeholder even in a nowrap > context, which makes this bug go away completely, but I think is also clearly > wrong. IE8b1 renders my original test case the same as Opera. I can't currently test with IE8b1. I will add a amended test case of your which I see somehow relates to this bug. https://bugzilla.mozilla.org/show_bug.cgi?id=25888 For this bug both Opera and IE8b1 does not show the overlap.

Gecko is calculating if there is space from the base of the first line box with foo and not the top of the line box.

> original test case (...). I can't currently test with IE8b1. Screenshot of attachment 326136 [details] (Alan Gresley) from bug 50630 (float should be as high as previous line box) in Internet Explorer 8 beta 1 Screenshot is 696px wide by 406px high

Bug 25888 is a different bug. I think the best thing to do here is to restrict the fix for bug 50630 to cases where we have a break opportunity at the float. That's very simple to implement and it means that in nowrap contexts (pretty rare) people will just get the old behaviour of the float moving to the next line --- not correct, but like Firefox 3 so shouldn't break things. Doing a complete fix for this is likely to require massive changes and now is not a good time for such changes, nor is it high enough priority to justify the work.

Same as before, but with the nowrap restriction as well.

(In reply to comment #8) > Bug 25888 is a different bug. I agree it's a different bug, I only said that it is related. > I think the best thing to do here is to restrict the fix for bug 50630 to cases > where we have a break opportunity at the float. That's very simple to implement > and it means that in nowrap contexts (pretty rare) people will just get the old > behaviour of the float moving to the next line --- not correct, but like > Firefox 3 so shouldn't break things. > > Doing a complete fix for this is likely to require massive changes and now is > not a good time for such changes, nor is it high enough priority to justify the > work. I agree. One fix at a time. I don't see the problem here is just related to these simple float and inline content interactions with all these test cases. I see this has a wider scope with how Gecko and similar WebKit handles the inline formatting context. I have created a new attachment showing wildly diverging behavior with Gecko, Safari and Opera. BTW, Does IE8b1 follow Opera behavior with this test case or does it show it own variant behavior? The first and third test have whitespace. The second and fourth test have no whitespace. Instead of contemplating fixing something that involves a major rewrite, I suggest firstly that we work out what is supposed to happen (via the CSS WG perhaps).

Test Case showing wildly diverging behavior between Gecko, WebKit and Opera.

So, it seems like having a break opportunity exactly at the float is not equivalent to psd->mNoWrap. I can think of cases where either is true and the other not: wo<float/>rd word <span style="white-space:nowrap"><float/></span> word This case also doesn't seem all that different from the case where we redo reflowing a line because we placed stuff after the last valid break opportunity. Just like we save the break opportunity for that case, could we also save a float-not-to-place?

(In reply to comment #12) > So, it seems like having a break opportunity exactly at the float is not > equivalent to psd->mNoWrap. I can think of cases where either is true and the > other not: > > wo<float/>rd Although the spec says not to allow a break here, we actually do (and so do Webkit and Opera). > word <span style="white-space:nowrap"><float/></span> word True. However, all we need for this patch to "work" is for there to be a break opportunity whenever !psd->mNoWrap (the converse is not required) and I think that holds. > This case also doesn't seem all that different from the case where we redo > reflowing a line because we placed stuff after the last valid break > opportunity. Just like we save the break opportunity for that case, could we > also save a float-not-to-place? I considered that, but I think it requires keeping a list of floats placed since the last registered break opportunity. Then if we overflow the line we have to go backwards through that list to see if pushing some of those floats to the next line makes enough room for the last word. Then we have to record the first float to push, and redo the line reflow. Maybe that's not as hard as I thought in comment #8, but I'm concerned that the line redo logic may get overcomplicated. It still sounds like overkill compared to this patch, which should work very well in practice. But I'll give it a go if you think it's worth it.

Comment on attachment 326383 [details] [diff] [review] fix v2 OK, r+sr=dbaron.

Pushed 443165d325aa.

Did this triple the number of assertions when running mochitest? I get 1164 times ###!!! ASSERTION: We placed a float where there was no room!: 'psd->mX - mTrimmableWidth <= psd->mRightEdge', file /mozilla/layout/generic/nsLineLayout.cpp, line 345

Probably

Argh, did I comment some wrong bug. This was commited long ago

The assertions weren't there still few days ago.

Never mind then

Roc, I tried to verify the fix here but I'm not able to determine what has been changed. When I compare the rendering in FF3.1 and FF3.0.6 both are the same. But this bug doesn't list any check-in on 1.9.0. So how can I see that it is fixed?

This fixed a 3.1 regression.

Ok. That would make sense. Marking this bug as a regression. Verified with: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090129 Shiretoko/3.1b3pre Ubiquity/0.1.5 ID:20090129021345 Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090126 Shiretoko/3.1b3pre ID:20090126033406