Bug 1562701 - Move the reframing logic of <details> element into WipeContainingBlock. r=dbaron
authorTing-Yu Lin <tlin@mozilla.com>
Thu, 11 Jul 2019 05:34:24 +0000
changeset 482336 10faf756124c232c9b460afc72168ea54fa6ab03
parent 482335 6cc26a5b9fe20bd8b0d7bee9ad10c285be16da98
child 482337 0c076622290967834426f37e4e557f1f475c7250
push id89702
push useraethanyc@gmail.com
push dateThu, 11 Jul 2019 05:35:28 +0000
treeherderautoland@0c0766222909 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdbaron
bugs1562701
milestone70.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1562701 - Move the reframing logic of <details> element into WipeContainingBlock. r=dbaron We don't need to treat a details frame as if it has multiple content insertion points in order to reframe it. We can just put the logic into WipeContainingBlock(). The reftests layout/reftests/details-summary/dynamic-add-*.html also cover adding <summary> into a <details> in various situations. Differential Revision: https://phabricator.services.mozilla.com/D36529
layout/base/nsCSSFrameConstructor.cpp
testing/web-platform/meta/css/css-multicol/multicol-span-all-dynamic-add-013.html.ini
testing/web-platform/tests/css/css-multicol/multicol-span-all-dynamic-add-013-ref.html
testing/web-platform/tests/css/css-multicol/multicol-span-all-dynamic-add-013.html
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -6539,33 +6539,19 @@ void nsCSSFrameConstructor::IssueSingleI
 
     // Call ContentRangeInserted with this node.
     ContentRangeInserted(child, child->GetNextSibling(), mTempFrameTreeState,
                          aInsertionKind);
   }
 }
 
 bool nsCSSFrameConstructor::InsertionPoint::IsMultiple() const {
-  if (!mParentFrame) {
-    return false;
-  }
-
   // Fieldset frames have multiple normal flow child frame lists so handle it
   // the same as if it had multiple content insertion points.
-  if (mParentFrame->IsFieldSetFrame()) {
-    return true;
-  }
-
-  // A details frame moves the first summary frame to be its first child, so we
-  // treat it as if it has multiple content insertion points.
-  if (mParentFrame->IsDetailsFrame()) {
-    return true;
-  }
-
-  return false;
+  return mParentFrame && mParentFrame->IsFieldSetFrame();
 }
 
 nsCSSFrameConstructor::InsertionPoint
 nsCSSFrameConstructor::GetRangeInsertionPoint(nsIContent* aStartChild,
                                               nsIContent* aEndChild,
                                               InsertionKind aInsertionKind) {
   MOZ_ASSERT(aStartChild);
   MOZ_ASSERT(aStartChild->GetParent());
@@ -6779,20 +6765,20 @@ void nsCSSFrameConstructor::ContentAppen
 #ifdef DEBUG
   if (gNoisyContentUpdates && IsFramePartOfIBSplit(parentFrame)) {
     printf("nsCSSFrameConstructor::ContentAppended: parentFrame=");
     parentFrame->ListTag(stdout);
     printf(" is ib-split\n");
   }
 #endif
 
-  // We should never get here with fieldsets or details, since they have
+  // We should never get here with fieldsets, since they have
   // multiple insertion points.
-  MOZ_ASSERT(!parentFrame->IsFieldSetFrame() && !parentFrame->IsDetailsFrame(),
-             "Parent frame should not be fieldset or details!");
+  MOZ_ASSERT(!parentFrame->IsFieldSetFrame(),
+             "Parent frame should not be fieldset!");
 
   nsIFrame* nextSibling = FindNextSiblingForAppend(insertion);
   if (nextSibling) {
     parentFrame = nextSibling->GetParent()->GetContentInsertionFrame();
   } else {
     parentFrame = ::ContinuationToAppendTo(parentFrame);
   }
 
@@ -7158,31 +7144,16 @@ void nsCSSFrameConstructor::ContentRange
     // to locate this legend in the inserted frames and extract it.
     LAYOUT_PHASE_TEMP_EXIT();
     RecreateFramesForContent(insertion.mParentFrame->GetContent(),
                              InsertionKind::Async);
     LAYOUT_PHASE_TEMP_REENTER();
     return;
   }
 
-  // We should only get here with details when doing a single insertion because
-  // we treat details frame as if it has multiple insertion points.
-  MOZ_ASSERT(isSingleInsert || frameType != LayoutFrameType::Details);
-  if (frameType == LayoutFrameType::Details) {
-    // When inserting an element into <details>, just reframe the details frame
-    // and let it figure out where the element should be laid out. It might seem
-    // expensive to recreate the entire details frame, but it's the simplest way
-    // to handle the insertion.
-    LAYOUT_PHASE_TEMP_EXIT();
-    RecreateFramesForContent(insertion.mParentFrame->GetContent(),
-                             InsertionKind::Async);
-    LAYOUT_PHASE_TEMP_REENTER();
-    return;
-  }
-
   // Don't construct kids of leaves
   if (insertion.mParentFrame->IsLeaf()) {
     // Clear lazy bits so we don't try to construct again.
     ClearLazyBits(aStartChild, aEndChild);
     return;
   }
 
   // FIXME(emilio): This looks terribly inefficient if you insert elements deep
@@ -11581,26 +11552,39 @@ bool nsCSSFrameConstructor::WipeContaini
       // Reframing aFrame->GetContent() is good enough, since the content of
       // table pseudo-frames is the ancestor content.
       TRACE("Pseudo-frames going wrong");
       RecreateFramesForContent(aFrame->GetContent(), InsertionKind::Async);
       return true;
     }
   }
 
-  // Situation #6 is a column hierarchy that's getting new children.
+  // Situation #6 is a <details> that's getting new children. When inserting
+  // elements into <details>, we reframe the <details> and let frame constructor
+  // move the main <summary> to the front when constructing the frame
+  // construction items.
+  if (aFrame->IsDetailsFrame()) {
+    TRACE("Details / Summary");
+    RecreateFramesForContent(aFrame->GetContent(), InsertionKind::Async);
+    return true;
+  }
+
+  // Situation #7 is a column hierarchy that's getting new children.
   if (aFrame->IsColumnSetWrapperFrame()) {
     // Reframe the multi-column container whenever elements insert/append
     // into it.
     TRACE("Multi-column");
     RecreateFramesForContent(aFrame->GetContent(), InsertionKind::Async);
     return true;
   }
 
   if (aFrame->HasAnyStateBits(NS_FRAME_HAS_MULTI_COLUMN_ANCESTOR)) {
+    MOZ_ASSERT(!aFrame->IsDetailsFrame(),
+               "Inserting elements into <details> should have been reframed!");
+
     bool anyColumnSpanItems = false;
     for (FCItemIterator iter(aItems); !iter.IsDone(); iter.Next()) {
       if (iter.item().mComputedStyle->StyleColumn()->IsColumnSpanStyle()) {
         anyColumnSpanItems = true;
         break;
       }
     }
 
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/meta/css/css-multicol/multicol-span-all-dynamic-add-013.html.ini
@@ -0,0 +1,2 @@
+[multicol-span-all-dynamic-add-013.html]
+  prefs: [layout.css.column-span.enabled:true]
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-multicol/multicol-span-all-dynamic-add-013-ref.html
@@ -0,0 +1,29 @@
+<!DOCTYPE html>
+<html>
+  <meta charset="utf-8">
+  <title>CSS Multi-column Layout Test: Insert a block into a multicol details containing column-span:all</title>
+  <link rel="author" title="Ting-Yu Lin" href="tlin@mozilla.com">
+  <link rel="author" title="Mozilla" href="http://www.mozilla.org/">
+
+  <style>
+  #details {
+    column-count: 3;
+    column-rule: 6px solid;
+    width: 400px;
+    outline: 1px solid black;
+  }
+  h3 {
+    column-span: all;
+    outline: 1px solid blue;
+  }
+  </style>
+
+  <body>
+    <details open id="details">
+      <div>block1</div>
+      <summary>Summary</summary>
+      <h3>spanner</h3>
+      <div>block2</div>
+    </details>
+  </body>
+</html>
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-multicol/multicol-span-all-dynamic-add-013.html
@@ -0,0 +1,48 @@
+<!DOCTYPE html>
+<html class="reftest-wait">
+  <meta charset="utf-8">
+  <title>CSS Multi-column Layout Test: Insert a block into a multicol details containing column-span:all</title>
+  <link rel="author" title="Ting-Yu Lin" href="tlin@mozilla.com">
+  <link rel="author" title="Mozilla" href="http://www.mozilla.org/">
+  <link rel="help" href="https://drafts.csswg.org/css-multicol-1/#column-span">
+  <link rel="match" href="multicol-span-all-dynamic-add-013-ref.html">
+  <meta name="assert" content="This test checks that <summary> is still rendered as the first element after inserting a block into multicol <details>, at the place before <summary>.">
+
+  <script>
+  function runTest() {
+    document.body.offsetHeight;
+
+    // Create a div, and insert it as the first child of <details>,
+    // just before <summary>.
+    var block1 = document.createElement("div");
+    var block1Text = document.createTextNode("block1");
+    block1.appendChild(block1Text);
+
+    var details = document.getElementById("details");
+    details.insertBefore(block1, details.children[0]);
+
+    document.documentElement.removeAttribute("class");
+  }
+  </script>
+
+  <style>
+  #details {
+    column-count: 3;
+    column-rule: 6px solid;
+    width: 400px;
+    outline: 1px solid black;
+  }
+  h3 {
+    column-span: all;
+    outline: 1px solid blue;
+  }
+  </style>
+
+  <body onload="runTest();">
+    <details open id="details">
+      <summary>Summary</summary>
+      <h3>spanner</h3>
+      <div>block2</div>
+    </details>
+  </body>
+</html>