Bug 438987. Propagate background color correctly when the root element is a table. r+sr=dbaron
authorRobert O'Callahan <robert@ocallahan.org>
Fri, 08 Aug 2008 13:52:41 +1200
changeset 16502 9628f4628ed46cc75bf48550086e036aea6e34a4
parent 16501 03693a68bee3e8595b845d894877d92bc4ccbb28
child 16503 d8140e40fbc31bcfb361b4da25d7a118ef4d24e6
push id1085
push userrocallahan@mozilla.com
push dateFri, 08 Aug 2008 01:52:55 +0000
treeherdermozilla-central@9628f4628ed4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs438987
milestone1.9.1a2pre
Bug 438987. Propagate background color correctly when the root element is a table. r+sr=dbaron
layout/base/nsCSSFrameConstructor.cpp
layout/base/nsCSSFrameConstructor.h
layout/base/nsCSSRendering.cpp
layout/reftests/bugs/438987-1-ref.html
layout/reftests/bugs/438987-1.html
layout/reftests/bugs/438987-2-ref.html
layout/reftests/bugs/438987-2a.html
layout/reftests/bugs/438987-2b.html
layout/reftests/bugs/438987-2c.html
layout/reftests/bugs/438987-2d.html
layout/reftests/bugs/reftest.list
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -1789,16 +1789,17 @@ GetChildListNameFor(nsIFrame*       aChi
 
 //----------------------------------------------------------------------
 
 nsCSSFrameConstructor::nsCSSFrameConstructor(nsIDocument *aDocument,
                                              nsIPresShell *aPresShell)
   : mDocument(aDocument)
   , mPresShell(aPresShell)
   , mInitialContainingBlock(nsnull)
+  , mRootElementStyleFrame(nsnull)
   , mFixedContainingBlock(nsnull)
   , mDocElementContainingBlock(nsnull)
   , mGfxScrollFrame(nsnull)
   , mPageSequenceFrame(nsnull)
   , mUpdateCount(0)
   , mQuotesDirty(PR_FALSE)
   , mCountersDirty(PR_FALSE)
   , mInitialContainingBlockIsAbsPosContainer(PR_FALSE)
@@ -4375,16 +4376,26 @@ nsCSSFrameConstructor::ConstructDocEleme
   // set the primary frame
   aState.mFrameManager->SetPrimaryFrameFor(aDocElement, contentFrame);
 
   *aNewFrame = contentFrame;
 
   mInitialContainingBlock = contentFrame;
   mInitialContainingBlockIsAbsPosContainer = PR_FALSE;
 
+  // Figure out which frame has the main style for the document element,
+  // assigning it to mRootElementStyleFrame.
+  // Backgrounds should be propagated from that frame to the viewport.
+  PRBool isChild;
+  contentFrame->GetParentStyleContextFrame(aState.mPresContext,
+          &mRootElementStyleFrame, &isChild);
+  if (!isChild) {
+    mRootElementStyleFrame = mInitialContainingBlock;
+  }
+
   // if it was a table then we don't need to process our children.
   if (!docElemIsTable) {
     // Process the child content
     nsFrameConstructorSaveState absoluteSaveState;
     nsFrameConstructorSaveState floatSaveState;
     nsFrameItems                childItems;
 
     if (isBlockFrame) {
@@ -9455,17 +9466,17 @@ nsCSSFrameConstructor::ContentRemoved(ns
       }
       else {
         rv = frameManager->RemoveFrame(parentFrame, nsnull, childFrame);
       }
     }
 
     if (mInitialContainingBlock == childFrame) {
       mInitialContainingBlock = nsnull;
-      mInitialContainingBlockIsAbsPosContainer = PR_FALSE;
+      mRootElementStyleFrame = nsnull;
     }
 
     if (haveFLS && mInitialContainingBlock) {
       NS_ASSERTION(containingBlock == GetFloatContainingBlock(parentFrame),
                    "What happened here?");
       nsFrameConstructorState state(mPresShell, mFixedContainingBlock,
                                     GetAbsoluteContainingBlock(parentFrame),
                                     containingBlock);
--- a/layout/base/nsCSSFrameConstructor.h
+++ b/layout/base/nsCSSFrameConstructor.h
@@ -243,17 +243,23 @@ public:
                                 nsIContent*     aChild,
                                 nsIFrame**      aResult,
                                 PRBool          aIsAppend,
                                 PRBool          aIsScrollbar,
                                 nsILayoutHistoryState* aFrameState);
 
   nsresult RemoveMappingsForFrameSubtree(nsIFrame* aRemovedFrame);
 
+  // This is misnamed! This returns the outermost frame for the root element
   nsIFrame* GetInitialContainingBlock() { return mInitialContainingBlock; }
+  // This returns the outermost frame for the root element
+  nsIFrame* GetRootElementFrame() { return mInitialContainingBlock; }
+  // This returns the frame for the root element that does not
+  // have a psuedo-element style
+  nsIFrame* GetRootElementStyleFrame() { return mRootElementStyleFrame; }
   nsIFrame* GetPageSequenceFrame() { return mPageSequenceFrame; }
 
 private:
 
   nsresult ReconstructDocElementHierarchyInternal();
 
   nsresult ReinsertContent(nsIContent*    aContainer,
                            nsIContent*    aChild);
@@ -1139,18 +1145,25 @@ private:
     nsCOMPtr<nsIPresShell> mPresShell;
     nsLazyFrameConstructionCallback* mCallback;
     void* mArg;
   };
 
   nsIDocument*        mDocument;  // Weak ref
   nsIPresShell*       mPresShell; // Weak ref
 
+  // This is not the real CSS 2.1 "initial containing block"! It is just
+  // the outermost frame for the root element.
   nsIFrame*           mInitialContainingBlock;
+  // This is the frame for the root element that has no pseudo-element style.
+  nsIFrame*           mRootElementStyleFrame;
+  // This is the containing block for fixed-pos frames --- the viewport
   nsIFrame*           mFixedContainingBlock;
+  // This is the containing block that contains the root element ---
+  // the real "initial containing block" according to CSS 2.1.
   nsIFrame*           mDocElementContainingBlock;
   nsIFrame*           mGfxScrollFrame;
   nsIFrame*           mPageSequenceFrame;
   nsQuoteList         mQuoteList;
   nsCounterManager    mCounterManager;
   PRUint16            mUpdateCount;
   PRPackedBool        mQuotesDirty : 1;
   PRPackedBool        mCountersDirty : 1;
--- a/layout/base/nsCSSRendering.cpp
+++ b/layout/base/nsCSSRendering.cpp
@@ -73,16 +73,17 @@
 #include "nsLayoutUtils.h"
 #include "nsINameSpaceManager.h"
 #include "nsBlockFrame.h"
 #include "gfxContext.h"
 #include "nsIInterfaceRequestorUtils.h"
 #include "gfxPlatform.h"
 #include "gfxImageSurface.h"
 #include "nsStyleStructInlines.h"
+#include "nsCSSFrameConstructor.h"
 
 #include "nsCSSRenderingBorders.h"
 
 // To avoid storing this data on nsInlineFrame (bloat) and to avoid
 // recalculating this for each frame in a continuation (perf), hold
 // a cache of various coordinate information that we need in order
 // to paint inline backgrounds.
 struct InlineBackgroundData
@@ -1002,161 +1003,133 @@ nsCSSRendering::FindNonTransparentBackgr
  * |FindBackground| returns true if a background should be painted, and
  * the resulting style context to use for the background information
  * will be filled in to |aBackground|.  It fills in a boolean indicating
  * whether the frame is the canvas frame to allow PaintBackground to
  * ensure that it always paints something non-transparent for the
  * canvas.
  */
 
-// Returns nsnull if aFrame is not a canvas frame.
-// Otherwise, it returns the frame we should look for the background on.
-// This is normally aFrame but if aFrame is the viewport, we need to
-// look for the background starting at the scroll root (which shares
-// style context with the document root) or the document root itself.
+// Returns true if aFrame is a canvas frame.
 // We need to treat the viewport as canvas because, even though
 // it does not actually paint a background, we need to get the right
 // background style so we correctly detect transparent documents.
-inline nsIFrame*
+inline PRBool
 IsCanvasFrame(nsIFrame *aFrame)
 {
   nsIAtom* frameType = aFrame->GetType();
-  if (frameType == nsGkAtoms::canvasFrame ||
-      frameType == nsGkAtoms::rootFrame ||
-      frameType == nsGkAtoms::pageFrame ||
-      frameType == nsGkAtoms::pageContentFrame) {
-    return aFrame;
-  } else if (frameType == nsGkAtoms::viewportFrame) {
-    nsIFrame* firstChild = aFrame->GetFirstChild(nsnull);
-    if (firstChild) {
-      return firstChild;
-    }
-  }
-  
-  return nsnull;
+  return frameType == nsGkAtoms::canvasFrame ||
+         frameType == nsGkAtoms::rootFrame ||
+         frameType == nsGkAtoms::pageFrame ||
+         frameType == nsGkAtoms::pageContentFrame ||
+         frameType == nsGkAtoms::viewportFrame;
 }
 
 inline PRBool
-FindCanvasBackground(nsIFrame* aForFrame,
+FindCanvasBackground(nsIFrame* aForFrame, nsIFrame* aRootElementFrame,
                      const nsStyleBackground** aBackground)
 {
-  // XXXldb What if the root element is positioned, etc.?  (We don't
-  // allow that yet, do we?)
-  nsIFrame *firstChild = aForFrame->GetFirstChild(nsnull);
-  if (firstChild) {
-    const nsStyleBackground* result = firstChild->GetStyleBackground();
-    nsIFrame* topFrame = aForFrame;
-
-    if (firstChild->GetType() == nsGkAtoms::pageContentFrame) {
-      topFrame = firstChild->GetFirstChild(nsnull);
-      NS_ASSERTION(topFrame,
-                   "nsPageContentFrame is missing a normal flow child");
-      if (!topFrame) {
-        return PR_FALSE;
-      }
-      NS_ASSERTION(topFrame->GetContent(),
-                   "nsPageContentFrame child without content");
-      result = topFrame->GetStyleBackground();
-    }
+  if (aRootElementFrame) {
+    const nsStyleBackground* result = aRootElementFrame->GetStyleBackground();
 
     // Check if we need to do propagation from BODY rather than HTML.
     if (result->IsTransparent()) {
-      nsIContent* content = topFrame->GetContent();
-      if (content) {
-        // Use |GetOwnerDoc| so it works during destruction.
-        nsIDocument* document = content->GetOwnerDoc();
-        nsCOMPtr<nsIHTMLDocument> htmlDoc = do_QueryInterface(document);
-        if (htmlDoc) {
-          nsIContent* bodyContent = htmlDoc->GetBodyContentExternal();
-          // We need to null check the body node (bug 118829) since
-          // there are cases, thanks to the fix for bug 5569, where we
-          // will reflow a document with no body.  In particular, if a
-          // SCRIPT element in the head blocks the parser and then has a
-          // SCRIPT that does "document.location.href = 'foo'", then
-          // nsParser::Terminate will call |DidBuildModel| methods
-          // through to the content sink, which will call |StartLayout|
-          // and thus |InitialReflow| on the pres shell.  See bug 119351
-          // for the ugly details.
-          if (bodyContent) {
-            nsIFrame *bodyFrame = aForFrame->PresContext()->GetPresShell()->
-              GetPrimaryFrameFor(bodyContent);
-            if (bodyFrame)
-              result = bodyFrame->GetStyleBackground();
-          }
+      nsIContent* content = aRootElementFrame->GetContent();
+      // The root element content can't be null. We wouldn't know what
+      // frame to create for aRootElementFrame.
+      // Use |GetOwnerDoc| so it works during destruction.
+      nsIDocument* document = content->GetOwnerDoc();
+      nsCOMPtr<nsIHTMLDocument> htmlDoc = do_QueryInterface(document);
+      if (htmlDoc) {
+        nsIContent* bodyContent = htmlDoc->GetBodyContentExternal();
+        // We need to null check the body node (bug 118829) since
+        // there are cases, thanks to the fix for bug 5569, where we
+        // will reflow a document with no body.  In particular, if a
+        // SCRIPT element in the head blocks the parser and then has a
+        // SCRIPT that does "document.location.href = 'foo'", then
+        // nsParser::Terminate will call |DidBuildModel| methods
+        // through to the content sink, which will call |StartLayout|
+        // and thus |InitialReflow| on the pres shell.  See bug 119351
+        // for the ugly details.
+        if (bodyContent) {
+          nsIFrame *bodyFrame = aForFrame->PresContext()->GetPresShell()->
+            GetPrimaryFrameFor(bodyContent);
+          if (bodyFrame)
+            result = bodyFrame->GetStyleBackground();
         }
       }
     }
 
     *aBackground = result;
   } else {
     // This should always give transparent, so we'll fill it in with the
     // default color if needed.  This seems to happen a bit while a page is
     // being loaded.
     *aBackground = aForFrame->GetStyleBackground();
   }
   
   return PR_TRUE;
 }
 
 inline PRBool
-FindElementBackground(nsIFrame* aForFrame,
+FindElementBackground(nsIFrame* aForFrame, nsIFrame* aRootElementFrame,
                       const nsStyleBackground** aBackground)
 {
-  nsIFrame *parentFrame = aForFrame->GetParent();
-  // XXXldb We shouldn't have to null-check |parentFrame| here.
-  if (parentFrame && IsCanvasFrame(parentFrame) == parentFrame) {
-    // Check that we're really the root (rather than in another child list).
-    nsIFrame *childFrame = parentFrame->GetFirstChild(nsnull);
-    if (childFrame == aForFrame)
-      return PR_FALSE; // Background was already drawn for the canvas.
+  if (aForFrame == aRootElementFrame) {
+    // We must have propagated our background to the viewport or canvas. Abort.
+    return PR_FALSE;
   }
 
   *aBackground = aForFrame->GetStyleBackground();
 
   // Return true unless the frame is for a BODY element whose background
   // was propagated to the viewport.
 
+  nsIContent* content = aForFrame->GetContent();
+  if (!content || content->Tag() != nsGkAtoms::body)
+    return PR_TRUE; // not frame for a "body" element
+  // It could be a non-HTML "body" element but that's OK, we'd fail the
+  // bodyContent check below
+
   if (aForFrame->GetStyleContext()->GetPseudoType())
     return PR_TRUE; // A pseudo-element frame.
 
-  nsIContent* content = aForFrame->GetContent();
-  if (!content || !content->IsNodeOfType(nsINode::eHTML))
-    return PR_TRUE;  // not frame for an HTML element
-
-  if (!parentFrame)
-    return PR_TRUE; // no parent to look at
-
-  if (content->Tag() != nsGkAtoms::body)
-    return PR_TRUE; // not frame for <BODY> element
-
   // We should only look at the <html> background if we're in an HTML document
   nsIDocument* document = content->GetOwnerDoc();
   nsCOMPtr<nsIHTMLDocument> htmlDoc = do_QueryInterface(document);
   if (!htmlDoc)
     return PR_TRUE;
 
   nsIContent* bodyContent = htmlDoc->GetBodyContentExternal();
   if (bodyContent != content)
     return PR_TRUE; // this wasn't the background that was propagated
 
-  const nsStyleBackground* htmlBG = parentFrame->GetStyleBackground();
+  // This can be called even when there's no root element yet, during frame
+  // construction, via nsLayoutUtils::FrameHasTransparency and
+  // nsContainerFrame::SyncFrameViewProperties.
+  if (!aRootElementFrame)
+    return PR_TRUE;
+
+  const nsStyleBackground* htmlBG = aRootElementFrame->GetStyleBackground();
   return !htmlBG->IsTransparent();
 }
 
 PRBool
 nsCSSRendering::FindBackground(nsPresContext* aPresContext,
                                nsIFrame* aForFrame,
                                const nsStyleBackground** aBackground,
                                PRBool* aIsCanvas)
 {
-  nsIFrame* canvasFrame = IsCanvasFrame(aForFrame);
-  *aIsCanvas = canvasFrame != nsnull;
-  return canvasFrame
-      ? FindCanvasBackground(canvasFrame, aBackground)
-      : FindElementBackground(aForFrame, aBackground);
+  nsIFrame* rootElementFrame =
+    aPresContext->PresShell()->FrameConstructor()->GetRootElementStyleFrame();
+  PRBool isCanvasFrame = IsCanvasFrame(aForFrame);
+  *aIsCanvas = isCanvasFrame;
+  return isCanvasFrame
+      ? FindCanvasBackground(aForFrame, rootElementFrame, aBackground)
+      : FindElementBackground(aForFrame, rootElementFrame, aBackground);
 }
 
 void
 nsCSSRendering::DidPaint()
 {
   gInlineBGData->Reset();
 }
 
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/438987-1-ref.html
@@ -0,0 +1,6 @@
+<!DOCTYPE HTML>
+<html style="background:yellow;">
+<body style="margin:0">
+The yellow table background should be propagated to the viewport.
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/438987-1.html
@@ -0,0 +1,7 @@
+<!DOCTYPE HTML>
+<!-- check that the background of a root table element propagates to the viewport -->
+<html style="display:table; background:yellow;">
+<body style="margin:0">
+The yellow table background should be propagated to the viewport.
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/438987-2-ref.html
@@ -0,0 +1,6 @@
+<!DOCTYPE HTML>
+<html>
+<body>
+<div style="background:rgba(0,0,255,0.5); position:fixed; top:0; left:0; right:0; bottom:0;"></div>
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/438987-2a.html
@@ -0,0 +1,5 @@
+<!DOCTYPE HTML>
+<html>
+<body style="background:rgba(0,0,255,0.5);">
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/438987-2b.html
@@ -0,0 +1,5 @@
+<!DOCTYPE HTML>
+<html style="background:rgba(0,0,255,0.5);">
+<body>
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/438987-2c.html
@@ -0,0 +1,5 @@
+<!DOCTYPE HTML>
+<html style="background:rgba(0,0,255,0.5);">
+<body style="background:red;">
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/438987-2d.html
@@ -0,0 +1,5 @@
+<!DOCTYPE HTML>
+<html style="background:rgba(0,0,255,0.5);">
+<body style="background:red;">
+</body>
+</html>
--- a/layout/reftests/bugs/reftest.list
+++ b/layout/reftests/bugs/reftest.list
@@ -856,11 +856,16 @@ random == 429849-1.html 429849-1-ref.htm
 == 431341-2.html 431341-2-ref.html
 == 431520-1.html 431520-1-ref.html
 == 440112.html 440112-ref.html
 == 433640-1.html 433640-1-ref.html
 == 433700.html 433700-ref.html
 == 436356-1.html 436356-1-ref.html
 == 436356-2.html 436356-2-ref.html
 == 438981-1.xhtml about:blank
+== 438987-1.html 438987-1-ref.html
+== 438987-2a.html 438987-2-ref.html
+== 438987-2b.html 438987-2-ref.html
+== 438987-2c.html 438987-2-ref.html
+!= about:blank 438987-2-ref.html # check that backgrounds work at all 
 == 439004-1.html 439004-1-ref.html
 == 439910.html 439910-ref.html
 # == 448987.html 448987-ref.html  # Disabled for now - it needs privileges