Bug 1395650. Make anonymous column groups into non-inheriting anon boxes, to better match the behavior of other browsers. r=heycam
authorBoris Zbarsky <bzbarsky@mit.edu>
Fri, 01 Sep 2017 00:53:33 -0400
changeset 657221 2cbfa0e50247c97457644f5d731534cbbfe27162
parent 657220 0f75865f19861841443ea3f6d80b9efbff2b3229
child 657222 fe1ff5e55f1ff8ca8f868047b714ab69bf35d690
child 657343 a3585c77e2b1bc5f5fea907e97762f7b47a12033
push id77481
push usergpascutto@mozilla.com
push dateFri, 01 Sep 2017 07:48:04 +0000
reviewersheycam
bugs1395650
milestone57.0a1
Bug 1395650. Make anonymous column groups into non-inheriting anon boxes, to better match the behavior of other browsers. r=heycam
layout/base/GeckoRestyleManager.cpp
layout/base/GeckoRestyleManager.h
layout/base/ServoRestyleManager.cpp
layout/base/ServoRestyleManager.h
layout/base/nsCSSFrameConstructor.cpp
layout/reftests/bugs/1395650-1-ref.html
layout/reftests/bugs/1395650-1.html
layout/reftests/bugs/reftest.list
layout/style/nsCSSAnonBoxList.h
layout/tables/nsTableFrame.cpp
--- a/layout/base/GeckoRestyleManager.cpp
+++ b/layout/base/GeckoRestyleManager.cpp
@@ -1019,26 +1019,23 @@ GeckoRestyleManager::ReparentStyleContex
                    oldContext->GetParent() !=
                      nextContinuationContext->GetParent(),
                    "continuations should have the same style context");
     }
   }
 #endif
 
   if (!newParentContext && !oldContext->GetParent()) {
-    // No need to do anything here.
-#ifdef DEBUG
-    // Make sure we have no children, so we really know there is nothing to do.
-    nsIFrame::ChildListIterator lists(aFrame);
-    for (; !lists.IsDone(); lists.Next()) {
-      MOZ_ASSERT(lists.CurrentList().IsEmpty(),
-                 "Failing to reparent style context for child of "
-                 "non-inheriting anon box");
-    }
-#endif // DEBUG
+    // No need to do anything here for this frame, but we should still reparent
+    // its descendants, because those may have styles that inherit from the
+    // parent of this frame (e.g. non-anonymous columns in an anonymous
+    // colgroup).
+    MOZ_ASSERT(aFrame->StyleContext()->IsNonInheritingAnonBox(),
+               "Why did this frame not end up with a parent context?");
+    ReparentFrameDescendants(aFrame, providerChild);
     return NS_OK;
   }
 
   NS_ASSERTION(newParentContext, "Reparenting something that has no usable"
                " parent? Shouldn't happen!");
   // XXX need to do something here to produce the correct style context for
   // an IB split whose first inline part is inside a first-line frame.
   // Currently the first IB anonymous block's style context takes the first
@@ -1086,36 +1083,17 @@ GeckoRestyleManager::ReparentStyleContex
       // Ensure the new context ends up resolving all the structs the old
       // context resolved.
       if (!copyFromContinuation) {
         newContext->AsGecko()->EnsureSameStructsCached(oldContext);
       }
 
       aFrame->SetStyleContext(newContext);
 
-      nsIFrame::ChildListIterator lists(aFrame);
-      for (; !lists.IsDone(); lists.Next()) {
-        for (nsIFrame* child : lists.CurrentList()) {
-          // only do frames that are in flow
-          if (!(child->GetStateBits() & NS_FRAME_OUT_OF_FLOW) &&
-              child != providerChild) {
-#ifdef DEBUG
-            if (child->IsPlaceholderFrame()) {
-              nsIFrame* outOfFlowFrame =
-                nsPlaceholderFrame::GetRealFrameForPlaceholder(child);
-              NS_ASSERTION(outOfFlowFrame, "no out-of-flow frame");
-
-              NS_ASSERTION(outOfFlowFrame != providerChild,
-                           "Out of flow provider?");
-            }
-#endif
-            ReparentStyleContext(child);
-          }
-        }
-      }
+      ReparentFrameDescendants(aFrame, providerChild);
 
       // If this frame is part of an IB split, then the style context of
       // the next part of the split might be a child of our style context.
       // Reparent its style context just in case one of our ancestors
       // (split or not) hasn't done so already). It's not a problem to
       // reparent the same frame twice because the "if (newContext !=
       // oldContext)" check will prevent us from redoing work.
       if ((aFrame->GetStateBits() & NS_FRAME_PART_OF_IBSPLIT) &&
@@ -1149,16 +1127,42 @@ GeckoRestyleManager::ReparentStyleContex
       DebugVerifyStyleTree(aFrame);
 #endif
     }
   }
 
   return NS_OK;
 }
 
+void
+GeckoRestyleManager::ReparentFrameDescendants(nsIFrame* aFrame,
+                                              nsIFrame* aProviderChild)
+{
+  nsIFrame::ChildListIterator lists(aFrame);
+  for (; !lists.IsDone(); lists.Next()) {
+    for (nsIFrame* child : lists.CurrentList()) {
+      // only do frames that are in flow
+      if (!(child->GetStateBits() & NS_FRAME_OUT_OF_FLOW) &&
+          child != aProviderChild) {
+#ifdef DEBUG
+        if (child->IsPlaceholderFrame()) {
+          nsIFrame* outOfFlowFrame =
+            nsPlaceholderFrame::GetRealFrameForPlaceholder(child);
+          NS_ASSERTION(outOfFlowFrame, "no out-of-flow frame");
+
+          NS_ASSERTION(outOfFlowFrame != aProviderChild,
+                       "Out of flow provider?");
+        }
+#endif
+        ReparentStyleContext(child);
+      }
+    }
+  }
+}
+
 ElementRestyler::ElementRestyler(nsPresContext* aPresContext,
                                  nsIFrame* aFrame,
                                  nsStyleChangeList* aChangeList,
                                  nsChangeHint aHintsHandledByAncestors,
                                  RestyleTracker& aRestyleTracker,
                                  nsTArray<nsCSSSelector*>&
                                    aSelectorsForDescendants,
                                  TreeMatchContext& aTreeMatchContext,
--- a/layout/base/GeckoRestyleManager.h
+++ b/layout/base/GeckoRestyleManager.h
@@ -83,16 +83,26 @@ public:
    * aFrame must be changed to the new parent before this function is called;
    * the new parent style context will be automatically computed based on the
    * new position in the frame tree.
    *
    * @param aFrame the root of the subtree to reparent.  Must not be null.
    */
   nsresult ReparentStyleContext(nsIFrame* aFrame);
 
+private:
+  /**
+   * Reparent the descendants of aFrame.  This is used by ReparentStyleContext
+   * and shouldn't be called by anyone else.  aProviderChild, if non-null, is a
+   * child that was the style parent for aFrame and hence shouldn't be
+   * reparented.
+   */
+  void ReparentFrameDescendants(nsIFrame* aFrame, nsIFrame* aProviderChild);
+
+public:
   void ClearSelectors() {
     mPendingRestyles.ClearSelectors();
   }
 
   void PostRestyleEventForLazyConstruction() { PostRestyleEventInternal(); }
 
 private:
   void PostRestyleEventInternal();
--- a/layout/base/ServoRestyleManager.cpp
+++ b/layout/base/ServoRestyleManager.cpp
@@ -1483,44 +1483,41 @@ ServoRestyleManager::DoReparentStyleCont
     for (; outOfFlow; outOfFlow = outOfFlow->GetNextContinuation()) {
       DoReparentStyleContext(outOfFlow, aStyleSet);
     }
   }
 
   nsIFrame* providerFrame;
   nsStyleContext* newParentContext =
     aFrame->GetParentStyleContext(&providerFrame);
-  if (!newParentContext) {
-    // No need to do anything here.
-#ifdef DEBUG
-    // Make sure we have no children, so we really know there is nothing to do.
-    nsIFrame::ChildListIterator lists(aFrame);
-    for (; !lists.IsDone(); lists.Next()) {
-      MOZ_ASSERT(lists.CurrentList().IsEmpty(),
-                 "Failing to reparent style context for child of "
-                 "non-inheriting anon box");
-    }
-#endif // DEBUG
-    return;
-  }
-
   // If our provider is our child, we want to reparent it first, because we
   // inherit style from it.
   bool isChild = providerFrame && providerFrame->GetParent() == aFrame;
   nsIFrame* providerChild = nullptr;
   if (isChild) {
     DoReparentStyleContext(providerFrame, aStyleSet);
     // Get the style context again after ReparentStyleContext() which might have
     // changed it.
     newParentContext = providerFrame->StyleContext();
     providerChild = providerFrame;
     MOZ_ASSERT(!providerFrame->HasAnyStateBits(NS_FRAME_OUT_OF_FLOW),
                "Out of flow provider?");
   }
 
+  if (!newParentContext) {
+    // No need to do anything here for this frame, but we should still reparent
+    // its descendants, because those may have styles that inherit from the
+    // parent of this frame (e.g. non-anonymous columns in an anonymous
+    // colgroup).
+    MOZ_ASSERT(aFrame->StyleContext()->IsNonInheritingAnonBox(),
+               "Why did this frame not end up with a parent context?");
+    ReparentFrameDescendants(aFrame, providerChild, aStyleSet);
+    return;
+  }
+
   bool isElement = aFrame->GetContent()->IsElement();
 
   // We probably don't want to initiate transitions from
   // ReparentStyleContext, since we call it during frame
   // construction rather than in response to dynamic changes.
   // Also see the comment at the start of
   // nsTransitionManager::ConsiderInitiatingTransition.
   //
@@ -1600,24 +1597,32 @@ ServoRestyleManager::DoReparentStyleCont
   // over kids, and we don't want to update the block part of a block-in-inline
   // split if the inline is on the first line but the block is not (and if the
   // block is, it's the child of something else on the first line and will get
   // updated as a child).  And given how this method ends up getting called, if
   // we reach here for a table frame, we are already in the middle of
   // reparenting the table wrapper frame.  So no need to
   // UpdateStyleOfOwnedAnonBoxes() here.
 
+  ReparentFrameDescendants(aFrame, providerChild, aStyleSet);
+
+  // We do not need to do the equivalent of UpdateFramePseudoElementStyles,
+  // because those are hadled by our descendant walk.
+}
+
+void
+ServoRestyleManager::ReparentFrameDescendants(nsIFrame* aFrame,
+                                              nsIFrame* aProviderChild,
+                                              ServoStyleSet& aStyleSet)
+{
   nsIFrame::ChildListIterator lists(aFrame);
   for (; !lists.IsDone(); lists.Next()) {
     for (nsIFrame* child : lists.CurrentList()) {
       // only do frames that are in flow
       if (!(child->GetStateBits() & NS_FRAME_OUT_OF_FLOW) &&
-          child != providerChild) {
+          child != aProviderChild) {
         DoReparentStyleContext(child, aStyleSet);
       }
     }
   }
-
-  // We do not need to do the equivalent of UpdateFramePseudoElementStyles,
-  // because those are hadled by our descendant walk.
 }
 
 } // namespace mozilla
--- a/layout/base/ServoRestyleManager.h
+++ b/layout/base/ServoRestyleManager.h
@@ -216,16 +216,27 @@ public:
                         nsIAtom* aAttribute, int32_t aModType,
                         const nsAttrValue* aOldValue);
 
   // This is only used to reparent things when moving them in/out of the
   // ::first-line.  Once we get rid of the Gecko style system, we should rename
   // this method accordingly (e.g. to ReparentStyleContextForFirstLine).
   nsresult ReparentStyleContext(nsIFrame* aFrame);
 
+private:
+  /**
+   * Reparent the descendants of aFrame.  This is used by ReparentStyleContext
+   * and shouldn't be called by anyone else.  aProviderChild, if non-null, is a
+   * child that was the style parent for aFrame and hence shouldn't be
+   * reparented.
+   */
+  void ReparentFrameDescendants(nsIFrame* aFrame, nsIFrame* aProviderChild,
+                                ServoStyleSet& aStyleSet);
+
+public:
   /**
    * Clears the ServoElementData and HasDirtyDescendants from all elements
    * in the subtree rooted at aElement.
    */
   static void ClearServoDataFromSubtree(Element* aElement);
 
   /**
    * Clears HasDirtyDescendants and RestyleData from all elements in the
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -10242,17 +10242,18 @@ nsCSSFrameConstructor::sPseudoParentData
                      FCDATA_DESIRED_PARENT_TYPE_TO_BITS(eTypeTable),
                      &nsCSSFrameConstructor::ConstructTableRowOrRowGroup),
     &nsCSSAnonBoxes::tableRowGroup
   },
   { // Column group
     FCDATA_DECL(FCDATA_IS_TABLE_PART | FCDATA_SKIP_FRAMESET |
                 FCDATA_DISALLOW_OUT_OF_FLOW | FCDATA_USE_CHILD_ITEMS |
                 FCDATA_SKIP_ABSPOS_PUSH |
-                FCDATA_IS_WRAPPER_ANON_BOX |
+                // Not FCDATA_IS_WRAPPER_ANON_BOX, because we don't need to
+                // restyle these: they have non-inheriting style contexts.
                 FCDATA_DESIRED_PARENT_TYPE_TO_BITS(eTypeTable),
                 NS_NewTableColGroupFrame),
     &nsCSSAnonBoxes::tableColGroup
   },
   { // Table
     FULL_CTOR_FCDATA(FCDATA_SKIP_FRAMESET | FCDATA_USE_CHILD_ITEMS |
                      FCDATA_IS_WRAPPER_ANON_BOX,
                      &nsCSSFrameConstructor::ConstructTable),
@@ -10881,19 +10882,26 @@ nsCSSFrameConstructor::WrapItemsInPseudo
 
   if (pseudoType == nsCSSAnonBoxes::table &&
       (parentDisplay == StyleDisplay::Inline ||
        parentDisplay == StyleDisplay::RubyBase ||
        parentDisplay == StyleDisplay::RubyText)) {
     pseudoType = nsCSSAnonBoxes::inlineTable;
   }
 
-  already_AddRefed<nsStyleContext> wrapperStyle =
-    mPresShell->StyleSet()->ResolveInheritingAnonymousBoxStyle(pseudoType,
-                                                               aParentStyle);
+  already_AddRefed<nsStyleContext> wrapperStyle;
+  if (pseudoData.mFCData.mBits & FCDATA_IS_WRAPPER_ANON_BOX) {
+    wrapperStyle =
+      mPresShell->StyleSet()->ResolveInheritingAnonymousBoxStyle(pseudoType,
+                                                                 aParentStyle);
+  } else {
+    wrapperStyle =
+      mPresShell->StyleSet()->ResolveNonInheritingAnonymousBoxStyle(pseudoType);
+  }
+
   FrameConstructionItem* newItem =
     new FrameConstructionItem(&pseudoData.mFCData,
                               // Use the content of our parent frame
                               aParentContent,
                               // Lie about the tag; it doesn't matter anyway
                               pseudoType,
                               // The namespace does matter, however; it needs
                               // to match that of our first child item to
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/1395650-1-ref.html
@@ -0,0 +1,4 @@
+<!DOCTYPE html>
+<table>
+  <tr><td>This text should be visible</td></tr>
+</table>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/1395650-1.html
@@ -0,0 +1,6 @@
+<!DOCTYPE html>
+<table style="visibility: collapse">
+  <tbody style="visibility: visible">
+    <tr><td>This text should be visible</td></tr>
+  </tbody>
+</table>
--- a/layout/reftests/bugs/reftest.list
+++ b/layout/reftests/bugs/reftest.list
@@ -2033,8 +2033,9 @@ fails-if(!stylo||styloVsGecko) == 136516
 needs-focus == 1377447-1.html 1377447-1-ref.html
 needs-focus != 1377447-1.html 1377447-2.html
 == 1379041.html 1379041-ref.html
 == 1379696.html 1379696-ref.html
 == 1380224-1.html 1380224-1-ref.html
 == 1384065.html 1384065-ref.html
 == 1384275-1.html 1384275-1-ref.html
 == 1381821.html 1381821-ref.html
+== 1395650-1.html 1395650-1-ref.html
--- a/layout/style/nsCSSAnonBoxList.h
+++ b/layout/style/nsCSSAnonBoxList.h
@@ -76,17 +76,17 @@ CSS_ANON_BOX(dropDownList, ":-moz-dropdo
 CSS_ANON_BOX(fieldsetContent, ":-moz-fieldset-content")
 CSS_NON_INHERITING_ANON_BOX(framesetBlank, ":-moz-frameset-blank")
 CSS_ANON_BOX(mozDisplayComboboxControlFrame, ":-moz-display-comboboxcontrol-frame")
 CSS_ANON_BOX(htmlCanvasContent, ":-moz-html-canvas-content")
 
 CSS_WRAPPER_ANON_BOX(inlineTable, ":-moz-inline-table")
 CSS_WRAPPER_ANON_BOX(table, ":-moz-table")
 CSS_WRAPPER_ANON_BOX(tableCell, ":-moz-table-cell")
-CSS_WRAPPER_ANON_BOX(tableColGroup, ":-moz-table-column-group")
+CSS_NON_INHERITING_ANON_BOX(tableColGroup, ":-moz-table-column-group")
 CSS_NON_INHERITING_ANON_BOX(tableCol, ":-moz-table-column")
 CSS_ANON_BOX(tableWrapper, ":-moz-table-wrapper")
 CSS_WRAPPER_ANON_BOX(tableRowGroup, ":-moz-table-row-group")
 CSS_WRAPPER_ANON_BOX(tableRow, ":-moz-table-row")
 
 CSS_ANON_BOX(canvas, ":-moz-canvas")
 CSS_NON_INHERITING_ANON_BOX(pageBreak, ":-moz-pagebreak")
 CSS_ANON_BOX(page, ":-moz-page")
--- a/layout/tables/nsTableFrame.cpp
+++ b/layout/tables/nsTableFrame.cpp
@@ -683,18 +683,17 @@ nsTableColGroupFrame*
 nsTableFrame::CreateAnonymousColGroupFrame(nsTableColGroupType aColGroupType)
 {
   nsIContent* colGroupContent = GetContent();
   nsPresContext* presContext = PresContext();
   nsIPresShell *shell = presContext->PresShell();
 
   RefPtr<nsStyleContext> colGroupStyle;
   colGroupStyle = shell->StyleSet()->
-    ResolveInheritingAnonymousBoxStyle(nsCSSAnonBoxes::tableColGroup,
-                                       mStyleContext);
+    ResolveNonInheritingAnonymousBoxStyle(nsCSSAnonBoxes::tableColGroup);
   // Create a col group frame
   nsIFrame* newFrame = NS_NewTableColGroupFrame(shell, colGroupStyle);
   ((nsTableColGroupFrame *)newFrame)->SetColType(aColGroupType);
   newFrame->Init(colGroupContent, this, nullptr);
   return (nsTableColGroupFrame *)newFrame;
 }
 
 void
@@ -8046,37 +8045,16 @@ void
 nsTableFrame::AppendDirectlyOwnedAnonBoxes(nsTArray<OwnedAnonBox>& aResult)
 {
   nsIFrame* wrapper = GetParent();
   MOZ_ASSERT(wrapper->StyleContext()->GetPseudo() ==
                nsCSSAnonBoxes::tableWrapper,
              "What happened to our parent?");
   aResult.AppendElement(
     OwnedAnonBox(wrapper, &UpdateStyleOfOwnedAnonBoxesForTableWrapper));
-
-  // We may also have an anonymous colgroup that we're responsible for.
-  // Specifically, we can have three types of colgroup frames: (A) corresponding
-  // to actual elements with "display: table-column-group", (B) wrapping runs of
-  // "display: table-column" kids, and (C) one colgroup frame added at the end
-  // to hold the anonymous colframes we need so each cell has an associated
-  // colframe.
-  //
-  // These types of colgroups are supposed to correspond to the values of the
-  // nsTableColGroupType enum: type (A) to eColGroupContent, type (B) to
-  // eColGroupAnonymousCol, and type (C) to eColGroupAnonymousCell.  But we
-  // never actually set eColGroupAnonymousCol on any colgroups right now; see
-  // bug 1387568.  In any case, eColGroupAnonymousCell works correctly to detect
-  // colgroups of type (C), which are the ones we want to restyle here.  Type
-  // (A) will be restyled via their element, and type (B) via the machinery for
-  // restyling wrapper anonymous frames.
-  auto colGroupFrame =
-    static_cast<nsTableColGroupFrame*>(mColGroups.LastChild());
-  if (colGroupFrame && colGroupFrame->GetColType() == eColGroupAnonymousCell) {
-    aResult.AppendElement(colGroupFrame);
-  }
 }
 
 /* static */ void
 nsTableFrame::UpdateStyleOfOwnedAnonBoxesForTableWrapper(
   nsIFrame* aOwningFrame,
   nsIFrame* aWrapperFrame,
   ServoRestyleState& aRestyleState)
 {