Issue 13792002: Fix <dialog> centering

Issue 13792002: Fix <dialog> centering (Closed)

Created:
7 years, 8 months ago by falken

Modified:
7 years, 8 months ago

Reviewers:
esprehn, eseidel, Julien - ping for review, ojan

CC:
blink-reviews, Nate Chapin, Alexis Menard, jchaffraix+rendering, abarth-chromium

Base URL:
svn://svn.chromium.org/blink/trunk

Visibility:
Public.

More Reviews

Description

Fix <dialog> centering This patch: 1) Forces a sync layout on dialog.show() and then computes the top value that would center the dialog. 2) Uses customStyleForRenderer to center the dialog. This satisfies the spec that dialog is centered only on dialog.show() and not re-centered on subsequent relayouts. Before this patch, <dialog> centering occurred during dialog's layout, which doesn't work because it incorrectly depends on dialog's ancestors having been already laid out. Furthermore, it recentered on a relayout. This patch just does vertical centering. It does not do horizontal centering and doesn't work well with writing-mode. BUG=232268 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=148615

Patch Set 1 #

Total comments: 3

Patch Set 2 : get working for relpos #

Total comments: 1

Patch Set 3 : new approach: adjust RenderStyle in attach #

Patch Set 4 : override top on style recalc, and don't make dialog's cb the icb #

Patch Set 5 : use hasCustomStyleCallbacks #

Total comments: 10

Patch Set 6 : review comments and try reparenting by RenderView again #

Patch Set 7 : just do centering in this patch, no reparenting logic #

Patch Set 8 : fix use of PassRefPtr #

Total comments: 12

Patch Set 9 : sync #

Patch Set 10 : review comments #

Created: 7 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+222 lines, -200 lines)			Patch
M	LayoutTests/fast/dom/HTMLDialogElement/non-anchored-dialog-positioning.html	View	1 2 3 4 5 6	3 chunks	+116 lines, -63 lines	0 comments	Download
M	LayoutTests/fast/dom/HTMLDialogElement/non-anchored-dialog-positioning-expected.txt	View	1 2 3 4 5 6	1 chunk	+47 lines, -3 lines	0 comments	Download
M	Source/core/core.gypi	View	1 2 3 4 5 6 7 8	1 chunk	+0 lines, -1 line	0 comments	Download
M	Source/core/html/HTMLDialogElement.h	View	1 2 3 4 5 6 7 8	1 chunk	+5 lines, -1 line	0 comments	Download
M	Source/core/html/HTMLDialogElement.cpp	View	1 2 3 4 5 6 7 8 9	6 chunks	+54 lines, -7 lines	0 comments	Download
D	Source/core/rendering/RenderDialog.h	View	1 2 3 4 5 6 7 8	1 chunk	+0 lines, -55 lines	0 comments	Download
D	Source/core/rendering/RenderDialog.cpp	View	1 2 3 4 5 6 7 8	1 chunk	+0 lines, -70 lines	0 comments	Download

Messages

Total messages: 21 (0 generated)

Expand Messages | Collapse Messages

falken

Hi Julien and Ojan, Back to dialog centering! Our approach #2, using static block position ...

7 years, 8 months ago (2013-04-08 13:07:35 UTC) #1

esprehn

https://codereview.chromium.org/13792002/diff/1/Source/WebCore/dom/NodeRenderingContext.cpp File Source/WebCore/dom/NodeRenderingContext.cpp (right): https://codereview.chromium.org/13792002/diff/1/Source/WebCore/dom/NodeRenderingContext.cpp#newcode157 Source/WebCore/dom/NodeRenderingContext.cpp:157: if (m_node->hasTagName(dialogTag)) { This is just for testing right?

7 years, 8 months ago (2013-04-09 02:24:31 UTC) #2

falken

7 years, 8 months ago (2013-04-09 02:30:26 UTC) #3

esprehn

On 2013/04/09 02:30:26, falken wrote: > https://codereview.chromium.org/13792002/diff/1/Source/WebCore/dom/NodeRenderingContext.cpp > File Source/WebCore/dom/NodeRenderingContext.cpp (right): > > https://codereview.chromium.org/13792002/diff/1/Source/WebCore/dom/NodeRenderingContext.cpp#newcode157 > ...

7 years, 8 months ago (2013-04-09 02:32:24 UTC) #4

falken

On 2013/04/09 02:32:24, Elliott Sprehn wrote: > On 2013/04/09 02:30:26, falken wrote: > > > ...

7 years, 8 months ago (2013-04-09 06:37:49 UTC) #5

esprehn

On 2013/04/09 06:37:49, falken wrote: > ... > Maybe dialog can just override containingBlock() to ...

7 years, 8 months ago (2013-04-09 06:42:27 UTC) #6

eseidel

Do we not already have other examples in the rendering tree of objects which reach ...

7 years, 8 months ago (2013-04-09 07:40:55 UTC) #7

falken

Maybe got it working for relpos! But it's still not ready for formal review. On ...

7 years, 8 months ago (2013-04-09 10:35:51 UTC) #8

eseidel

Positioned children bust out of their containing block, no? This is sorta like that except ...

7 years, 8 months ago (2013-04-09 19:59:36 UTC) #9

falken

On 2013/04/09 19:59:36, Eric Seidel (Google) wrote: > Positioned children bust out of their containing ...

7 years, 8 months ago (2013-04-10 08:19:24 UTC) #10

falken

Uploaded a new WIP patch that does the new approach, storing top on HTMLDialogElement and ...

7 years, 8 months ago (2013-04-11 10:14:47 UTC) #11

Uploaded a new WIP patch that does the new approach, storing top on
HTMLDialogElement and overriding RenderStyle's top on attach.

It's not working for something like this:

<div style="position: absolute; top: 100px;">
  <dialog>Hi</dialog>
</div>

Changing containingBlock() to return view() doesn't seem sufficient. I think we
may to do something with RenderLayer too so its parent layer is the root layer.
For:

In the above example, we end up with a dialog 100px away from the center. Dialog
gets set a top of 271px (which is the correct center value) but its RenderLayer
seems to be positioned at 371px. 

Here is the DumpRenderTree output:

layer at (0,0) size 800x600
  RenderView at (0,0) size 800x600 
layer at (0,0) size 800x8
  RenderBlock {HTML} at (0,0) size 800x8
    RenderBody {BODY} at (8,8) size 784x0
layer at (8,371) size 720x58
  RenderDialog {DIALOG} at (0,271) size 720x58 [bgcolor=#FFFFFF] [border: (3px
solid #000000)]
    RenderText {#text} at (19,19) size 88x19
      text run at (19,19) width 88: "It is my dialog."

And showRenderTree() and showLayerTree():

*RenderView 0x17bff7c59578              #document       0x17bff7820020
  RenderBlock 0x17bff7c3a738            HTML    0x17bff7eaa440
    RenderBody 0x17bff7c3a138           BODY    0x17bff7b1abd0
      RenderBlock (positioned) 0x17bff7c3a638   DIV     0x17bff7b1a650
STYLE=position: absolute; top: 100px; width: 90%
        RenderDialog 0x17bff77f0338     DIALOG  0x17bff7cb9b20
          RenderText 0x17bff791ecf8     #text   0x17bff78caa40 "It is my
dialog."

layer 0x17bff7848c38 at (0,0) size 800x600
  RenderView 0x17bff7c59578 at (0,0) size 800x600
 positive z-order list(1)
  layer 0x17bff78e74b8 at (0,0) size 800x8
    RenderBlock 0x17bff7c3a738 {HTML} at (0,0) size 800x8
      RenderBody 0x17bff7c3a138 {BODY} at (8,8) size 784x0
   positive z-order list(2)
    layer 0x17bff7c491b8 at (8,100) size 720x0
      RenderBlock (positioned) 0x17bff7c3a638 {DIV} at (8,100) size 720x0
    layer 0x17bff7848db8 at (8,371) size 720x58
      RenderDialog 0x17bff77f0338 {DIALOG} at (0,271) size 720x58
[bgcolor=#FFFFFF] [border: (3px solid #000000)] id="mydialog"
        RenderText 0x17bff791ecf8 {#text} at (19,19) size 88x19
          text run at (19,19) width 88: "It is my dialog."

Still investigating.

falken

I think it's ready for review! Some notes: - Changing containingBlock() to return RenderView for ...

7 years, 8 months ago (2013-04-12 08:20:47 UTC) #12

esprehn

On 2013/04/12 08:20:47, falken wrote: > I think it's ready for review! > > Some ...

7 years, 8 months ago (2013-04-12 08:49:12 UTC) #13

falken

I updated the patch to use hasCustomStyleCallbacks. On 2013/04/12 08:49:12, esprehn wrote: > On 2013/04/12 ...

7 years, 8 months ago (2013-04-12 10:17:29 UTC) #14

I updated the patch to use hasCustomStyleCallbacks.

On 2013/04/12 08:49:12, esprehn wrote:
> On 2013/04/12 08:20:47, falken wrote:
> > I think it's ready for review!
> > 
> > Some notes:
> > 
> > - Changing containingBlock() to return RenderView for dialog is not
sufficient
> > because RenderLayer doesn't seem to use the information correctly.
> > Layout positions the dialog correctly but the result is wrong because during
> > paint the RenderLayer's position is wrong. My hunch is we need
> > to modify methods like stackingContainer, enclosingPositionedAncestor,
> > enclosing*Layer* to account for containing block. It looks like a lot of
work.
> > So, I think to make dialog's cb the icb, we should have it be parented by
> > RenderView like we do for top layer. We can fix RenderLayer later.
> 
> So you're saying at some point we won't reparent it anymore?

Yes, if we can support that. My understanding is we don't like the DOM and
render tree to differ. Right now we have to do it because the codebase doesn't
handle containingBlock() returning an arbitrary renderer well.

> 
> > - My first patch made dialog parented by RenderView but Elliott mentioned
it's
> > gross to special case NodeRenderingContext for a specific element. If we
want
> > to make the cb the icb, is it better to have some flag on Element that its
> > renderer should be parented by RenderView (and generalize to CSS later)?
> 
> Yeah, I need to think about how to approach this more. I'd be inclined to say
> NodeRenderingTraversal learns about this new parentIsRenderView() callback and
> you get the slow path for dialog.
> 
> We should rename that shadow DOM bit, it means lots of things.

Let me give that a try. I think we want dialog's cb to be the icb after all.
It'd be too weird to have such different behavior for show() and showModal(). I
think Hixie meant to make this change as part of
https://www.w3.org/Bugs/Public/show_bug.cgi?id=20418 but it looks like he only
changed it for anchored positioning.

> 
> > - In the email, Ojan suggested overriding attach and styleWillChange to
apply
> > the static top position. I think it's too late in styleWillChange. It
> > seems fine to do everything in adjustRenderStyle.
> > 
> 
> You probably want DialogElement::didRecalcStyle for this and set the
> hasCustomStyleCallbacks flag. I don't think we want to do layout from inside
> adjustRenderStyle.

Done.

esprehn

Looks much better. This isn't optimal, but this feature is behind a flag so we ...

7 years, 8 months ago (2013-04-16 08:32:06 UTC) #15

Looks much better. This isn't optimal, but this feature is behind a flag so we
can just iterate on it.

https://codereview.chromium.org/13792002/diff/29001/Source/WebCore/html/HTMLD...
File Source/WebCore/html/HTMLDialogElement.cpp (right):

https://codereview.chromium.org/13792002/diff/29001/Source/WebCore/html/HTMLD...
Source/WebCore/html/HTMLDialogElement.cpp:72: if (style->position() ==
AbsolutePosition && style->top().isAuto() && style->bottom().isAuto()) {
Can you put this check in a static helper method? You check it in two places
(one with a ! before it)

https://codereview.chromium.org/13792002/diff/29001/Source/WebCore/html/HTMLD...
Source/WebCore/html/HTMLDialogElement.cpp:80: void
HTMLDialogElement::setupDefaultUnanchoredPosition()
I might rename this positionAndReattach(), it's not entirely clear a setup*()
method also destroys your renderer and must be called after addToTopLayer

https://codereview.chromium.org/13792002/diff/29001/Source/WebCore/html/HTMLD...
Source/WebCore/html/HTMLDialogElement.cpp:92: RenderBlock* block =
toRenderBlock(renderer());
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; } ?

https://codereview.chromium.org/13792002/diff/29001/Source/WebCore/html/HTMLD...
Source/WebCore/html/HTMLDialogElement.cpp:106: reattach();
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.

https://codereview.chromium.org/13792002/diff/29001/Source/WebCore/html/HTMLD...
Source/WebCore/html/HTMLDialogElement.cpp:114: setupDefaultUnanchoredPosition();
You can trivially make this assert by doing dialog.removeAttribute('open') and
then doing show() again. These checks don't look safe.

falken

Elliott thanks for the review! I've updated the patch based on your comments. I'm also ...

7 years, 8 months ago (2013-04-16 12:02:14 UTC) #16

Elliott thanks for the review! I've updated the patch based on your comments.

I'm also working on getting the dialog reparented by RenderView, as mentioned in
my last patch. It's back to the hacky "hasTagName(dialogTag)" check. I intend to
fix this to be based on a isParentedByRenderView() callback as you suggested.

I had to update some tests to account for dialog being parented by the icb
always, which is different behavior than before.

Now that I've uploaded this, I think I should split this into two patches: one
that does the centering and then one that does the reparenting. What do you
think?

https://codereview.chromium.org/13792002/diff/29001/Source/WebCore/html/HTMLD...
File Source/WebCore/html/HTMLDialogElement.cpp (right):

https://codereview.chromium.org/13792002/diff/29001/Source/WebCore/html/HTMLD...
Source/WebCore/html/HTMLDialogElement.cpp:72: if (style->position() ==
AbsolutePosition && style->top().isAuto() && style->bottom().isAuto()) {
On 2013/04/16 08:32:06, esprehn wrote:
> Can you put this check in a static helper method? You check it in two places
> (one with a ! before it)

Done.

https://codereview.chromium.org/13792002/diff/29001/Source/WebCore/html/HTMLD...
Source/WebCore/html/HTMLDialogElement.cpp:80: void
HTMLDialogElement::setupDefaultUnanchoredPosition()
On 2013/04/16 08:32:06, esprehn wrote:
> I might rename this positionAndReattach(), it's not entirely clear a setup*()
> method also destroys your renderer and must be called after addToTopLayer

Done.

https://codereview.chromium.org/13792002/diff/29001/Source/WebCore/html/HTMLD...
Source/WebCore/html/HTMLDialogElement.cpp:92: RenderBlock* block =
toRenderBlock(renderer());
On 2013/04/16 08:32:06, esprehn wrote:
> 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; } ?

Done.

https://codereview.chromium.org/13792002/diff/29001/Source/WebCore/html/HTMLD...
Source/WebCore/html/HTMLDialogElement.cpp:106: reattach();
On 2013/04/16 08:32:06, esprehn wrote:
> 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.

Done.

https://codereview.chromium.org/13792002/diff/29001/Source/WebCore/html/HTMLD...
Source/WebCore/html/HTMLDialogElement.cpp:114: setupDefaultUnanchoredPosition();
On 2013/04/16 08:32:06, esprehn wrote:
> You can trivially make this assert by doing dialog.removeAttribute('open') and
> then doing show() again. These checks don't look safe.

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

falken

Elliott, please take another look. I've removed the reparenting logic from this patch and will ...

7 years, 8 months ago (2013-04-17 04:33:28 UTC) #17

Julien - ping for review

LGTM with the following conditions: * fix what is pointed out here. * add a ...

7 years, 8 months ago (2013-04-17 22:12:25 UTC) #18

LGTM with the following conditions:
* fix what is pointed out here.
* add a FIXME or file an issue on reparenting.
* update this bug description to say that you don't properly handle horizontal
centering (or add testing for it but I would like to see the new test)

https://codereview.chromium.org/13792002/diff/45001/Source/WebCore/html/HTMLD...
File Source/WebCore/html/HTMLDialogElement.cpp (right):

https://codereview.chromium.org/13792002/diff/45001/Source/WebCore/html/HTMLD...
Source/WebCore/html/HTMLDialogElement.cpp:78:
style->setTop(Length(m_top.toInt(), WebCore::Fixed));
Tab pointed out that this should respond to 'writing-mode' so it should be
setLogicalTop (including some testing for that).

https://codereview.chromium.org/13792002/diff/45001/Source/WebCore/html/HTMLD...
Source/WebCore/html/HTMLDialogElement.cpp:91: RenderStyle* styleToUse =
box->style();
This variable is only used below so you could just remove it.

https://codereview.chromium.org/13792002/diff/45001/Source/WebCore/html/HTMLD...
Source/WebCore/html/HTMLDialogElement.cpp:103: FloatPoint localPoint =
box->containingBlock()->absoluteToLocal(absolutePoint);
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.

https://codereview.chromium.org/13792002/diff/45001/Source/WebCore/html/HTMLD...
Source/WebCore/html/HTMLDialogElement.cpp:105: m_top =
LayoutSize(localPoint.x(), localPoint.y()).height();
m_top = localPoint.y(); !!!

https://codereview.chromium.org/13792002/diff/45001/Source/WebCore/html/HTMLD...
Source/WebCore/html/HTMLDialogElement.cpp:106: m_topIsValid = true;
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

https://codereview.chromium.org/13792002/diff/45001/Source/WebCore/html/HTMLD...
Source/WebCore/html/HTMLDialogElement.cpp:117: positionAndReattach();
Is it normal that we also center on 'show'?

falken

Thanks for the review! On 2013/04/17 22:12:25, Julien Chaffraix wrote: > * add a FIXME ...

7 years, 8 months ago (2013-04-18 01:46:57 UTC) #19

Thanks for the review!

On 2013/04/17 22:12:25, Julien Chaffraix wrote:
> * add a FIXME or file an issue on reparenting.

I'll file an issue for this.

> * update this bug description to say that you don't properly handle horizontal
> centering (or add testing for it but I would like to see the new test)

Updated the bug description. I didn't do horizontal yet because the spec doesn't
say we should do horizontal centering, just vertical. I'm not sure why. Maybe
because the UA stylesheet already has margin: auto? I'll try to ask Tab or
Hixie.

https://codereview.chromium.org/13792002/diff/45001/Source/WebCore/html/HTMLD...
File Source/WebCore/html/HTMLDialogElement.cpp (right):

https://codereview.chromium.org/13792002/diff/45001/Source/WebCore/html/HTMLD...
Source/WebCore/html/HTMLDialogElement.cpp:78:
style->setTop(Length(m_top.toInt(), WebCore::Fixed));
On 2013/04/17 22:12:25, Julien Chaffraix wrote:
> Tab pointed out that this should respond to 'writing-mode' so it should be
> setLogicalTop (including some testing for that).

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).

https://codereview.chromium.org/13792002/diff/45001/Source/WebCore/html/HTMLD...
Source/WebCore/html/HTMLDialogElement.cpp:91: RenderStyle* styleToUse =
box->style();
On 2013/04/17 22:12:25, Julien Chaffraix wrote:
> This variable is only used below so you could just remove it.

Done.

https://codereview.chromium.org/13792002/diff/45001/Source/WebCore/html/HTMLD...
Source/WebCore/html/HTMLDialogElement.cpp:103: FloatPoint localPoint =
box->containingBlock()->absoluteToLocal(absolutePoint);
On 2013/04/17 22:12:25, Julien Chaffraix wrote:
> 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.

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

https://codereview.chromium.org/13792002/diff/45001/Source/WebCore/html/HTMLD...
Source/WebCore/html/HTMLDialogElement.cpp:105: m_top =
LayoutSize(localPoint.x(), localPoint.y()).height();
On 2013/04/17 22:12:25, Julien Chaffraix wrote:
> m_top = localPoint.y(); !!!

Done.

https://codereview.chromium.org/13792002/diff/45001/Source/WebCore/html/HTMLD...
Source/WebCore/html/HTMLDialogElement.cpp:106: m_topIsValid = true;
On 2013/04/17 22:12:25, Julien Chaffraix wrote:
> 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

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.

https://codereview.chromium.org/13792002/diff/45001/Source/WebCore/html/HTMLD...
Source/WebCore/html/HTMLDialogElement.cpp:117: positionAndReattach();
On 2013/04/17 22:12:25, Julien Chaffraix wrote:
> Is it normal that we also center on 'show'?

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

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/falken@chromium.org/13792002/36002

7 years, 8 months ago (2013-04-18 04:27:26 UTC) #20

Message was sent while issue was closed.

Change committed as 148615

Expand Messages | Collapse Messages