[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 aca7e5f678957806584ebd4a7f6f2f5bb89b0497..fb564c56c7320f6ed1714bfb6ed7c2d455bb0ae7 100644
--- a/Source/core/dom/NodeRenderingContext.cpp
+++ b/Source/core/dom/NodeRenderingContext.cpp
@@ -142,7 +142,7 @@ RenderObject* NodeRenderingContext::previousRenderer() const
     return 0;
 }
 
-RenderObject* NodeRenderingContext::parentRenderer() const

[Julien - ping for review, 2013/05/09 15:30:18]

That's a bad change on its own. Why would parentRenderer modify this?

[Mihai Maerean, 2013/05/09 16:25:55]

The bug was caused by a "race condition": it wasn't clear in the old code when
the NamedFlowThread should be created.
The fix consists in creating the NamedFlowThread when we encounter the 1st
element that needs to be flowed into it.
The parent renderer for such an element is its flow thread. If the flow thread
doesn't exist, it must be created.
This is why this method is not const.

[Julien - ping for review, 2013/05/10 00:13:05]

I will need to read deeper but it seems wrong that a simple getter is now
modifying the object. This is a definite sign that the change is probably going
in the wrong direction and that the architecture isn't accommodating regions as
it should.

+RenderObject* NodeRenderingContext::parentRenderer()
 {
     if (RenderObject* renderer = m_node->renderer())
         return renderer->parent();
@@ -162,10 +162,15 @@ RenderObject* NodeRenderingContext::parentRenderer() const
     if (m_parentFlowRenderer)
         return m_parentFlowRenderer;
 
+    if (m_node->isElementNode() && toElement(m_node)->moveToFlowThreadIsNeeded(m_style)) {
+        moveToFlowThread();
+        return m_parentFlowRenderer;
+    }
+
     return m_renderingParent ? m_renderingParent->renderer() : 0;
 }
 
-bool NodeRenderingContext::shouldCreateRenderer() const

[Julien - ping for review, 2013/05/09 15:30:18]

Ditto.

+bool NodeRenderingContext::shouldCreateRenderer()
 {
     if (!m_node->document()->shouldCreateRenderers())
         return false;
@@ -174,8 +179,16 @@ bool NodeRenderingContext::shouldCreateRenderer() const
     RenderObject* parentRenderer = this->parentRenderer();
     if (!parentRenderer)
         return false;
-    if (!parentRenderer->canHaveChildren())
-        return false;
+    if (!parentRenderer->canHaveChildren()) {
+        if (parentRenderer->canDOMChildrenHaveRenderParent()) {
+            // In a region, only the children that need to be in a flow thread should have a renderer.
+            bool shouldBeInNamedFlow = m_node->isElementNode() && toElement(m_node)->moveToFlowThreadIsNeeded(m_style);
+            if (!shouldBeInNamedFlow)
+                return false;
+        } else
+            return false;
+    }
+
     if (!m_renderingParent->childShouldCreateRenderer(*this))
         return false;
     return true;
@@ -184,37 +197,24 @@ bool NodeRenderingContext::shouldCreateRenderer() const
 void NodeRenderingContext::moveToFlowThreadIfNeeded()
 {
     ASSERT(m_node->isElementNode());
-    ASSERT(m_style);
-    if (!RuntimeEnabledFeatures::cssRegionsEnabled())
-        return;
-
-    if (m_style->flowThread().isEmpty())
-        return;
 
-    // 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())
+    if (!toElement(m_node)->moveToFlowThreadIsNeeded(m_style))
         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;
+    moveToFlowThread();
+}
 
-#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;
-#endif
+void NodeRenderingContext::moveToFlowThread()
+{
+    ASSERT(m_node->isElementNode());
+    ASSERT(toElement(m_node)->moveToFlowThreadIsNeeded(m_style));
 
-    m_flowThread = m_style->flowThread();
+    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);
 }
 
@@ -236,7 +236,8 @@ void NodeRenderingContext::createRendererForElementIfNeeded()
 
     if (!shouldCreateRenderer())
         return;
-    m_style = element->styleForRenderer();
+    if (!m_style)
+        m_style = element->styleForRenderer();
     ASSERT(m_style);
 
     moveToFlowThreadIfNeeded();