441259 - Placement of floats on a line with content should take account of trimmable width

Status: VERIFIED FIXED

(Core :: Layout: Floats, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: testcase #2

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.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 5

[Alan Gresley]

(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.

Comment 6

[Alan Gresley]

Attachment: Same as textcase #2 but with higher inline-block

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.

Comment 7

[Gérard Talbot]

Attachment: Screenshot of attachment 326136 from bug 50630 in IE 8 b1

> 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

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

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

Comment 10

[Alan Gresley]

(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).

Comment 11

[Alan Gresley]

Attachment: testcase #3

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

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 326383 [details] [diff] [review]
fix v2

OK, r+sr=dbaron.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed 443165d325aa.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Probably

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

The assertions weren't there still few days ago.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Never mind then

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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?

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This fixed a 3.1 regression.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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