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

RESOLVED FIXED in Firefox 42

Status

()

Core
Layout: Block and Inline
RESOLVED FIXED
9 years ago
2 years ago

People

(Reporter: Greg Hauptmann, Assigned: dbaron)

Tracking

(Depends on: 2 bugs, 4 keywords)

Trunk
mozilla42
css2, dev-doc-complete, site-compat, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(8 attachments)

(Reporter)

Description

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

Comment 1

9 years ago
Created attachment 335123 [details]
html page with css that highlights problem
Component: General → Style System (CSS)
Product: Firefox → Core
QA Contact: general → style-system

Comment 2

9 years ago
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.
(Assignee)

Updated

9 years ago
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
(Assignee)

Updated

9 years ago
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)
(Assignee)

Comment 4

9 years ago
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.

Comment 6

8 years ago
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.

Updated

8 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
Version: unspecified → Trunk

Updated

8 years ago
Duplicate of this bug: 483551

Updated

8 years ago
Duplicate of this bug: 349580

Updated

7 years ago
Duplicate of this bug: 537808

Comment 10

7 years ago
Created attachment 419993 [details]
Shows bug, proof, and hack around it (using padding)
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

7 years ago
Created attachment 419994 [details]
Shows bug - minimal test case

Comment 13

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

Updated

7 years ago
Duplicate of this bug: 542972
(Assignee)

Updated

7 years ago
Keywords: css2
(Assignee)

Updated

7 years ago
Duplicate of this bug: 618696

Updated

6 years ago
Duplicate of this bug: 641779

Updated

6 years ago
Duplicate of this bug: 604555

Updated

6 years ago
Depends on: 50959

Updated

6 years ago
Duplicate of this bug: 656894

Updated

6 years ago
Duplicate of this bug: 658394

Updated

6 years ago
Duplicate of this bug: 685671

Updated

6 years ago
Duplicate of this bug: 706063

Updated

5 years ago
Duplicate of this bug: 717880

Updated

5 years ago
Duplicate of this bug: 734279

Comment 24

5 years ago
For god's sake, almost four years, bug still there WTF

Updated

5 years ago
Duplicate of this bug: 787926

Updated

5 years ago
Duplicate of this bug: 788566

Comment 27

5 years ago
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>
Blocks: 837417
Duplicate of this bug: 861014

Comment 29

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

4 years ago
Five years ago, a bug was born. The proud Phoenix rests on his laurels, and Mammon starts taking advantage again.....

Comment 31

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

Updated

4 years ago
Duplicate of this bug: 837417

Updated

4 years ago
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
Duplicate of this bug: 835162
See Also: → bug 1012634
Attachment #673520 - Attachment mime type: text/plain → text/html
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1190783
(Assignee)

Updated

2 years ago
Assignee: nobody → dbaron
(Assignee)

Comment 36

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2f0bdd94806
(Assignee)

Comment 37

2 years ago
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).
Attachment #8644054 - Flags: review?(roc)
(Assignee)

Comment 38

2 years ago
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.).
Attachment #8644055 - Flags: review?(roc)
Attachment #8644054 - Flags: review?(roc) → review+
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!
Attachment #8644055 - Flags: review?(roc) → review+
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 41

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c47f6709cf5
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e5e1a1f4f20
https://hg.mozilla.org/mozilla-central/rev/8c47f6709cf5
https://hg.mozilla.org/mozilla-central/rev/9e5e1a1f4f20
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1012634
(Assignee)

Updated

2 years ago
No longer blocks: 918244
Duplicate of this bug: 918244
Added the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/css-float-bugs-have-been-fixed/
Keywords: dev-doc-needed, site-compat
Added a brief note here: https://developer.mozilla.org/en-US/Firefox/Releases/42#CSS
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.