398144 - inline-block should always grow to contain floats

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Nikola Kocić]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007092705 Minefield/3.0a9pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007092705 Minefield/3.0a9pre ID:2007092705

Layout on http://www.anime-planet.com/ is messed up but it's displayed normally in IE, Opera and FF2.

Reproducible: Always

Comment 1

[u88484]

User agent sniffing?

Comment 2

[Nikola Kocić]

Tried with

Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 5.1)
and
Opera/9.20 (Windows NT 6.0; U; en)

no changes

Comment 3

[u88484]

Yeah but if you change the user agent from within Minefield to say Firefox does it work then? That is what I meant.

I tested and it does not look different if I change what I was talking about above.

Can you create a reduced testcase?

Comment 4

[Nikola Kocić]

Sorry, I meant I was using User Agent Switcher (from Minefield) with those strings and there was no change.

About testcase, sorry but I can't do anything about that, I just noticed an error and after consulting with site admin (he told me that it's almost certainly Minefield bug) I decided to fill bug here and to hope it gets fixed... I hope someone can help.

Comment 5

[u88484]

Can you find the regression range on which was the last build it looked correctly and the first one it didn't look correctly?

Comment 6

[Nikola Kocić]

Found it:
2007-01-27-04-works
2007-01-28-04-broken

yeah, it was broken back in 3.0a2 times...

Comment 7

[Steve England [:stevee]]

Heh. I've just found the same range.. mostly because it looks like the same thing I filed bug 392182 about. If the problem is due to inline-block, then this should probably be a dupe of 392182.

Comment 8

[Nikola Kocić]

Hmm... Bug 392182's testcase is same in current Minefield and Opera (but in Opera even when you zoom-in there's no difference) but it's correctly displayed in IE.
www.anime-planet.com is same in Opera and IE.

Not sure if it makes a difference though...

Comment 9

[Steve England [:stevee]]

Just to be clear with my range, works:
- Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a2pre) Gecko/2007012602 Minefield/3.0a2pre

Broken:
- Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a2pre) Gecko/2007012715 Minefield/3.0a2pre

Which is a day's difference to the range found in comment 6, but the same range as bug 392182

Comment 10

[Nikola Kocić]

May I ask where did you got that hourly builds from? I can only find nightlies (xxxx-xx-xx-04) on mozilla ftp server. And it's possible that regression was between 2007012704 and 2007012715, that way both your and mine ranges are correct...

Comment 11

[Steve England [:stevee]]

Yes, our regression ranges do actually overlap, which is good news! But in future, it would help if you could paste the whole user agent string ( from about: ) rather than just a date, because different OS builds will be at different hours.

Yes, the mozilla archive only keeps the official nightlies. The builds I used were from my own archive of hourly and nightly builds I've downloaded and saved.

Comment 12

[philippe (part-time)]

WEbKit and Safari 3 display that page correctly (not overlap of content).

In Minefield, the offending element (div id=homeintro) has indeed display:inline-block, and that is the cause of the problem. But I don't think Minefield is behaving correctly. Not sure what the problem is though.

Comment 13

[philippe (part-time)]

Attachment: minimised test case

The top block and bottom block are identical but for one thing: the inline-block has a border at the bottom: it displays correctly. In the top block, the inline-block collapses: it does not contain the floats correctly (it should).

Comment 14

[philippe (part-time)]

Additionally, the site in the URL field has this (a common technique to clear floats), but it has no effect on inline-block.
div#homeintro:after {
	display:block;
	clear:both;
	content:"";
	height:0;
	}
----------

This is a pretty annoying problem, as 'display:inline-block' is regularly used as bug fixer (it triggers 'hasLayout') in IE 6 and 7 Windows.

Comment 15

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

You mean 'clear' has no effect *after* an inline-block?  It's not supposed to have an effect on an inline-block [1], nor should it have any effect at all in this testcase.  However, the first half of the testcase is showing a bug where the inline-block ends up with the incorrect size.  It establishes a new block formatting context and therefore should grow to contain floats [2].  I'm not sure why it's doing so only some of the time here.  (I'm assuming the first and second halves of the simplified testcase differ only by a border, though I might have missed something.)

[1] http://www.w3.org/TR/CSS21/visuren.html#propdef-clear and
    http://www.w3.org/TR/CSS21/visuren.html#block-boxes

[2] http://www.w3.org/TR/CSS21/visuren.html#block-formatting and
    http://www.w3.org/TR/CSS21/visudet.html#root-height

Comment 16

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

Or did you intend comment 14 to be about a separate bug?

Comment 17

[philippe (part-time)]

(In reply to comment #15)

In the minimised test case, the top and bottom only differ by the border. Add a border and the inline-block contains the floats (as it should do anyway, as it establishes a block formatting context, per note [2] in your comment). And note, only a bottom-border is needed.

And no, I did not mean to imply that the clear property applied to an element that follows the inline-block should have an effect on floats inside an inline-block.
(in the test case, the clear:both applied to .u-block, I left it there as it was part of the original)

(In reply to comment #16)
> Or did you intend comment 14 to be about a separate bug?
> 
I noted the ::after 'hack' in the original stylesheet as it explains why the site displays correctly with Gecko 1.8.

But the ::after generated content applies _inside_ the inline-block, and should clear the floats _inside_ the inline-block. That might actually work correctly I think (but is only visible if there is real content: ::after {content:"xxxxxxxxxx"}).
I haven't played much with the combination of inline-block and generated content to know it there are any problems with it. I'll investigate, and file bugs if needed.

Comment 18

[philippe (part-time)]

Attachment: minimised test case

some clean up, some more colour to visualise what is going on.

Note that the inline-block actually grows to contain the float(s) (I added some padding to the left in the inline-block, in lime-green). The clearing doesn't work properly: the yellow block must be below the line-green block.

Comment 19

[j.j.]

Attachment: testcase2

Simpler testcase

Comment 20

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

What's happening here is that given
<div style="display:inline-block;"><div style="float:left;">left float</div></div>

The inline-block is deciding that it's "empty" --- because all its in-flow contents are empty --- and therefore its block container gives its line zero advance height.

I think inline-blocks should be treated as non-empty, always, like replaced elements.

Comment 21

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

Attachment: fix

Simple fix

Comment 22

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

It seems like anything that has a space manager (as inline blocks do) should also check its float list for emptyness before declaring that it's empty.  Though this fix may also be good (are there good ways to test this in other browsers? and does the spec define emptiness for any of the things we use IsEmpty for?), and may make that more general fix not matter.

How about some reftests?

Comment 23

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

(In reply to comment #22)
> It seems like anything that has a space manager (as inline blocks do) should
> also check its float list for emptyness before declaring that it's empty.

That's an option, I guess.
 
> Though this fix may also be good (are there good ways to test this in other
> browsers?

I'll have a go.

> and does the spec define emptiness for any of the things we use
> IsEmpty for?),

http://www.w3.org/TR/CSS21/box.html

Our IsEmpty controls whether to collapse top and bottom margins for an element, and the spec says to collapse top and bottom margins for an element when they're "adjoining", where:

An element's own margins are adjoining if the 'min-height' property is zero, and it has neither top or bottom borders nor top or bottom padding, and it has a 'height' of either 0 or 'auto', and it does not contain a line box, and all of its in-flow children's margins (if any) are adjoining.

It also says:
Margins of inline-block elements do not collapse (not even with their in-flow children).

*If* completely trimmed-away text does not cause creation of a line box, according to the spec, then this corresponds quite closely to IsEmpty(). In that case I think I can interpret the spec as implying inline-block elements (and any other margin-root!) are not empty... Maybe I should test for margin-root-ness instead of inline-block, making anything that's a margin-root non-empty?

> How about some reftests?

Easy, I'll do some when I check the fix in.

Comment 24

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

Attachment: emptiness test

This test tests emptiness of inline-block. When an inline-block is treated as empty, the margins of its parent DIV collapse together and the gap between the blue DIVs is 100px; this is what trunk does. When it's treated like a replaced element, the margins of the parent DIV do not collapse together and the gap is 200px. This is what we do with my patch, and it's what Safari 3 does.

Comment 25

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

Apparently IE7 also gives 200px.

I'll revise my patch to check for margin-root-ness. That seems like a very logical way to solve this.

Comment 26

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

Attachment: updated fix

Comment 27

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

Comment on attachment 285855 [details] [diff] [review]
updated fix

r+sr=dbaron

Comment 28

[Reed Loden [:reed]]

Checking in layout/generic/nsBlockFrame.cpp;
/cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.883; previous revision: 3.882
done

Comment 29

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

Attachment: reftest

I'll check this in when I get a chance

Comment 30

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

Checked in reftest.

Comment 31

[(no longer active)]

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008050810 Minefield/3.0pre

Verified.