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
--- 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