Closed Bug 451791 Opened 16 years ago Closed 9 years ago

CSS margin-top collapses across cleared element inside previous sibling and out top of previous sibling (works in Safari, but Firefox has a bug)

Categories

(Core :: Layout: Block and Inline, defect)

Product:
Core
Component:
Layout: Block and Inline
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla42
Tracking Flags:
Tracking Status
firefox42 --- fixed

People

(Reporter: greg, Assigned: dbaron)

References

(Depends on 2 open bugs)

Details

(4 keywords)

Attachments

(8 files)

html page with css that highlights problem
109.33 KB, application/x-zip-compressed
Details
minimal test case
459 bytes, text/html
Details
variable testcase
806 bytes, text/html
Details
Shows bug, proof, and hack around it (using padding)
2.04 KB, text/html
Details
Shows bug - minimal test case
192 bytes, text/html
Details
Even more minimal test case
119 bytes, text/html
Details
MozReview Request: Bug 451791 patch 1 - Remove write-only nsHTMLReflowState::mFlags::mHasClearance. r?roc
40 bytes, text/x-review-board-request
roc
: review+
Details
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
roc
: review+
Details 
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)
Component: General → Style System (CSS)
Product: Firefox → Core
QA Contact: general → style-system
Attached file minimal test case — 
green floated div, followed by an empty (block) element with clear both.

red div has 10px margin-top.
Component: Style System (CSS) → Layout: Block and Inline
QA Contact: style-system → layout.block-and-inline
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).
OS: Mac OS X → All
Hardware: Macintosh → All
Summary: CSS margin is inherited by Parent (works in Safari, but Firefox has a bug) → CSS margin collapses across preceding cleared element and out top of parent (works in Safari, but Firefox has a bug)
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.
Summary: CSS margin collapses across preceding cleared element and out top of parent (works in Safari, but Firefox has a bug) → CSS margin-top collapses across cleared element inside previous sibling and out top of previous sibling (works in Safari, but Firefox has a bug)
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.
Attached file 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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
Version: unspecified → Trunk
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.
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>
Depends on: 50959
For god's sake, almost four years, bug still there WTF
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>
Blocks: 837417
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"....
Five years ago, a bug was born. The proud Phoenix rests on his laurels, and Mammon starts taking advantage again.....
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
No longer blocks: 837417
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.
Depends on: 477462
Blocks: 918244
See Also: → 1012634
Attachment #673520 - Attachment mime type: text/plain → text/html
Assignee: nobody → dbaron
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).
Attachment #8644054 - Flags: review?(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.).
Attachment #8644055 - Flags: review?(roc)
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 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!
https://hg.mozilla.org/mozilla-central/rev/8c47f6709cf5
https://hg.mozilla.org/mozilla-central/rev/9e5e1a1f4f20
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: