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
authorL. David Baron <dbaron@dbaron.org>
Mon, 17 Feb 2014 20:07:45 -0800
changeset 169573 83a2b19f7492fd07b0aa133bca65b0e91c54f49d
parent 169572 72c0c955cf53298175b1da4a1b399ceb7c4a598d
child 169574 beff43e0a138794ad463c6447dd327f240d5428c
push id270
push userpvanderbeken@mozilla.com
push dateThu, 06 Mar 2014 09:24:21 +0000
reviewersroc
bugs709014, 538935
milestone30.0a1
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
layout/reftests/bugs/538935-1-ref.html
layout/reftests/inline/inline-block-margin.html
layout/reftests/inline/inline-block-padding.html
layout/reftests/inline/inline-block-width.html
layout/reftests/inline/inline-block-zero.html
layout/reftests/inline/reftest.list
layout/reftests/inline/zero-inline-block-margin-left.html
layout/reftests/inline/zero-inline-block-margin-ref.html
layout/reftests/inline/zero-inline-block-margin-ref2.html
layout/reftests/inline/zero-inline-block-margin-right.html
layout/reftests/reftest.list
layout/reftests/text-overflow/marker-basic-ref.html
layout/style/test/test_value_computation.html
--- a/layout/generic/nsLineLayout.cpp
+++ b/layout/generic/nsLineLayout.cpp
@@ -1128,57 +1128,51 @@ nsLineLayout::CanPlaceFrame(PerFrameData
                             bool aCanRollBackBeforeFrame,
                             nsHTMLReflowMetrics& aMetrics,
                             nsReflowStatus& aStatus,
                             bool* aOptionalBreakAfterFits)
 {
   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);
+
+  // 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;
+  /*
+   * 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) {
     // When wrapping is off, everything fits.
     return true;
   }
 
-  bool ltr = NS_STYLE_DIRECTION_LTR == aFrameDirection;
   nscoord endMargin = ltr ? pfd->mMargin.right : pfd->mMargin.left;
 
 #ifdef NOISY_CAN_PLACE_FRAME
   if (nullptr != psd->mFrame) {
     nsFrame::ListTag(stdout, psd->mFrame->mFrame);
   }
   else {
     nsFrame::ListTag(stdout, mBlockReflowState->frame);
@@ -1285,37 +1279,34 @@ nsLineLayout::CanPlaceFrame(PerFrameData
 }
 
 /**
  * Place the frame. Update running counters.
  */
 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();
   else
     pfd->mAscent = aMetrics.TopAscent();
 
   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++;
   }
 }
 
 void
 nsLineLayout::AddBulletFrame(nsIFrame* aFrame,
                              const nsHTMLReflowMetrics& aMetrics)
 {
--- a/layout/reftests/bugs/538935-1-ref.html
+++ b/layout/reftests/bugs/538935-1-ref.html
@@ -1,16 +1,16 @@
 <!DOCTYPE HTML>
 <html>
 <body>
 <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>
 
 <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>
new file mode 100644
--- /dev/null
+++ b/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
new file mode 100644
--- /dev/null
+++ b/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
new file mode 100644
--- /dev/null
+++ b/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
new file mode 100644
--- /dev/null
+++ b/layout/reftests/inline/inline-block-zero.html
@@ -0,0 +1,3 @@
+<!DOCTYPE HTML>
+<title>margin-left on zero-sized inline-block</title>
+hello world
new file mode 100644
--- /dev/null
+++ b/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
new file mode 100644
--- /dev/null
+++ b/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
new file mode 100644
--- /dev/null
+++ b/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>
new file mode 100644
--- /dev/null
+++ b/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
new file mode 100644
--- /dev/null
+++ b/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
--- a/layout/reftests/reftest.list
+++ b/layout/reftests/reftest.list
@@ -187,16 +187,19 @@ skip-if(B2G) include image-element/refte
 skip-if(B2G) include image-rect/reftest.list
 
 # image-region/
 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
 
 # layers/
 include layers/reftest.list
 
 # line-breaking/
 skip-if(B2G) include line-breaking/reftest.list
--- a/layout/reftests/text-overflow/marker-basic-ref.html
+++ b/layout/reftests/text-overflow/marker-basic-ref.html
@@ -255,17 +255,17 @@ x1 m { position:absolute; right:0; font-
 <div id="test7c">
 <div class="s a"><div class="p ltr"><span class="c5 a">|<x style="display:inline-block;width:0.8em;text-align:right"><m style="font-size:16px;">&#x2026;</m></x><img class="ins1" src="../image/big.png"></span></div></div>
 </div>
 
 <div id="test7d"><div class="s a"><div class="p ltr r"><img class="a ins1" src="../image/big.png"><x class="a" style="display:inline-block;width:0.8em;text-align:left"><m style="font-size:16px;">&#x2026;</m></x><span class="c5 a" style="margin-right:0">&nbsp;</span></div></div></div>
 
 
 <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>
 <div id="test9c"><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="test9d"><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>
 
 <!-- no marker for overflow:auto that doesn't trigger scrollbar -->
 <div id="test10a"><div class="s a"><div class="p o ltr"><span style="margin-left:-5px" >||||||</span></div></div></div>
--- a/layout/style/test/test_value_computation.html
+++ b/layout/style/test/test_value_computation.html
@@ -8,17 +8,17 @@
   <script type="text/javascript" src="property_database.js"></script>
   <style type="text/css" id="stylesheet"></style>
   <style type="text/css">
   /* For 'width', 'height', etc., need a constant size container. */
   #display { width: 500px; height: 200px }
   </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);
 
   var load_count = 0;
   function load_done() {
     if (++load_count == 3)
       run_tests();
   }