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..86d330e86ec5f17378b3d154928f6150f515fce8 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,53 @@ void HTMLDialogElement::close(ExceptionCode& ec)
     }
     setBooleanAttribute(openAttr, false);
     document()->removeFromTopLayer(this);
+    m_topIsValid = false;
+}
+
+static bool needsCenteredPositioning(const RenderStyle* style)
+{
+    return style->position() == AbsolutePosition && style->hasAutoTopAndBottom();
+}
+
+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 (needsCenteredPositioning(style.get()) && m_topIsValid)
+        style->setTop(Length(m_top.toInt(), WebCore::Fixed));

[Julien - ping for review, 2013/04/17 22:12:25]

Tab pointed out that this should respond to 'writing-mode' so it should be
setLogicalTop (including some testing for that).

[falken, 2013/04/18 01:46:57]

Yes, this patch doesn't handle writing-mode yet. There's a FIXME in
positionAndReattach for it. I don't think we can use setLogicalTop until that's
fixed or we'd be mixing physical and logical coordinates (m_top is computed with
physical heights).

+
+    return style.release();
+}
+
+void HTMLDialogElement::positionAndReattach()
+{
+    // Layout because we need to know our ancestors' positions and our own height.
+    document()->updateLayoutIgnorePendingStylesheets();
+
+    RenderBox* box = renderBox();
+    if (!box)
+        return;
+    RenderStyle* styleToUse = box->style();

[Julien - ping for review, 2013/04/17 22:12:25]

This variable is only used below so you could just remove it.

[falken, 2013/04/18 01:46:57]

Done.

+    if (!needsCenteredPositioning(styleToUse))
+        return;
+
+    // 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 (box->height() < visibleHeight)
+        absolutePoint.move(0, (visibleHeight - box->height()) / 2);
+    FloatPoint localPoint = box->containingBlock()->absoluteToLocal(absolutePoint);

[Julien - ping for review, 2013/04/17 22:12:25]

We discussed this with Elliot and Tab and think you should just reparent the
dialog renderer under the RenderView. You need to do all this computation
(including a walk up to the root of your tree) wouldn't be needed in this case.

Also because you don't account for orthogonal writing mode nor transforms in
absoluteToLocal, I am not convinced those 2 cases are handled properly.

Note that this shouldn't block this patch from landing.

[falken, 2013/04/18 01:46:57]

Thanks for discussing this!! I'll work on reparenting in a subsequent patch.

+
+    m_top = LayoutSize(localPoint.x(), localPoint.y()).height();

[Julien - ping for review, 2013/04/17 22:12:25]

m_top = localPoint.y(); !!!

[falken, 2013/04/18 01:46:57]

Done.

+    m_topIsValid = true;

[Julien - ping for review, 2013/04/17 22:12:25]

Currently you only handle m_top so it makes sense to name it here. Thinking one
patch down, you will also center horizontally so we should account for that in
the name. Alternative name: m_positionIsValid, (or better IMO) m_isShowingDialog

[falken, 2013/04/18 01:46:57]

Hmm.. let me handle this in the next patch. m_topIsValid can be false while the
dialog is showing if needsCenteredPositioning returned false in
positionAndReattach, so m_isShowingDialog is probably not what we want.

+
+    // FIXME: It's inefficient to reattach here. We could do better by mutating style directly and forcing another layout.
+    reattach();
 }
 
 void HTMLDialogElement::show()
@@ -61,6 +114,7 @@ void HTMLDialogElement::show()
     if (fastHasAttribute(openAttr))
         return;
     setBooleanAttribute(openAttr, true);
+    positionAndReattach();

[Julien - ping for review, 2013/04/17 22:12:25]

Is it normal that we also center on 'show'?

[falken, 2013/04/18 01:46:57]

I'm not sure what you mean... we want to center when dialog.show() is called,
yes. I probably don't understand the question.

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