Last Comment Bug 451791 - CSS margin-top collapses across cleared element inside previous sibling and out top of previous sibling (works in Safari, but Firefox has a bug)
: CSS margin-top collapses across cleared element inside previous sibling and o...
Status: RESOLVED FIXED
: css2, dev-doc-complete, site-compat, testcase
Product: Core
Classification: Components
Component: Layout: Block and Inline (show other bugs)
: Trunk
: All All
: -- normal with 5 votes (vote)
: mozilla42
Assigned To: David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
:
Mentors:
: 349580 483551 537808 542972 604555 618696 641779 656894 658394 685671 706063 717880 734279 787926 788566 835162 837417 861014 918244 1012634 1190783 (view as bug list)
Depends on: 50959 477462
Blocks:
  Show dependency treegraph
 
Reported: 2008-08-22 16:16 PDT by Greg Hauptmann
Modified: 2015-09-29 01:00 PDT (History)
40 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
html page with css that highlights problem (109.33 KB, application/x-zip-compressed)
2008-08-22 16:18 PDT, Greg Hauptmann
no flags Details
minimal test case (459 bytes, text/html)
2008-08-22 20:59 PDT, philippe (part-time)
no flags Details
variable testcase (806 bytes, text/html)
2009-02-21 06:05 PST, Daniel.S
no flags Details
Shows bug, proof, and hack around it (using padding) (2.04 KB, text/html)
2010-01-04 16:30 PST, Brandon Frohs
no flags Details
Shows bug - minimal test case (192 bytes, text/html)
2010-01-04 16:33 PST, Brandon Frohs
no flags Details
Even more minimal test case (119 bytes, text/html)
2012-10-20 00:12 PDT, Justin Watt
no flags Details
MozReview Request: Bug 451791 patch 1 - Remove write-only nsHTMLReflowState::mFlags::mHasClearance. r?roc (40 bytes, text/x-review-board-request)
2015-08-05 17:21 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
roc: review+
Details | Review
MozReview Request: Bug 451791 patch 2 - Report block non-empty to its parent block during margin collapsing if we encounter clearance. r?roc (40 bytes, text/x-review-board-request)
2015-08-05 17:21 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
roc: review+
Details | Review

Description Greg Hauptmann 2008-08-22 16:16:23 PDT
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.1) Gecko/2008070206 Firefox/3.0.1
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.1) Gecko/2008070206 Firefox/3.0.1

I have a case where a margin CSS setting on a DIV is inherited by it's parent, and puts an offset at the top of the parent.  In the attached example page you will see the whole website is rendered 5px below the top where it shouldn't be because of this.  

The section of the CSS stylesheet in the example:
  .active-scaffold { margin: 5px 0; }

The section from the HTML source where the style should apply, but is also seemingly applied to the parent as well (firebug shows it as a 5px offset) is>
   <div id="transaction-active-scaffold" class="active-scaffold active-scaffold-transaction default-theme">

I will try to attach page source below if I can



Reproducible: Always

Steps to Reproduce:
1. Load sample page I provie
2.
3.
Actual Results:  
Look at page and notice 5px gap at top of page

Expected Results:  
No gap of 5px at top of page (as per how Safari renders)
Comment 1 Greg Hauptmann 2008-08-22 16:18:04 PDT
Created attachment 335123 [details]
html page with css that highlights problem
Comment 2 philippe (part-time) 2008-08-22 20:59:03 PDT
Created attachment 335149 [details]
minimal test case

green floated div, followed by an empty (block) element with clear both.

red div has 10px margin-top.
Comment 3 Steven Michaud [:smichaud] (Retired) 2008-09-05 14:52:55 PDT
Testing with Philippe's testcase (attachment 335149 [details]), this problem
happens in FF2 and FF3 on OS X, Windows and Linux.

It doesn't happen with Safari (on OS X), Opera (on OS X), IE 7 (on
Windows XP) or Konqueror (on Linux).
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-09-05 15:10:11 PDT
It's worth reading http://www.w3.org/TR/CSS21/box.html#collapsing-margins very carefully to see what it says should happen here.  What it says isn't always logical.
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-09-05 15:13:27 PDT
I'd prefer not to touch margin-collapsing this cycle. That stuff is fragile and we don't have good test coverage yet. I'd like to postpone more margin-collapse work until someone has time to block out some time, write a test suite and pound on the bugs.
Comment 6 Daniel.S 2009-02-21 06:05:11 PST
Created attachment 363474 [details]
variable testcase

This is a bug.

The DOM looks like

body
 div (A)
  div (B)
  div (C)
 div (D)

I think our code can't deal with this situation. It thinks the top margin of D should collapse with the body element. We are correct enough to put margin between C and D, though.

If body has a top border, or if A is removed, or D is moved inside A, or C got height, the bug does not appear.

If you look at the new testcase, you'll see that the top margin of D is always doubled.
Comment 7 philippe (part-time) 2009-03-16 16:17:42 PDT
*** Bug 483551 has been marked as a duplicate of this bug. ***
Comment 8 Daniel.S 2009-04-12 08:37:14 PDT
*** Bug 349580 has been marked as a duplicate of this bug. ***
Comment 9 Brandon Frohs 2010-01-04 16:25:23 PST
*** Bug 537808 has been marked as a duplicate of this bug. ***
Comment 10 Brandon Frohs 2010-01-04 16:30:44 PST
Created attachment 419993 [details]
Shows bug, proof, and hack around it (using padding)
Comment 11 :Aryeh Gregor (working until September 2) 2010-01-04 16:32:06 PST
More minimal test case:

data:text/html,<!DOCTYPE html>
<div>
    <div style="float:left">xxx</div>
    <div style="clear:both"></div>
</div>
<div style="margin-top:50px">yyy</div>

WebKit and Opera have no space above the xxx, Firefox has a large gap above it.
Comment 12 Brandon Frohs 2010-01-04 16:33:13 PST
Created attachment 419994 [details]
Shows bug - minimal test case
Comment 13 Brandon Frohs 2010-01-04 18:32:18 PST
Quick Hack - #wrapper{padding:0.001em;} where #wrapper contains the float, the clear, and the element with top margin. Since there is typically a wrapper around every element (body or html if nothing else will work), this can fix the problem without messing up the layout (0.001em won't even create a single pixel in most cases - possible exception is when zoomed in).

<div id="wrapper">
 <div>
  <div style="float:left">xxx</div>
  <div style="clear:both"></div>
 </div>
 <div style="margin-top:50px;">yyy</div>
</div>
Comment 14 Daniel.S 2010-01-29 09:13:07 PST
*** Bug 542972 has been marked as a duplicate of this bug. ***
Comment 15 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-12-12 09:42:33 PST
*** Bug 618696 has been marked as a duplicate of this bug. ***
Comment 16 philippe (part-time) 2011-03-15 16:25:09 PDT
*** Bug 641779 has been marked as a duplicate of this bug. ***
Comment 17 Daniel.S 2011-05-09 08:30:51 PDT
*** Bug 604555 has been marked as a duplicate of this bug. ***
Comment 18 Mats Palmgren (:mats) 2011-05-15 05:36:41 PDT
*** Bug 656894 has been marked as a duplicate of this bug. ***
Comment 19 philippe (part-time) 2011-05-19 19:22:25 PDT
*** Bug 658394 has been marked as a duplicate of this bug. ***
Comment 20 j.j. 2011-09-08 14:46:39 PDT
*** Bug 685671 has been marked as a duplicate of this bug. ***
Comment 21 philippe (part-time) 2011-11-30 00:41:35 PST
*** Bug 706063 has been marked as a duplicate of this bug. ***
Comment 22 philippe (part-time) 2012-01-13 17:16:30 PST
*** Bug 717880 has been marked as a duplicate of this bug. ***
Comment 23 philippe (part-time) 2012-03-08 23:19:10 PST
*** Bug 734279 has been marked as a duplicate of this bug. ***
Comment 24 matteo sisti sette 2012-08-10 13:31:31 PDT
For god's sake, almost four years, bug still there WTF
Comment 25 Alice0775 White 2012-09-03 06:51:10 PDT
*** Bug 787926 has been marked as a duplicate of this bug. ***
Comment 26 Mats Palmgren (:mats) 2012-09-05 18:26:37 PDT
*** Bug 788566 has been marked as a duplicate of this bug. ***
Comment 27 Justin Watt 2012-10-20 00:12:10 PDT
Created attachment 673520 [details]
Even more minimal test case

Attaching another, even more minimal testcase, similar to how I stumbled upon this bug in the first place. The easiest workaround I've found so far involves adding a non-breaking space inside the div, alongside the float, e.g.

<!DOCTYPE html>
<div style="margin-bottom:100px;">
    <div style="float:left">Why do I have a margin-top?</div>&nbsp;
</div>
Comment 28 Boris Zbarsky [:bz] 2013-04-11 19:16:16 PDT
*** Bug 861014 has been marked as a duplicate of this bug. ***
Comment 29 matteo sisti sette 2013-06-02 12:49:32 PDT
How many decades is this going to take to be fixed? This has been around for 5 years and this is the only browser that has it.

Every time I open Firefox (which is every once in a long while, when I really have to) and see page layouts destroyed by this bug, I'm like: "OMFG it can't be _still_ there"....
Comment 30 Arkana 2013-07-18 03:44:04 PDT
Five years ago, a bug was born. The proud Phoenix rests on his laurels, and Mammon starts taking advantage again.....
Comment 31 David Frank 2013-08-10 10:22:40 PDT
Although this is a rare bug, it might worth mozilla the time to re-investigate firefox' margin collapsing algorithm: the number of duplicate issues suggest this bug has caused many headaches in practise.

Given margin collapse is one of the lesser-known concept of css, I would not be surprised if other developers find themselves confused by firefox' inconsistent behaviours; it's probably bad for firefox' public image withint the web community to keep a bug open for 5 years also (not that webkit hadn't done this)...

anyway, i have just made a simple example (with workarounds), hopefully it's helpful for further investigation.

https://gist.github.com/bitinn/6201106
Comment 32 Zack Weinberg (:zwol) 2013-08-10 11:26:28 PDT
*** Bug 837417 has been marked as a duplicate of this bug. ***
Comment 33 Zack Weinberg (:zwol) 2013-08-10 11:36:03 PDT
For the record, this bug has stayed open for so long not because we don't care, but because the margin-collapsing code is extremely fragile, and we don't want to screw anything else up in the process of fixing it.  There are lots of other closely-related issues, too (see bug 477462 and bug 50959).

We do not need further examples of the problem described by this bug; we know what the syndrome is.  The most helpful thing anyone can do at this point is to assist Daniel.S in writing a comprehensive margin-collapsing test suite, over in bug 477462.
Comment 34 Boris Zbarsky [:bz] 2013-11-14 06:46:44 PST
*** Bug 835162 has been marked as a duplicate of this bug. ***
Comment 35 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2015-08-05 10:19:47 PDT
*** Bug 1190783 has been marked as a duplicate of this bug. ***
Comment 36 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2015-08-05 15:24:54 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2f0bdd94806
Comment 37 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2015-08-05 17:21:06 PDT
Created attachment 8644054 [details]
MozReview Request: Bug 451791 patch 1 - Remove write-only nsHTMLReflowState::mFlags::mHasClearance.  r?roc

Bug 451791 patch 1 - Remove write-only nsHTMLReflowState::mFlags::mHasClearance.  r?roc

This was introduced in bug 209694 (gecko-dev 13a65028) but became unused
through bug 292295 (gecko-dev 4593df2a57) and bug 300030 (gecko-dev
31f18988).
Comment 38 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2015-08-05 17:21:08 PDT
Created attachment 8644055 [details]
MozReview Request: Bug 451791 patch 2 - Report block non-empty to its parent block during margin collapsing if we encounter clearance.  r?roc

Bug 451791 patch 2 - Report block non-empty to its parent block during margin collapsing if we encounter clearance.  r?roc

The goal of ComputeCollapsedBStartMargin is to collapse all of the
margins that collapse with a block's top margin.  It does this by
scanning forward through the child list until it finds something that
blocks collapsing; it descends into children through recursion.  When we
find a non-empty block or line, we stop collapsing and report to the
parent that the child is non-empty so that it stops collapsing as well.

This patch changes our behavior when we have clearance to do the same
thing that we do for non-empty lines or blocks (which makes both
occurrences of |goto done| be preceded by the same code).  Without the
patch we would fail to report being non-empty to the parent (and instead
report emptiness based on the IsEmpty() method).  This meant that,
without the patch, if a block has a child with clearance but also has
IsEmpty() true, we would stop scanning margins in that block after the
clearance, but start searching again for margins in the block's parent,
starting with the block's bottom margin.  This patch sets *aBlockIsEmpty
to true in that case so that we do not pick up again in the block's
parent (or, potentially, grandparent, etc.).
Comment 39 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2015-08-05 18:32:13 PDT
Comment on attachment 8644054 [details]
MozReview Request: Bug 451791 patch 1 - Remove write-only nsHTMLReflowState::mFlags::mHasClearance.  r?roc

https://reviewboard.mozilla.org/r/15203/#review13633

Ship It!
Comment 40 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2015-08-05 18:32:35 PDT
Comment on attachment 8644055 [details]
MozReview Request: Bug 451791 patch 2 - Report block non-empty to its parent block during margin collapsing if we encounter clearance.  r?roc

https://reviewboard.mozilla.org/r/15205/#review13635

Ship It!
Comment 43 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2015-08-06 22:43:57 PDT
*** Bug 1012634 has been marked as a duplicate of this bug. ***
Comment 44 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2015-08-09 00:08:36 PDT
*** Bug 918244 has been marked as a duplicate of this bug. ***
Comment 45 Kohei Yoshino [:kohei] 2015-09-23 05:32:31 PDT
Added the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/css-float-bugs-have-been-fixed/
Comment 46 Jean-Yves Perrier [:teoli] 2015-09-29 01:00:52 PDT
Added a brief note here: https://developer.mozilla.org/en-US/Firefox/Releases/42#CSS

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