inline-block should always grow to contain floats

RESOLVED FIXED in mozilla1.9beta2

Status

()

P2
major
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: nikolakocicbz, Assigned: roc)

Tracking

({regression, testcase})

Trunk
mozilla1.9beta2
x86
Windows XP
regression, testcase
Points:
---
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [dbaron-1.9:RwCo], URL)

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
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

12 years ago
User agent sniffing?
(Reporter)

Comment 2

12 years ago
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

12 years ago
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression

Updated

12 years ago
Version: unspecified → Trunk
(Reporter)

Comment 4

12 years ago
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

12 years ago
Can you find the regression range on which was the last build it looked correctly and the first one it didn't look correctly?
(Reporter)

Comment 6

12 years ago
Found it:
2007-01-27-04-works
2007-01-28-04-broken

yeah, it was broken back in 3.0a2 times...
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.
(Reporter)

Comment 8

12 years ago
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...
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
(Reporter)

Comment 10

12 years ago
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...
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.
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.
Created attachment 283121 [details]
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).

Updated

12 years ago
Blocks: 9458
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.
Severity: normal → major
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
Flags: blocking1.9?
Summary: Layout messed up at http://www.anime-planet.com/ → inline-block should always grow to contain floats
(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.

Created attachment 283133 [details]
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.
Attachment #283121 - Attachment is obsolete: true

Comment 19

12 years ago
Created attachment 283206 [details]
testcase2

Simpler testcase
Keywords: testcase
Assignee: nobody → roc
Flags: blocking1.9? → blocking1.9+
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.
Created attachment 285840 [details] [diff] [review]
fix

Simple fix
Attachment #285840 - Flags: superreview?(dbaron)
Attachment #285840 - Flags: review?(dbaron)
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?
(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.
Created attachment 285854 [details]
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.
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.
Created attachment 285855 [details] [diff] [review]
updated fix
Attachment #285840 - Attachment is obsolete: true
Attachment #285855 - Flags: superreview?(dbaron)
Attachment #285855 - Flags: review?(dbaron)
Attachment #285840 - Flags: superreview?(dbaron)
Attachment #285840 - Flags: review?(dbaron)
Whiteboard: [dbaron-1.9:RwCo] → [dbaron-1.9:RwCo][needs landing]
Priority: -- → P2
Checking in layout/generic/nsBlockFrame.cpp;
/cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.883; previous revision: 3.882
done
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [dbaron-1.9:RwCo][needs landing] → [dbaron-1.9:RwCo]
Target Milestone: --- → mozilla1.9 M10
Created attachment 287925 [details] [diff] [review]
reftest

I'll check this in when I get a chance
Checked in reftest.
Flags: in-testsuite+

Comment 31

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

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