Fix <dialog> centering

Source/WebCore/html/HTMLDialogElement.cpp

Index: Source/WebCore/html/HTMLDialogElement.cpp
diff --git a/Source/WebCore/html/HTMLDialogElement.cpp b/Source/WebCore/html/HTMLDialogElement.cpp
index 3f32fa3cd223537a4fc0b09ea5a6d3bf26b88f55..12206db0b180ce306140bb264855d45883273d31 100644
--- a/Source/WebCore/html/HTMLDialogElement.cpp
+++ b/Source/WebCore/html/HTMLDialogElement.cpp
@@ -28,7 +28,10 @@
 #include "HTMLDialogElement.h"
 
 #include "ExceptionCode.h"
-#include "RenderDialog.h"
+#include "FrameView.h"
+#include "RenderBlock.h"
+#include "RenderStyle.h"
+#include "StyleResolver.h"
 
 namespace WebCore {
 
@@ -36,8 +39,11 @@ using namespace HTMLNames;
 
 HTMLDialogElement::HTMLDialogElement(const QualifiedName& tagName, Document* document)
     : HTMLElement(tagName, document)
+    , m_topIsValid(false)
+    , m_top(0)
 {
     ASSERT(hasTagName(dialogTag));
+    setHasCustomStyleCallbacks();
     ScriptWrappable::init(this);
 }
 
@@ -54,6 +60,50 @@ void HTMLDialogElement::close(ExceptionCode& ec)
     }
     setBooleanAttribute(openAttr, false);
     document()->removeFromTopLayer(this);
+    m_topIsValid = false;
+}
+
+PassRefPtr<RenderStyle> HTMLDialogElement::customStyleForRenderer()
+{
+    RefPtr<RenderStyle> originalStyle = document()->styleResolver()->styleForElement(this);
+    RefPtr<RenderStyle> style = RenderStyle::clone(originalStyle.get());
+
+    // Override top to remain centered after style recalcs.
+    if (style->position() == AbsolutePosition && style->top().isAuto() && style->bottom().isAuto()) {

[esprehn, 2013/04/16 08:32:06]

Can you put this check in a static helper method? You check it in two places
(one with a ! before it)

[falken, 2013/04/16 12:02:14]

Done.

+        if (m_topIsValid)
+            style->setTop(Length(m_top.toInt(), WebCore::Fixed));
+    }
+
+    return style.release();
+}
+
+void HTMLDialogElement::setupDefaultUnanchoredPosition()

[esprehn, 2013/04/16 08:32:06]

I might rename this positionAndReattach(), it's not entirely clear a setup*()
method also destroys your renderer and must be called after addToTopLayer

[falken, 2013/04/16 12:02:14]

Done.

+{
+    ASSERT(!m_topIsValid);
+
+    // Layout because we need to know our ancestors' positions and our own height.
+    document()->updateLayoutIgnorePendingStylesheets();
+
+    if (!renderer())
+        return;
+    RenderStyle* styleToUse = renderer()->style();
+    if (styleToUse->position() != AbsolutePosition || !styleToUse->top().isAuto() || !styleToUse->bottom().isAuto())
+        return;
+    RenderBlock* block = toRenderBlock(renderer());

[esprehn, 2013/04/16 08:32:06]

I'd use a renderBox() method instead and RenderBox* with a null check and early
return. Do we really require a RenderBlock? What if I did dialog { display:
table-row; } ?

[falken, 2013/04/16 12:02:14]

Done.

+
+    // Set up dialog's position to be safe-centered in the viewport.
+    // FIXME: Figure out what to do in vertical writing mode.
+    FrameView* frameView = document()->view();
+    int scrollTop = frameView->scrollOffset().height();
+    FloatPoint absolutePoint(0, scrollTop);
+    int visibleHeight = frameView->visibleContentRect(ScrollableArea::IncludeScrollbars).height();
+    if (block->height() < visibleHeight)
+        absolutePoint.move(0, (visibleHeight - block->height()) / 2);
+    FloatPoint localPoint = block->containingBlock()->absoluteToLocal(absolutePoint);
+
+    m_top = LayoutSize(localPoint.x(), localPoint.y()).height();
+    m_topIsValid = true;
+    reattach();

[esprehn, 2013/04/16 08:32:06]

You might add a FIXME here. We can instead mutate the RenderStyle directly here
and just call document()->updateLayoutIgnorePendingStylesheets() again and avoid
creating and destroying the dialog renderer again. Your current impl creates,
destroys and recreates the <dialog> renderer and all it's children when calling
show() which isn't super optimal. We can fix that later though.

[falken, 2013/04/16 12:02:14]

Done.

 }
 
 void HTMLDialogElement::show()
@@ -61,6 +111,7 @@ void HTMLDialogElement::show()
     if (fastHasAttribute(openAttr))
         return;
     setBooleanAttribute(openAttr, true);
+    setupDefaultUnanchoredPosition();

[esprehn, 2013/04/16 08:32:06]

You can trivially make this assert by doing dialog.removeAttribute('open') and
then doing show() again. These checks don't look safe.

[falken, 2013/04/16 12:02:14]

Good point. I removed the assert and added a test that the dialog is recentered
if you call show() twice that way.

 }
 
 void HTMLDialogElement::showModal(ExceptionCode& ec)
@@ -69,8 +120,9 @@ void HTMLDialogElement::showModal(ExceptionCode& ec)
         ec = INVALID_STATE_ERR;
         return;
     }
-    setBooleanAttribute(openAttr, true);
     document()->addToTopLayer(this);
+    setBooleanAttribute(openAttr, true);
+    setupDefaultUnanchoredPosition();
 }
 
 bool HTMLDialogElement::isPresentationAttribute(const QualifiedName& name) const
@@ -83,11 +135,6 @@ bool HTMLDialogElement::isPresentationAttribute(const QualifiedName& name) const
     return HTMLElement::isPresentationAttribute(name);
 }
 
-RenderObject* HTMLDialogElement::createRenderer(RenderArena* arena, RenderStyle*)
-{
-    return new (arena) RenderDialog(this);
-}
-
 }
 
 #endif