[CSS Regions] Elements in a region should be assignable to a named flow

Source/core/dom/NodeRenderingContext.cpp

Index: Source/core/dom/NodeRenderingContext.cpp
diff --git a/Source/core/dom/NodeRenderingContext.cpp b/Source/core/dom/NodeRenderingContext.cpp
index 1533f0accef6fcc060669ea4d3e3c6c2d8793282..614f7ef458973619892366629cdded96ceb39b05 100644
--- a/Source/core/dom/NodeRenderingContext.cpp
+++ b/Source/core/dom/NodeRenderingContext.cpp
@@ -190,40 +190,65 @@ bool NodeRenderingContext::shouldCreateRenderer() const
     return true;
 }
 
-void NodeRenderingContext::moveToFlowThreadIfNeeded()
+bool NodeRenderingContext::shouldMoveToFlowThread() const
 {
     ASSERT(m_node->isElementNode());
-    ASSERT(m_style);
+    Element* element = toElement(m_node);
+
+    Document* doc = m_node->document();
+
     if (!RuntimeEnabledFeatures::cssRegionsEnabled())
-        return;
+        return false;
 
-    if (m_style->flowThread().isEmpty())
-        return;
+    if (doc->webkitIsFullScreen() && doc->webkitCurrentFullScreenElement() == m_node)
+        return false;
+
+    if (m_node->isInShadowTree())
+        return false;
 
     // As per http://dev.w3.org/csswg/css3-regions/#flow-into, pseudo-elements such as ::first-line, ::first-letter, ::before or ::after
     // cannot be directly collected into a named flow.
     if (m_node->isPseudoElement())
-        return;
-
-    // FIXME: Do not collect elements if they are in shadow tree.
-    if (m_node->isInShadowTree())
-        return;
-
-    Document* document = m_node->document();
-    if (document->webkitIsFullScreen() && document->webkitCurrentFullScreenElement() == m_node)
-        return;
+        return false;
 
-#if ENABLE(SVG)
     // Allow only svg root elements to be directly collected by a render flow thread.
     if (m_node->isSVGElement()
         && (!(m_node->hasTagName(SVGNames::svgTag) && m_node->parentNode() && !m_node->parentNode()->isSVGElement())))
+        return false;
+
+    if (!m_style)
+        m_style = element->styleForRenderer();
+    if (!m_style)
+        return false;
+
+    if (m_style->flowThread().isEmpty())
+        return false;
+
+    if (doc->renderView()->flowThreadController()->isContentNodeRegisteredWithAnyNamedFlow(m_node))
+        return false;
+
+    return true;
+}
+
+void NodeRenderingContext::moveToFlowThreadIfNeeded()
+{
+    if (!shouldMoveToFlowThread())
         return;
-#endif
 
-    m_flowThread = m_style->flowThread();
+    moveToFlowThread();
+}
+
+void NodeRenderingContext::moveToFlowThread()
+{
+    ASSERT(m_node->isElementNode());
+    ASSERT(shouldMoveToFlowThread());
+
+    if (!m_style)
+        m_style = toElement(m_node)->styleForRenderer();
+    ASSERT(m_style);
     ASSERT(m_node->document()->renderView());
     FlowThreadController* flowThreadController = m_node->document()->renderView()->flowThreadController();
-    m_parentFlowRenderer = flowThreadController->ensureRenderFlowThreadWithName(m_flowThread);
+    m_parentFlowRenderer = flowThreadController->ensureRenderFlowThreadWithName(m_style->flowThread());
     flowThreadController->registerNamedFlowContentNode(m_node, m_parentFlowRenderer);
 }
 
@@ -243,9 +268,12 @@ void NodeRenderingContext::createRendererForElementIfNeeded()
 
     Element* element = toElement(m_node);
 
+    moveToFlowThreadIfNeeded();

[Julien - ping for review, 2013/05/16 15:54:41]

There are 2 calls to moveToFlowThreadIfNeeded 8 lines apart which seems
unneeded.

Note that because of your change, we *will* do a style resolution for all
elements even if |shouldCreateRenderer| would return false. If we could keep the
current behavior of having the moveToFlowThreadIfNeeded called after calling
|shouldCreateRenderer| it would be much better but you probably want to have
moved the element before that call.

Btw, the fact that it needs to be called before shouldCreateRenderer could be
tested by adding some content under a <col> (which returns false to
canHaveChildren). Normally you shouldn't see the content but by hoisting it in
your named flow and it should be displayed.

[Mihai Maerean, 2013/05/17 15:11:25]

I have uploaded a new patch that incorporates the feedback to avoid doing a
style resolution for all elements even if |shouldCreateRenderer| would return
false.
Please review.

+
     if (!shouldCreateRenderer())
         return;
-    m_style = element->styleForRenderer();
+    if (!m_style)
+        m_style = element->styleForRenderer();
     ASSERT(m_style);
 
     moveToFlowThreadIfNeeded();