Bug 1304441 Part 3 - Insert main summary's frame construction item at front of the list. draft
authorTing-Yu Lin <tlin@mozilla.com>
Tue, 04 Oct 2016 00:26:54 +0800
changeset 421010 2afe90b8e963e0a298c337781d78e3f6924ee599
parent 420422 0807db8942c53e27739caf019539524170ba4d2c
child 532947 5839e0f922faad0e3c21c603db43ba4a0c6545f2
push id31352
push userbmo:tlin@mozilla.com
push dateWed, 05 Oct 2016 05:32:09 +0000
bugs1304441, 520605
milestone52.0a1
Bug 1304441 Part 3 - Insert main summary's frame construction item at front of the list. Change the logic that moves the main summary to the front from operating on generated frames in DetailsFrame::SetInitialChildList() to operating on frame construction item list in AddFrameConstructionItemsInternal() so that it will be correct when cooperating with ::first-line. The root cause of the bug reported is because when specifying ::first-line on details element, the first frame of summary element, which is generated due to ib-split, will be wrapped in nsFirstLineFrame. The original code fails to find the summary frame in the wrapper frame and triggers assertion because of the second ib-split summary frame. To fix that, we need to descend into the child list of wrapper frames when checking the main summary. Add original test case as a crashtest as well as reftests to clearly reproduce the issue. Note that in the reftest, the blue color in ::first-line is applied incorrectly to the second line in the summary due to bug 520605. MozReview-Commit-ID: Bv4Vcvxp6pY
layout/base/nsCSSFrameConstructor.cpp
layout/base/nsCSSFrameConstructor.h
layout/generic/DetailsFrame.cpp
layout/generic/DetailsFrame.h
layout/generic/crashtests/1304441.html
layout/generic/crashtests/crashtests.list
layout/reftests/details-summary/details-first-line-ref.html
layout/reftests/details-summary/details-first-line.html
layout/reftests/details-summary/open-details-first-line-1.html
layout/reftests/details-summary/open-details-first-line-2.html
layout/reftests/details-summary/open-details-first-line-ref.html
layout/reftests/details-summary/open-summary-inline-style-ref.html
layout/reftests/details-summary/open-summary-inline-style.html
layout/reftests/details-summary/open-summary-table-cell-style-ref.html
layout/reftests/details-summary/open-summary-table-cell-style.html
layout/reftests/details-summary/reftest.list
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -5891,20 +5891,33 @@ nsCSSFrameConstructor::AddFrameConstruct
     CreateGeneratedContentItem(aState, aParentFrame, aContent, styleContext,
                                CSSPseudoElementType::after, aItems);
     if (canHavePageBreak && display->mBreakAfter) {
       AddPageBreakItem(aContent, aStyleContext, aItems);
     }
     return;
   }
 
-  FrameConstructionItem* item =
-    aItems.AppendItem(data, aContent, aTag, aNameSpaceID,
-                      pendingBinding, styleContext.forget(),
-                      aSuppressWhiteSpaceOptimizations, aAnonChildren);
+  FrameConstructionItem* item = nullptr;
+  if (details && details->IsDetailsEnabled() && details->Open()) {
+    auto* summary = HTMLSummaryElement::FromContentOrNull(aContent);
+    if (summary && summary->IsMainSummary()) {
+      // If details is open, the main summary needs to be rendered as if it is
+      // the first child, so add the item to the front of the item list.
+      item = aItems.PrependItem(data, aContent, aTag, aNameSpaceID,
+                                pendingBinding, styleContext.forget(),
+                                aSuppressWhiteSpaceOptimizations, aAnonChildren);
+    }
+  }
+
+  if (!item) {
+    item = aItems.AppendItem(data, aContent, aTag, aNameSpaceID,
+                             pendingBinding, styleContext.forget(),
+                             aSuppressWhiteSpaceOptimizations, aAnonChildren);
+  }
   item->mIsText = isText;
   item->mIsGeneratedContent = isGeneratedContent;
   item->mIsAnonymousContentCreatorContent =
     aFlags & ITEM_IS_ANONYMOUSCONTENTCREATOR_CONTENT;
   if (isGeneratedContent) {
     NS_ADDREF(item->mContent);
   }
   item->mIsRootPopupgroup =
--- a/layout/base/nsCSSFrameConstructor.h
+++ b/layout/base/nsCSSFrameConstructor.h
@@ -857,16 +857,37 @@ private:
                                   aSuppressWhiteSpaceOptimizations,
                                   aAnonChildren);
       PR_APPEND_LINK(item, &mItems);
       ++mItemCount;
       ++mDesiredParentCounts[item->DesiredParentType()];
       return item;
     }
 
+    // Arguments are the same as AppendItem().
+    FrameConstructionItem* PrependItem(const FrameConstructionData* aFCData,
+                                       nsIContent* aContent,
+                                       nsIAtom* aTag,
+                                       int32_t aNameSpaceID,
+                                       PendingBinding* aPendingBinding,
+                                       already_AddRefed<nsStyleContext>&& aStyleContext,
+                                       bool aSuppressWhiteSpaceOptimizations,
+                                       nsTArray<nsIAnonymousContentCreator::ContentInfo>* aAnonChildren)
+    {
+      FrameConstructionItem* item =
+        new FrameConstructionItem(aFCData, aContent, aTag, aNameSpaceID,
+                                  aPendingBinding, aStyleContext,
+                                  aSuppressWhiteSpaceOptimizations,
+                                  aAnonChildren);
+      PR_INSERT_LINK(item, &mItems);
+      ++mItemCount;
+      ++mDesiredParentCounts[item->DesiredParentType()];
+      return item;
+    }
+
     void AppendUndisplayedItem(nsIContent* aContent,
                                nsStyleContext* aStyleContext) {
       mUndisplayedItems.AppendElement(UndisplayedItem(aContent, aStyleContext));
     }
 
     void InlineItemAdded() { ++mInlineCount; }
     void BlockItemAdded() { ++mBlockCount; }
     void LineParticipantItemAdded() { ++mLineParticipantCount; }
--- a/layout/generic/DetailsFrame.cpp
+++ b/layout/generic/DetailsFrame.cpp
@@ -41,64 +41,51 @@ nsIAtom*
 DetailsFrame::GetType() const
 {
   return nsGkAtoms::detailsFrame;
 }
 
 void
 DetailsFrame::SetInitialChildList(ChildListID aListID, nsFrameList& aChildList)
 {
+#ifdef DEBUG
   if (aListID == kPrincipalList) {
-    HTMLDetailsElement* details = HTMLDetailsElement::FromContent(GetContent());
-    bool isOpen = details->Open();
-
-    if (isOpen) {
-      // If details is open, the first summary needs to be rendered as if it is
-      // the first child.
-      for (nsIFrame* child : aChildList) {
-        HTMLSummaryElement* summary =
-          HTMLSummaryElement::FromContent(child->GetContent());
-
-        if (summary && summary->IsMainSummary()) {
-          // Take out the first summary frame and insert it to the beginning of
-          // the list.
-          aChildList.RemoveFrame(child);
-          aChildList.InsertFrame(nullptr, nullptr, child);
-          break;
-        }
-      }
-    }
-
-#ifdef DEBUG
     CheckValidMainSummary(aChildList);
+  }
 #endif
 
-  }
-
   nsBlockFrame::SetInitialChildList(aListID, aChildList);
 }
 
 #ifdef DEBUG
-void
+bool
 DetailsFrame::CheckValidMainSummary(const nsFrameList& aFrameList) const
 {
   for (nsIFrame* child : aFrameList) {
     HTMLSummaryElement* summary =
       HTMLSummaryElement::FromContent(child->GetContent());
 
     if (child == aFrameList.FirstChild()) {
       if (summary && summary->IsMainSummary()) {
-        break;
+        return true;
+      } else if (child->GetContent() == GetContent()) {
+        // The child frame's content is the same as our content, which means
+        // it's a kind of wrapper frame. Descend into its child list to find
+        // main summary.
+        if (CheckValidMainSummary(child->PrincipalChildList())) {
+          return true;
+        }
       }
     } else {
       NS_ASSERTION(!summary || !summary->IsMainSummary(),
                    "Rest of the children are either not summary element "
                    "or are not the main summary!");
     }
   }
+  return false;
 }
 #endif
 
 void
 DetailsFrame::DestroyFrom(nsIFrame* aDestructRoot)
 {
   nsContentUtils::DestroyAnonymousContent(&mDefaultSummary);
   nsBlockFrame::DestroyFrom(aDestructRoot);
--- a/layout/generic/DetailsFrame.h
+++ b/layout/generic/DetailsFrame.h
@@ -35,18 +35,18 @@ public:
   nsresult GetFrameName(nsAString& aResult) const override
   {
     return MakeFrameName(NS_LITERAL_STRING("Details"), aResult);
   }
 #endif
 
 #ifdef DEBUG
   // Check the frame of the main summary element is the first child in the frame
-  // list.
-  void CheckValidMainSummary(const nsFrameList& aFrameList) const;
+  // list. Returns true if we found the main summary frame; false otherwise.
+  bool CheckValidMainSummary(const nsFrameList& aFrameList) const;
 #endif
 
   void SetInitialChildList(ChildListID aListID,
                            nsFrameList& aChildList) override;
 
   void DestroyFrom(nsIFrame* aDestructRoot) override;
 
   // nsIAnonymousContentCreator
new file mode 100644
--- /dev/null
+++ b/layout/generic/crashtests/1304441.html
@@ -0,0 +1,9 @@
+<details>
+<summary>
+<li>
+<style>
+summary{
+all:initial
+}
+:first-child::first-line
+{}
--- a/layout/generic/crashtests/crashtests.list
+++ b/layout/generic/crashtests/crashtests.list
@@ -634,8 +634,9 @@ load 1278007.html
 load 1279814.html
 load large-border-radius-dashed.html
 load large-border-radius-dashed2.html
 load large-border-radius-dotted.html
 load large-border-radius-dotted2.html
 load 1297427-non-equal-centers.html
 load 1278461-1.html
 load 1278461-2.html
+load 1304441.html
new file mode 100644
--- /dev/null
+++ b/layout/reftests/details-summary/details-first-line-ref.html
@@ -0,0 +1,18 @@
+<!DOCTYPE html>
+<!-- Any copyright is dedicated to the Public Domain.
+   - http://creativecommons.org/publicdomain/zero/1.0/ -->
+
+<html>
+  <style>
+  div#details::first-line {
+    color: blue;
+  }
+  </style>
+  <body>
+    <div id="details">
+      <span>Summary
+        <div>Block in summary</div>
+      </span>
+    </div>
+  </body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/details-summary/details-first-line.html
@@ -0,0 +1,24 @@
+<!DOCTYPE html>
+<!-- Any copyright is dedicated to the Public Domain.
+   - http://creativecommons.org/publicdomain/zero/1.0/ -->
+
+<html>
+  <style>
+  summary {
+    display: inline; /* ::first-line appiles only to inline element. */
+  }
+
+  details::first-line {
+    color: blue;
+  }
+  </style>
+  <body>
+    <details>
+      <summary>Summary
+        <!-- Need ib-split so that the summary has multiple frames. -->
+        <div>Block in summary</div>
+      </summary>
+      <p>This is the details.</p>
+    </details>
+  </body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/details-summary/open-details-first-line-1.html
@@ -0,0 +1,24 @@
+<!DOCTYPE html>
+<!-- Any copyright is dedicated to the Public Domain.
+   - http://creativecommons.org/publicdomain/zero/1.0/ -->
+
+<html>
+  <style>
+  summary {
+    display: inline; /* ::first-line appiles only to inline element. */
+  }
+
+  details::first-line {
+    color: blue;
+  }
+  </style>
+  <body>
+    <details open>
+      <summary>Summary
+        <!-- Need ib-split so that the summary has multiple frames. -->
+        <div>Block in summary</div>
+      </summary>
+      <span>This is the details.</span>
+    </details>
+  </body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/details-summary/open-details-first-line-2.html
@@ -0,0 +1,24 @@
+<!DOCTYPE html>
+<!-- Any copyright is dedicated to the Public Domain.
+   - http://creativecommons.org/publicdomain/zero/1.0/ -->
+
+<html>
+  <style>
+  summary {
+    display: inline; /* ::first-line appiles only to inline element. */
+  }
+
+  details::first-line {
+    color: blue;
+  }
+  </style>
+  <body>
+    <details open>
+      <span>This is the details.</span>
+      <summary>Summary
+        <!-- Need ib-split so that the summary has multiple frames. -->
+        <div>Block in summary</div>
+      </summary>
+    </details>
+  </body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/details-summary/open-details-first-line-ref.html
@@ -0,0 +1,19 @@
+<!DOCTYPE html>
+<!-- Any copyright is dedicated to the Public Domain.
+   - http://creativecommons.org/publicdomain/zero/1.0/ -->
+
+<html>
+  <style>
+  div#details::first-line {
+    color: blue;
+  }
+  </style>
+  <body>
+    <div id="details">
+      <span>Summary
+        <div>Block in summary</div>
+      </span>
+      <span>This is the details.</span>
+    </div>
+  </body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/details-summary/open-summary-inline-style-ref.html
@@ -0,0 +1,14 @@
+<!DOCTYPE html>
+<!-- Any copyright is dedicated to the Public Domain.
+   - http://creativecommons.org/publicdomain/zero/1.0/ -->
+
+<html>
+  <body>
+    <div>
+      <span>Summary
+        <div>Block in summary</div>
+      </span>
+      <p>This is the details.</p>
+    </div>
+  </body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/details-summary/open-summary-inline-style.html
@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<!-- Any copyright is dedicated to the Public Domain.
+   - http://creativecommons.org/publicdomain/zero/1.0/ -->
+
+<html>
+  <style>
+  summary {
+    display: inline;
+  }
+  </style>
+  <body>
+    <details open>
+      <p>This is the details.</p>
+      <!-- Make summary the second element child so that layout will try to
+           render it as the first child. -->
+      <summary>Summary
+        <!-- Need ib-split so that the summary has multiple frames. -->
+        <div>Block in summary</div>
+      </summary>
+    </details>
+  </body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/details-summary/open-summary-table-cell-style-ref.html
@@ -0,0 +1,14 @@
+<!DOCTYPE html>
+<!-- Any copyright is dedicated to the Public Domain.
+   - http://creativecommons.org/publicdomain/zero/1.0/ -->
+
+<html>
+  <body>
+    <div>
+      <span style="display: table-cell;">Summary
+        <div>Block in summary</div>
+      </span>
+      <p>This is the details.</p>
+    </div>
+  </body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/details-summary/open-summary-table-cell-style.html
@@ -0,0 +1,21 @@
+<!DOCTYPE html>
+<!-- Any copyright is dedicated to the Public Domain.
+   - http://creativecommons.org/publicdomain/zero/1.0/ -->
+
+<html>
+  <style>
+  summary {
+    display: table-cell;
+  }
+  </style>
+  <body>
+    <details open>
+      <p>This is the details.</p>
+      <!-- Make summary the second element child so that layout will try to
+           render it as the first child. -->
+      <summary>Summary
+        <div>Block in summary</div>
+      </summary>
+    </details>
+  </body>
+</html>
--- a/layout/reftests/details-summary/reftest.list
+++ b/layout/reftests/details-summary/reftest.list
@@ -6,16 +6,18 @@ pref(dom.details_element.enabled,false) 
 pref(dom.details_element.enabled,false) == no-summary.html disabled-no-summary-ref.html
 
 # Basic <summary> handling
 == multiple-summary.html single-summary.html
 == open-multiple-summary.html open-multiple-summary-ref.html
 == summary-not-first-child.html single-summary.html
 == open-summary-not-first-child.html open-single-summary.html
 == open-summary-block-style.html open-summary-block-style-ref.html
+== open-summary-inline-style.html open-summary-inline-style-ref.html
+== open-summary-table-cell-style.html open-summary-table-cell-style-ref.html
 == no-summary.html no-summary-ref.html
 == open-no-summary.html open-no-summary-ref.html
 == summary-not-in-details.html summary-not-in-details-ref.html
 == summary-not-direct-child.html summary-not-direct-child-ref.html
 == float-in-summary.html float-in-summary-ref.html
 
 # Add elements dynamically
 == dynamic-add-single-summary.html open-single-summary.html
@@ -65,16 +67,19 @@ pref(dom.details_element.enabled,false) 
 # Various properties on details or summary
 == details-display-inline.html details-display-inline-ref.html
 == details-percentage-height-children.html details-percentage-height-children-ref.html
 == details-absolute-children.html details-absolute-children-ref.html
 == details-three-columns.html details-three-columns-ref.html
 == details-writing-mode.html details-writing-mode-ref.html
 == details-in-ol.html details-in-ol-ref.html
 == summary-three-columns.html summary-three-columns-ref.html
+== details-first-line.html details-first-line-ref.html
+== open-details-first-line-1.html open-details-first-line-ref.html
+== open-details-first-line-2.html open-details-first-line-ref.html
 
 # Dispatch mouse click to summary
 == mouse-click-single-summary.html open-single-summary.html
 == mouse-click-twice-single-summary.html single-summary.html
 == mouse-click-open-single-summary.html single-summary.html
 == mouse-click-twice-open-single-summary.html open-single-summary.html
 == mouse-click-open-second-summary.html open-multiple-summary.html
 == mouse-click-overflow-hidden-details.html overflow-hidden-open-details.html