Bug 1426760: Don't let placeholders affect line height. r?jfkthame draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 22 Dec 2017 17:28:24 +0100
changeset 714495 3774fc92e2ab88989e56a147da5dc7883f9019a9
parent 714494 261536977dda32f4621478c587eacc5d9ed01a44
child 744597 2aa5a977b5a167bc8554b9ad5172dc6dc5ca4c96
push id93930
push userbmo:emilio@crisal.io
push dateSat, 23 Dec 2017 18:17:17 +0000
reviewersjfkthame
bugs1426760
milestone59.0a1
Bug 1426760: Don't let placeholders affect line height. r?jfkthame MozReview-Commit-ID: IJHerDOKOkj
layout/generic/nsBlockFrame.cpp
layout/generic/nsLineLayout.cpp
layout/generic/nsLineLayout.h
testing/web-platform/meta/MANIFEST.json
testing/web-platform/tests/css/CSS2/linebox/line-height-oof-descendants-001-ref.html
testing/web-platform/tests/css/CSS2/linebox/line-height-oof-descendants-001.html
--- a/layout/generic/nsBlockFrame.cpp
+++ b/layout/generic/nsBlockFrame.cpp
@@ -4142,21 +4142,17 @@ nsBlockFrame::DoReflowInlineFrames(Block
  */
 void
 nsBlockFrame::ReflowInlineFrame(BlockReflowInput& aState,
                                 nsLineLayout& aLineLayout,
                                 LineIterator aLine,
                                 nsIFrame* aFrame,
                                 LineReflowStatus* aLineReflowStatus)
 {
-  if (!aFrame) { // XXX change to MOZ_ASSERT(aFrame)
-    NS_ERROR("why call me?");
-    return;
-  }
-
+  MOZ_ASSERT(aFrame);
   *aLineReflowStatus = LineReflowStatus::OK;
 
 #ifdef NOISY_FIRST_LETTER
   ListTag(stdout);
   printf(": reflowing ");
   nsFrame::ListTag(stdout, aFrame);
   printf(" reflowingFirstLetter=%s\n",
          aLineLayout.GetFirstLetterStyleOK() ? "on" : "off");
@@ -4164,17 +4160,17 @@ nsBlockFrame::ReflowInlineFrame(BlockRef
 
   if (aFrame->IsPlaceholderFrame()) {
     auto ph = static_cast<nsPlaceholderFrame*>(aFrame);
     ph->ForgetLineIsEmptySoFar();
   }
 
   // Reflow the inline frame
   nsReflowStatus frameReflowStatus;
-  bool           pushedFrame;
+  bool pushedFrame;
   aLineLayout.ReflowFrame(aFrame, frameReflowStatus, nullptr, pushedFrame);
 
   if (frameReflowStatus.NextInFlowNeedsReflow()) {
     aLineLayout.SetDirtyNextLine();
   }
 
 #ifdef REALLY_NOISY_REFLOW
   nsFrame::ListTag(stdout, aFrame);
--- a/layout/generic/nsLineLayout.cpp
+++ b/layout/generic/nsLineLayout.cpp
@@ -657,16 +657,17 @@ nsLineLayout::NewPerFrameData(nsIFrame* 
   pfd->mIsTextFrame = false;
   pfd->mIsNonEmptyTextFrame = false;
   pfd->mIsNonWhitespaceTextFrame = false;
   pfd->mIsLetterFrame = false;
   pfd->mRecomputeOverflow = false;
   pfd->mIsBullet = false;
   pfd->mSkipWhenTrimmingWhitespace = false;
   pfd->mIsEmpty = false;
+  pfd->mIsPlaceholder = false;
   pfd->mIsLinkedToBase = false;
 
   pfd->mWritingMode = aFrame->GetWritingMode();
   WritingMode lineWM = mRootSpan->mWritingMode;
   pfd->mBounds = LogicalRect(lineWM);
   pfd->mOverflowAreas.Clear();
   pfd->mMargin = LogicalMargin(lineWM);
   pfd->mBorderPadding = LogicalMargin(lineWM);
@@ -835,17 +836,17 @@ nsLineLayout::ReflowFrame(nsIFrame* aFra
   //
   // Capture this state *before* we reflow the frame in case it clears
   // the state out. We need to know how to treat the current frame
   // when breaking.
   bool notSafeToBreak = LineIsEmpty() && !mImpactedByFloats;
 
   // Figure out whether we're talking about a textframe here
   LayoutFrameType frameType = aFrame->Type();
-  bool isText = frameType == LayoutFrameType::Text;
+  const bool isText = frameType == LayoutFrameType::Text;
 
   // Inline-ish and text-ish things don't compute their width;
   // everything else does.  We need to give them an available width that
   // reflects the space left on the line.
   LAYOUT_WARN_IF_FALSE(psd->mIEnd != NS_UNCONSTRAINEDSIZE,
                       "have unconstrained width; this should only result from "
                       "very large sizes, not attempts at intrinsic width "
                       "calculation");
@@ -935,16 +936,17 @@ nsLineLayout::ReflowFrame(nsIFrame* aFra
   // content.
   bool placedFloat = false;
   bool isEmpty;
   if (frameType == LayoutFrameType::None) {
     isEmpty = pfd->mFrame->IsEmpty();
   } else {
     if (LayoutFrameType::Placeholder == frameType) {
       isEmpty = true;
+      pfd->mIsPlaceholder = true;
       pfd->mSkipWhenTrimmingWhitespace = true;
       nsIFrame* outOfFlowFrame = nsLayoutUtils::GetFloatFromPlaceholder(aFrame);
       if (outOfFlowFrame) {
         // Add mTrimmableISize to the available width since if the line ends
         // here, the width of the inline content will be reduced by
         // mTrimmableISize.
         nscoord availableISize = psd->mIEnd - (psd->mICoord - mTrimmableISize);
         if (psd->mNoWrap) {
@@ -2204,27 +2206,27 @@ nsLineLayout::VerticalAlignFrames(PerSpa
       //           and layout is using dynamic font heights (bug 20394)
       //        -- Note #1: With this code enabled and with the fact that we are not
       //           using Em[Ascent|Descent] as nsDimensions for text metrics in
       //           GFX mean that the discussion in bug 13072 cannot hold.
       //        -- Note #2: We still don't want empty-text frames to interfere.
       //           For example in quirks mode, avoiding empty text frames prevents
       //           "tall" lines around elements like <hr> since the rules of <hr>
       //           in quirks.css have pseudo text contents with LF in them.
-#if 0
-      if (!pfd->mIsTextFrame) {
-#else
-      // Only consider non empty text frames when line-height=normal
-      bool canUpdate = !pfd->mIsTextFrame;
-      if (!canUpdate && pfd->mIsNonWhitespaceTextFrame) {
-        canUpdate =
+      bool canUpdate;
+      if (pfd->mIsTextFrame) {
+        // Only consider text frames if they're not empty and
+        // line-height=normal.
+        canUpdate = pfd->mIsNonWhitespaceTextFrame &&
           frame->StyleText()->mLineHeight.GetUnit() == eStyleUnit_Normal;
+      } else {
+        canUpdate = !pfd->mIsPlaceholder;
       }
+
       if (canUpdate) {
-#endif
         nscoord blockStart, blockEnd;
         if (frameSpan) {
           // For spans that were are now placing, use their position
           // plus their already computed min-Y and max-Y values for
           // computing blockStart and blockEnd.
           blockStart = pfd->mBounds.BStart(lineWM) + frameSpan->mMinBCoord;
           blockEnd = pfd->mBounds.BStart(lineWM) + frameSpan->mMaxBCoord;
         }
--- a/layout/generic/nsLineLayout.h
+++ b/layout/generic/nsLineLayout.h
@@ -466,16 +466,17 @@ protected:
     bool mIsTextFrame : 1;
     bool mIsNonEmptyTextFrame : 1;
     bool mIsNonWhitespaceTextFrame : 1;
     bool mIsLetterFrame : 1;
     bool mRecomputeOverflow : 1;
     bool mIsBullet : 1;
     bool mSkipWhenTrimmingWhitespace : 1;
     bool mIsEmpty : 1;
+    bool mIsPlaceholder : 1;
     bool mIsLinkedToBase : 1;
 
     // Other state we use
     uint8_t mBlockDirAlign;
     mozilla::WritingMode mWritingMode;
 
     PerFrameData* Last() {
       PerFrameData* pfd = this;
--- a/testing/web-platform/meta/MANIFEST.json
+++ b/testing/web-platform/meta/MANIFEST.json
@@ -52768,16 +52768,28 @@
       [
        "/css/CSS2/linebox/line-height-bleed-002-ref.xht",
        "=="
       ]
      ],
      {}
     ]
    ],
+   "css/CSS2/linebox/line-height-oof-descendants-001.html": [
+    [
+     "/css/CSS2/linebox/line-height-oof-descendants-001.html",
+     [
+      [
+       "/css/CSS2/linebox/line-height-oof-descendants-001-ref.html",
+       "=="
+      ]
+     ],
+     {}
+    ]
+   ],
    "css/CSS2/linebox/vertical-align-004.xht": [
     [
      "/css/CSS2/linebox/vertical-align-004.xht",
      [
       [
        "/css/CSS2/linebox/vertical-align-004-ref.xht",
        "=="
       ]
@@ -215915,16 +215927,21 @@
      {}
     ]
    ],
    "css/CSS2/linebox/line-height-bleed-002-ref.xht": [
     [
      {}
     ]
    ],
+   "css/CSS2/linebox/line-height-oof-descendants-001-ref.html": [
+    [
+     {}
+    ]
+   ],
    "css/CSS2/linebox/support/1x1-green.png": [
     [
      {}
     ]
    ],
    "css/CSS2/linebox/support/1x1-white.png": [
     [
      {}
@@ -429248,16 +429265,24 @@
   "css/CSS2/linebox/line-height-largest-001.xht": [
    "bf09afbb618ba220ca515cd9825545b80461894c",
    "visual"
   ],
   "css/CSS2/linebox/line-height-normal-recommendation-001.xht": [
    "2927ed2a8c86f6a791db5d6eb670ad1961b17e9e",
    "visual"
   ],
+  "css/CSS2/linebox/line-height-oof-descendants-001-ref.html": [
+   "284fd0f610f5428bea7a5f9c0dee1bdde3a4670b",
+   "support"
+  ],
+  "css/CSS2/linebox/line-height-oof-descendants-001.html": [
+   "bb8949f890f140305ac76beb3f3ae1f2d15b16a3",
+   "reftest"
+  ],
   "css/CSS2/linebox/support/1x1-green.png": [
    "51e7b6974a09eda6cb31337717c5eaeb9c44b443",
    "support"
   ],
   "css/CSS2/linebox/support/1x1-white.png": [
    "71b246439f915ad21c7d39414d9f85c8ed73b4ca",
    "support"
   ],
@@ -545833,33 +545858,33 @@
    "a18743fbd553b68e0ceb657a5d65e3424e6f6a52",
    "support"
   ],
   "html/semantics/scripting-1/the-script-element/module/imports.html": [
    "15b0b32d86bc6411b29c5d978db71053c00a1d65",
    "testharness"
   ],
   "html/semantics/scripting-1/the-script-element/module/instantiation-error-1.html": [
-   "2b4fa3b558dccb50bf0aee12a78e3320501ea1b5",
+   "b48335aa61dc13c34d2a77806f20663e2156bc6f",
    "testharness"
   ],
   "html/semantics/scripting-1/the-script-element/module/instantiation-error-1.js": [
    "f2a20180b6bf5f9c89f5b9541885d55dc8a8ade6",
    "support"
   ],
   "html/semantics/scripting-1/the-script-element/module/instantiation-error-2.html": [
-   "70271ef6fbf9f6e4f6e61438691b6fce317137e9",
+   "e2c860b1b348148fc6b9d77f918894b1bac42c94",
    "testharness"
   ],
   "html/semantics/scripting-1/the-script-element/module/instantiation-error-3.html": [
-   "80fa90a214bb4839703c36f9db36e07f3a2ca7f2",
+   "996d1aa45c5975e13ac0f1e9c9249b3d452ed2e2",
    "testharness"
   ],
   "html/semantics/scripting-1/the-script-element/module/instantiation-error-4.html": [
-   "dbb8eb640576cd4f658e32dec441919e943f8d21",
+   "224fe5510f09c3dd6d58f9dcf61b4d6fca04c96c",
    "testharness"
   ],
   "html/semantics/scripting-1/the-script-element/module/instantiation-error-4a.js": [
    "051586d3af5158af29ee8f46157964252a47ef65",
    "support"
   ],
   "html/semantics/scripting-1/the-script-element/module/instantiation-error-4b.js": [
    "ab58fcee083c1484097922a5e4f90927d798b13d",
@@ -545869,17 +545894,17 @@
    "7ba0fbc265b5d816566bbee9dda4959270cd739c",
    "support"
   ],
   "html/semantics/scripting-1/the-script-element/module/instantiation-error-4d.js": [
    "bcbde9b3648aaac72d8d2ae20846a53cef8abed3",
    "support"
   ],
   "html/semantics/scripting-1/the-script-element/module/instantiation-error-5.html": [
-   "fcc8da57e88ee87592a02888c54bb6d66e5172f6",
+   "7239ae9f5705f7baf5630e67cf4bfdc6c25b108d",
    "testharness"
   ],
   "html/semantics/scripting-1/the-script-element/module/instantiation-error-5a.js": [
    "b946a91f9241587336e2e173ad2e094c3c374e79",
    "support"
   ],
   "html/semantics/scripting-1/the-script-element/module/instantiation-error-5b.js": [
    "4ce41460910bd773fb1ba8432e20187991af5a9e",
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/CSS2/linebox/line-height-oof-descendants-001-ref.html
@@ -0,0 +1,14 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>CSS Test Reference</title>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<style>
+p {
+  font-size: 20px;
+  line-height: 0;
+}
+</style>
+<p>Some paragraph</p>
+<p>Some paragraph</p>
+<p>Some paragraph</p>
+<p>Some other paragraph</p>
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/CSS2/linebox/line-height-oof-descendants-001.html
@@ -0,0 +1,17 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>CSS Test: line-height is not affected by out-of-flow descendants</title>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<link rel="help" href="https://www.w3.org/TR/CSS21/visudet.html#line-height">
+<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1426760">
+<link rel="match" href="line-height-oof-descendants-001-ref.html">
+<style>
+p {
+  font-size: 20px;
+  line-height: 0;
+}
+</style>
+<p><span style="position: absolute;"></span>Some paragraph</p>
+<p><span style="float: left;"></span>Some paragraph</p>
+<p><span style="position: fixed;"></span>Some paragraph</p>
+<p>Some other paragraph</p>