Make abspos <dialog>'s containing block be the ICB.

Source/core/dom/NodeRenderingContext.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
  * Copyright (C) 2011 Google Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ skipping 61 matching lines @@
 NodeRenderingContext::~NodeRenderingContext()
 {
 }
 
 static bool isRendererReparented(const RenderObject* renderer)
 {
     if (!renderer->node()->isElementNode())
         return false;
     if (renderer->style() && !renderer->style()->flowThread().isEmpty())
         return true;
-    if (toElement(renderer->node())->isInTopLayer())
+    if (toElement(renderer->node())->shouldBeReparentedUnderRenderView(renderer->style()))
         return true;
     return false;
 }
 
 RenderObject* NodeRenderingContext::nextRenderer() const
 {
     if (RenderObject* renderer = m_node->renderer())
         return renderer->nextSibling();
 
     Element* element = m_node->isElementNode() ? toElement(m_node) : 0;
+    if (element && element->shouldBeReparentedUnderRenderView(m_style.get()) && !element->isInTopLayer())

[Julien - ping for review, 2013/05/09 18:46:58]

I misled you here. I was thinking about checking element->isDialogElement() &&
!element->isInTopLayer() as it would totally explain the intent (probably with a
comment about why we ignore them)

I would love some explanations about why we want a <dialog> outside the top
layer to be always be appended last instead of being where its renderer should
be (ie before its next sibling's renderer).

[falken, 2013/05/10 05:50:25]

I see now how the code is confusing and probably didn't fully understand myself
until you asked. I've added a FIXME comment and explain in more detail below.

The problem with reparenting is that parentRenderer must return RenderView, but
what nextRenderer does is a bigger task. I think we want two things:
(1) Top layer renderers are inserted in top layer order. This allows the top
layer rendering to work: RenderLayer collects the top layer elements in order
and they are painted from bottom to top.
(2) Non-top-layer renderers are inserted in DOM tree order. This means they
would be painted in DOM tree order. I think this is not explicitly required by
the spec (as reparenting is not a spec concept) but seems like the expected
behavior.

The implementation already does (1). With this patch, we still do (1) correctly
but not (2). It's not completely obvious the right way to do (2). We must find
the renderer of the next reparented element in tree order. We can't just use
NodeRenderingTraversal since it doesn't know about reparenting. The correct
thing to do may be to make NodeRenderingTraversal reparenting-aware, and
traverse the tree until we find the next reparented element, get its renderer,
and return it.

If possible, I'd like to handle (2) in a subsequent patch. The accomplishment of
this patch is to make non-modal dialog's containing block the ICB; the only
issue is that the stacking order of them relative to each other may be random.

+        return 0;
     if (element && element->isInTopLayer()) {
         const Vector<RefPtr<Element> >& topLayerElements = element->document()->topLayerElements();
         size_t position = topLayerElements.find(element);
         ASSERT(position != notFound);
         for (size_t i = position + 1; i < topLayerElements.size(); ++i) {
             if (RenderObject* renderer = topLayerElements[i]->renderer())
                 return renderer;
         }
         return 0;
     }
@@ skipping 13 matching lines @@
     }
 
     return 0;
 }
 
 RenderObject* NodeRenderingContext::previousRenderer() const
 {
     if (RenderObject* renderer = m_node->renderer())
         return renderer->previousSibling();
 
-    // FIXME: This doesn't work correctly for things in the top layer that are
+    // FIXME: This doesn't work correctly for reparented elements that are
     // display: none. We'd need to duplicate the logic in nextRenderer, but since
     // nothing needs that yet just assert.
-    ASSERT(!m_node->isElementNode() || !toElement(m_node)->isInTopLayer());
+    ASSERT(!m_node->isElementNode() || !toElement(m_node)->shouldBeReparentedUnderRenderView(m_style.get()));
 
     if (m_parentFlowRenderer)
         return m_parentFlowRenderer->previousRendererForNode(m_node);
 
     // FIXME: We should have the same O(N^2) avoidance as nextRenderer does
     // however, when I tried adding it, several tests failed.
     for (Node* sibling = NodeRenderingTraversal::previousSibling(m_node); sibling; sibling = NodeRenderingTraversal::previousSibling(sibling)) {
         RenderObject* renderer = sibling->renderer();
         if (renderer && !isRendererReparented(renderer))
             return renderer;
     }
 
     return 0;
 }
 
 RenderObject* NodeRenderingContext::parentRenderer() const
 {
     if (RenderObject* renderer = m_node->renderer())
         return renderer->parent();
 
-    if (m_node->isElementNode() && toElement(m_node)->isInTopLayer()) {
-        // The parent renderer of top layer elements is the RenderView, but only
+    if (m_node->isElementNode() && toElement(m_node)->shouldBeReparentedUnderRenderView(m_style.get())) {
+        // The parent renderer of reparented elements is the RenderView, but only
         // if the normal parent would have had a renderer.
         // FIXME: This behavior isn't quite right as the spec for top layer
         // only talks about display: none ancestors so putting a <dialog> inside
         // an <optgroup> seems like it should still work even though this check
         // will prevent it.
         if (!m_renderingParent || !m_renderingParent->renderer())
             return 0;
         return m_node->document()->renderView();
     }
 
@@ skipping 144 matching lines @@
     newRenderer->setFlowThreadState(parentRenderer->flowThreadState());
 
     RenderObject* nextRenderer = this->nextRenderer();
     textNode->setRenderer(newRenderer);
     // Parent takes care of the animations, no need to call setAnimatableStyle.
     newRenderer->setStyle(m_style.release());
     parentRenderer->addChild(newRenderer, nextRenderer);
 }
 
 }