Bug 709014 patch 1: Honor margin-left and margin-right on elements in inline layout that have 0 width and/or height (commonly, inline-blocks). r=roc

Prior to this patch, we failed to honor:
 * margin-left on elements in inline layout with 0 width and 0 height
 * margin-right on elements in inline layout with 0 width
I think that was because the code in CanPlaceFrame to discard both
margins when the width was 0 was running after the left-margin was
applied, unless the later code in PlaceFrame (checking both width 0 and
height 0) un-applied that left margin.

The assertion count change in test_value_computation.html is due to 2
additional "bad width" assertions (I presume from honoring large
margins that were previously ignored).

The change to 538935-1-ref.html is to match an improvement in rendering
of the margins in the test, where both sides of the margin are now
honored.

The change to layout/reftests/text-overflow/marker-basic-ref.html is to
keep the reference (which uses margins) rendering the same way following
the changes to margin handling.

The new behavior (in the reftests added in layout/reftests/inline/)
matches at least Chromium; I didn't check any other browsers.

layout/generic/nsLineLayout.cpp

@@ -1133,39 +1133,34 @@ nsLineLayout::CanPlaceFrame(PerFrameData* pfd,
   NS_PRECONDITION(pfd && pfd->mFrame, "bad args, null pointers for frame data");
   
   *aOptionalBreakAfterFits = true;
-  // Compute right margin to use
-  if (0 != pfd->mBounds.width) {
-    // XXXwaterson this is probably not exactly right; e.g., embeddings, etc.
-    bool ltr = (NS_STYLE_DIRECTION_LTR == aFrameDirection);
 
-    /*
-     * We want to only apply the end margin if we're the last continuation and
-     * either not in an {ib} split or the last inline in it.  In all other
-     * cases we want to zero it out.  That means zeroing it out if any of these
-     * conditions hold:
-     * 1) The frame is not complete (in this case it will get a next-in-flow)
-     * 2) The frame is complete but has a non-fluid continuation on its
-     *    continuation chain.  Note that if it has a fluid continuation, that
-     *    continuation will get destroyed later, so we don't want to drop the
-     *    end-margin in that case.
-     * 3) The frame is in an {ib} split and is not the last part.
-     *
-     * However, none of that applies if this is a letter frame (XXXbz why?)
-     */
-    if ((NS_FRAME_IS_NOT_COMPLETE(aStatus) ||
-         pfd->mFrame->LastInFlow()->GetNextContinuation() ||
-         pfd->mFrame->FrameIsNonLastInIBSplit())
-        && !pfd->GetFlag(PFD_ISLETTERFRAME)) {
-      if (ltr)
-        pfd->mMargin.right = 0;
-      else
-        pfd->mMargin.left = 0;
+  // XXXwaterson this is probably not exactly right; e.g., embeddings, etc.
+  bool ltr = NS_STYLE_DIRECTION_LTR == aFrameDirection;
+
+  /*
+   * We want to only apply the end margin if we're the last continuation and
+   * either not in an {ib} split or the last inline in it.  In all other
+   * cases we want to zero it out.  That means zeroing it out if any of these
+   * conditions hold:
+   * 1) The frame is not complete (in this case it will get a next-in-flow)
+   * 2) The frame is complete but has a non-fluid continuation on its
+   *    continuation chain.  Note that if it has a fluid continuation, that
+   *    continuation will get destroyed later, so we don't want to drop the
+   *    end-margin in that case.
+   * 3) The frame is in an {ib} split and is not the last part.
+   *
+   * However, none of that applies if this is a letter frame (XXXbz why?)
+   */
+  if ((NS_FRAME_IS_NOT_COMPLETE(aStatus) ||
+       pfd->mFrame->LastInFlow()->GetNextContinuation() ||
+       pfd->mFrame->FrameIsNonLastInIBSplit())
+      && !pfd->GetFlag(PFD_ISLETTERFRAME)) {
+    if (ltr) {
+      pfd->mMargin.right = 0;
+    } else {
+      pfd->mMargin.left = 0;
     }
   }
-  else {
-    // Don't apply margin to empty frames.
-    pfd->mMargin.left = pfd->mMargin.right = 0;
-  }
 
   PerSpanData* psd = mCurrentSpan;
   if (psd->mNoWrap) {
@@ -1173,7 +1168,6 @@ nsLineLayout::CanPlaceFrame(PerFrameData* pfd,
     return true;
   }
 
-  bool ltr = NS_STYLE_DIRECTION_LTR == aFrameDirection;
   nscoord endMargin = ltr ? pfd->mMargin.right : pfd->mMargin.left;
 
 #ifdef NOISY_CAN_PLACE_FRAME
@@ -1290,15 +1284,6 @@ nsLineLayout::CanPlaceFrame(PerFrameData* pfd,
 void
 nsLineLayout::PlaceFrame(PerFrameData* pfd, nsHTMLReflowMetrics& aMetrics)
 {
-  // If frame is zero width then do not apply its left and right margins.
-  PerSpanData* psd = mCurrentSpan;
-  bool emptyFrame = false;
-  if ((0 == pfd->mBounds.width) && (0 == pfd->mBounds.height)) {
-    pfd->mBounds.x = psd->mX;
-    pfd->mBounds.y = mTopEdge;
-    emptyFrame = true;
-  }
-
   // Record ascent and update max-ascent and max-descent values
   if (aMetrics.TopAscent() == nsHTMLReflowMetrics::ASK_FOR_BASELINE)
     pfd->mAscent = pfd->mFrame->GetBaseline();
@@ -1307,10 +1292,16 @@ nsLineLayout::PlaceFrame(PerFrameData* pfd, nsHTMLReflowMetrics& aMetrics)
 
   bool ltr = (NS_STYLE_DIRECTION_LTR == pfd->mFrame->StyleVisibility()->mDirection);
   // Advance to next X coordinate
-  psd->mX = pfd->mBounds.XMost() + (ltr ? pfd->mMargin.right : pfd->mMargin.left);
+  mCurrentSpan->mX = pfd->mBounds.XMost() +
+                     (ltr ? pfd->mMargin.right : pfd->mMargin.left);
 
-  // Count the number of non-empty frames on the line...
-  if (!emptyFrame) {
+  // Count the number of non-placeholder frames on the line...
+  if (pfd->mFrame->GetType() == nsGkAtoms::placeholderFrame) {
+    NS_ASSERTION(pfd->mBounds.width == 0 && pfd->mBounds.height == 0,
+                 "placeholders should have 0 width/height (checking "
+                 "placeholders were never counted by the old code in "
+                 "this function)");
+  } else {
     mTotalPlacedFrames++;
   }
 }

layout/reftests/bugs/538935-1-ref.html

@@ -5,12 +5,12 @@
 <div style="padding-left:2px;">Hello</div>
 <div>Hello</div>
 <div style="padding-left:2px;">Hello</div>
-<div style="padding-left:1px;">Hello</div>
+<div style="padding-left:2px;">Hello</div>
 
 <div>Hello</div>
 <div style="padding-left:2px;">Hello</div>
 <div>Hello</div>
 <div style="padding-left:2px;">Hello</div>
-<div style="padding-left:1px;">Hello</div>
+<div style="padding-left:2px;">Hello</div>
 </body>
 </html>

layout/reftests/inline/inline-block-margin.html

@@ -0,0 +1,6 @@
+<!DOCTYPE HTML>
+<title>margin-left on zero-sized inline-block</title>
+<style>
+span { display: inline-block; height: 0; padding-right: 100px }
+</style>
+<span>hello&nbsp;</span>world

layout/reftests/inline/inline-block-padding.html

@@ -0,0 +1,6 @@
+<!DOCTYPE HTML>
+<title>margin-left on zero-sized inline-block</title>
+<style>
+span { display: inline-block; height: 0; padding-right: 100px }
+</style>
+<span>hello&nbsp;</span>world

layout/reftests/inline/inline-block-width.html

@@ -0,0 +1,6 @@
+<!DOCTYPE HTML>
+<title>margin-left on zero-sized inline-block</title>
+<style>
+span { display: inline-block; height: 0; width: 100px }
+</style>
+hello <span></span>world

layout/reftests/inline/inline-block-zero.html

@@ -0,0 +1,3 @@
+<!DOCTYPE HTML>
+<title>margin-left on zero-sized inline-block</title>
+hello world

layout/reftests/inline/reftest.list

@@ -0,0 +1,7 @@
+== zero-inline-block-margin-left.html zero-inline-block-margin-ref.html
+== zero-inline-block-margin-right.html zero-inline-block-margin-ref.html
+== zero-inline-block-margin-ref.html zero-inline-block-margin-ref2.html
+== inline-block-width.html zero-inline-block-margin-ref.html
+== inline-block-padding.html inline-block-width.html
+== inline-block-margin.html inline-block-width.html
+!= inline-block-width.html inline-block-zero.html

layout/reftests/inline/zero-inline-block-margin-left.html

@@ -0,0 +1,6 @@
+<!DOCTYPE HTML>
+<title>margin-left on zero-sized inline-block</title>
+<style>
+span { display: inline-block; height: 0; width: 0; margin-left: 100px }
+</style>
+hello <span></span>world

layout/reftests/inline/zero-inline-block-margin-ref.html

@@ -0,0 +1,6 @@
+<!DOCTYPE HTML>
+<title>margin-left on zero-sized inline-block</title>
+<style>
+span { margin-left: 100px }
+</style>
+hello <span>world</span>

layout/reftests/inline/zero-inline-block-margin-ref2.html

@@ -0,0 +1,6 @@
+<!DOCTYPE HTML>
+<title>margin-left on zero-sized inline-block</title>
+<style>
+span { margin-right: 100px }
+</style>
+<span>hello</span> world

layout/reftests/inline/zero-inline-block-margin-right.html

@@ -0,0 +1,6 @@
+<!DOCTYPE HTML>
+<title>margin-left on zero-sized inline-block</title>
+<style>
+span { display: inline-block; height: 0; width: 0; margin-right: 100px }
+</style>
+hello <span></span>world

layout/reftests/reftest.list

@@ -192,6 +192,9 @@ skip-if(B2G) include image-region/reftest.list
 # indic shaping with harfbuzz
 skip-if(B2G) include indic-shaping/reftest.list
 
+# inline layout
+include inline/reftest.list
+
 # inline borders and padding
 skip-if(B2G) include inline-borderpadding/reftest.list
 

layout/reftests/text-overflow/marker-basic-ref.html

@@ -260,7 +260,7 @@ x1 m { position:absolute; right:0; font-size:16px; }
 
 
 <div id="test8a"><div class="s a"><div class="a p ltr"><span class="c5 a"></span><span class="e"></span><span style="margin-right:-0.5em">&#x2026;</span><span>&#x200c;</span></div></div></div>
-<div id="test8d"><div class="s a"><div class="a p rtl"><span class="c5 a"></span><span class="e"></span><span style="margin-right:-0.5em">&#x2026;</span><span>&#x200c;</span></div></div></div>
+<div id="test8d"><div class="s a"><div class="a p rtl"><span class="c5 a"></span><span class="e"></span><span style="margin-left:-0.5em">&#x2026;</span><span>&#x200c;</span></div></div></div>
 
 <div id="test9a"><div class="s a"><div class="p ltr"><span class="e"></span><i>&nbsp;&nbsp;&nbsp;&nbsp;</i><m>&#x2026;</m><span class="e a"></span></div></div></div>
 <div id="test9b"><div class="s a"><div class="p rtl"><span class="e"></span><i>&nbsp;&nbsp;&nbsp;&nbsp;</i><m>&#x2026;</m><span class="e a"></span></div></div></div>

layout/style/test/test_value_computation.html

@@ -13,7 +13,7 @@
   </style>
   <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
   <script type="text/javascript">
-  SimpleTest.expectAssertions(7);
+  SimpleTest.expectAssertions(9);
   SimpleTest.waitForExplicitFinish();
   SimpleTest.requestLongerTimeout(2);