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

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

With this commit, absolutely positioned dialogs are reparented by the
RenderView, so that their containing block is the initial containing block.
Particularly for dialogs that are laid out to be centered in the viewport, the
ICB makes more sense than the usual containing block for things like percent
lengths.  It also eliminates the need to translate absolute to local
coordinates when centering the dialog.

Instead of forcibly moving the renderer, I first tried to modify
containingBlock() to return RenderView, but this did not work, likely due to
bad interactions with RenderLayer (see the discussion in
https://codereview.chromium.org/13792002).

BUG=234072
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=150190

[falken, 2013-04-25 08:22:12]

Hi all,

Here's the reparenting by RenderView patch. Please take a look.

[Julien - ping for review, 2013-05-01 23:35:48]

FWIW I think we should try to keep NodeRenderingContext::parentRenderer close to
nextRenderer (that was one of the issue from a earlier <dialog> change).

https://codereview.chromium.org/14373010/diff/1/LayoutTests/fast/dom/HTMLDial...
File
LayoutTests/fast/dom/HTMLDialogElement/non-modal-dialog-containing-block.html
(right):

https://codereview.chromium.org/14373010/diff/1/LayoutTests/fast/dom/HTMLDial...
LayoutTests/fast/dom/HTMLDialogElement/non-modal-dialog-containing-block.html:36:
</html>
Do we have testing for a scrolled viewport? (would be nice to have it)

https://codereview.chromium.org/14373010/diff/1/Source/core/dom/NodeRendering...
File Source/core/dom/NodeRenderingContext.cpp (right):

https://codereview.chromium.org/14373010/diff/1/Source/core/dom/NodeRendering...
Source/core/dom/NodeRenderingContext.cpp:95: if (!element->isInTopLayer())
Why don't you just check isInTopLayer above instead of isReparentedByRenderView
as it should cover that?

https://codereview.chromium.org/14373010/diff/1/Source/core/html/HTMLDialogEl...
File Source/core/html/HTMLDialogElement.h (right):

https://codereview.chromium.org/14373010/diff/1/Source/core/html/HTMLDialogEl...
Source/core/html/HTMLDialogElement.h:49: virtual bool
isReparentedByRenderView(const RenderStyle*) const OVERRIDE;
Not a huge fan of the naming here. How about shouldBeReparentedUnderRenderView?

[falken, 2013-05-02 13:11:59]

Thanks for the review. I'll work on updating the patch next week (it's Golden
Week holidays in Japan now).

> FWIW I think we should try to keep NodeRenderingContext::parentRenderer close
to
> nextRenderer (that was one of the issue from a earlier <dialog> change).

You mean previousRenderer and nextRenderer, right?

https://codereview.chromium.org/14373010/diff/1/LayoutTests/fast/dom/HTMLDial...
File
LayoutTests/fast/dom/HTMLDialogElement/non-modal-dialog-containing-block.html
(right):

https://codereview.chromium.org/14373010/diff/1/LayoutTests/fast/dom/HTMLDial...
LayoutTests/fast/dom/HTMLDialogElement/non-modal-dialog-containing-block.html:36:
</html>
On 2013/05/01 23:35:48, Julien Chaffraix wrote:
> Do we have testing for a scrolled viewport? (would be nice to have it)

I'll see if I can add this.

https://codereview.chromium.org/14373010/diff/1/Source/core/dom/NodeRendering...
File Source/core/dom/NodeRenderingContext.cpp (right):

https://codereview.chromium.org/14373010/diff/1/Source/core/dom/NodeRendering...
Source/core/dom/NodeRenderingContext.cpp:95: if (!element->isInTopLayer())
On 2013/05/01 23:35:48, Julien Chaffraix wrote:
> Why don't you just check isInTopLayer above instead of
isReparentedByRenderView
> as it should cover that?

If the renderer is reparented, we don't want to get to the nextSibling part at
the end of the function. Typically 0 is returned except for the top layer
special case where the renderer has to be inserted in top layer order.

https://codereview.chromium.org/14373010/diff/1/Source/core/html/HTMLDialogEl...
File Source/core/html/HTMLDialogElement.h (right):

https://codereview.chromium.org/14373010/diff/1/Source/core/html/HTMLDialogEl...
Source/core/html/HTMLDialogElement.h:49: virtual bool
isReparentedByRenderView(const RenderStyle*) const OVERRIDE;
On 2013/05/01 23:35:48, Julien Chaffraix wrote:
> Not a huge fan of the naming here. How about
shouldBeReparentedUnderRenderView?

Sounds OK to me.

[Julien - ping for review, 2013-05-02 16:27:48]

>> FWIW I think we should try to keep NodeRenderingContext::parentRenderer close
to
>> nextRenderer (that was one of the issue from a earlier <dialog> change).

> You mean previousRenderer and nextRenderer, right?

No, the 2 functions that were patches to account for top layer were
parentRenderer and nextRenderer so those are the one to be kept in sync.

https://codereview.chromium.org/14373010/diff/1/Source/core/dom/NodeRendering...
File Source/core/dom/NodeRenderingContext.cpp (right):

https://codereview.chromium.org/14373010/diff/1/Source/core/dom/NodeRendering...
Source/core/dom/NodeRenderingContext.cpp:95: if (!element->isInTopLayer())
On 2013/05/02 13:11:59, falken wrote:
> On 2013/05/01 23:35:48, Julien Chaffraix wrote:
> > Why don't you just check isInTopLayer above instead of
> isReparentedByRenderView
> > as it should cover that?
> 
> If the renderer is reparented, we don't want to get to the nextSibling part at
> the end of the function. Typically 0 is returned except for the top layer
> special case where the renderer has to be inserted in top layer order.

OK, this code is definitely confusing I would say. If you want to bail out for
HTMLDialog elements not in the top layer, we should explicitly check for it
instead of indirectly checking that (by checking isReparentedByRenderView &&
!isInTopLayer).

[falken, 2013-05-08 08:13:29]

Hi Julien, please take another look.

I'm still not sure I understand what you mean about keeping parentRenderer and
nextRenderer close. I thought for a while about it, but don't see a positive way
to change either function to make it more similar to the other. It seems since
they do different things they'll have to be different.

https://codereview.chromium.org/14373010/diff/1/LayoutTests/fast/dom/HTMLDial...
File
LayoutTests/fast/dom/HTMLDialogElement/non-modal-dialog-containing-block.html
(right):

https://codereview.chromium.org/14373010/diff/1/LayoutTests/fast/dom/HTMLDial...
LayoutTests/fast/dom/HTMLDialogElement/non-modal-dialog-containing-block.html:36:
</html>
On 2013/05/02 13:11:59, falken wrote:
> On 2013/05/01 23:35:48, Julien Chaffraix wrote:
> > Do we have testing for a scrolled viewport? (would be nice to have it)
> 
> I'll see if I can add this.

Done.

https://codereview.chromium.org/14373010/diff/1/Source/core/dom/NodeRendering...
File Source/core/dom/NodeRenderingContext.cpp (right):

https://codereview.chromium.org/14373010/diff/1/Source/core/dom/NodeRendering...
Source/core/dom/NodeRenderingContext.cpp:95: if (!element->isInTopLayer())
On 2013/05/02 16:27:49, Julien Chaffraix wrote:
> On 2013/05/02 13:11:59, falken wrote:
> > On 2013/05/01 23:35:48, Julien Chaffraix wrote:
> > > Why don't you just check isInTopLayer above instead of
> > isReparentedByRenderView
> > > as it should cover that?
> > 
> > If the renderer is reparented, we don't want to get to the nextSibling part
at
> > the end of the function. Typically 0 is returned except for the top layer
> > special case where the renderer has to be inserted in top layer order.
> 
> OK, this code is definitely confusing I would say. If you want to bail out for
> HTMLDialog elements not in the top layer, we should explicitly check for it
> instead of indirectly checking that (by checking isReparentedByRenderView &&
> !isInTopLayer).

Done.

https://codereview.chromium.org/14373010/diff/1/Source/core/html/HTMLDialogEl...
File Source/core/html/HTMLDialogElement.h (right):

https://codereview.chromium.org/14373010/diff/1/Source/core/html/HTMLDialogEl...
Source/core/html/HTMLDialogElement.h:49: virtual bool
isReparentedByRenderView(const RenderStyle*) const OVERRIDE;
On 2013/05/02 13:11:59, falken wrote:
> On 2013/05/01 23:35:48, Julien Chaffraix wrote:
> > Not a huge fan of the naming here. How about
> shouldBeReparentedUnderRenderView?
> 
> Sounds OK to me.

Done.

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

The change looks fine but I really would like to see more explanations about the
nextRenderer change.

It's possible I am just wrong about not having any difference between
nextRenderer and parentRenderer but we should definitely be wary about them.

https://codereview.chromium.org/14373010/diff/15001/LayoutTests/fast/dom/HTML...
File
LayoutTests/fast/dom/HTMLDialogElement/top-layer-containing-block-scrolled-viewport.html
(right):

https://codereview.chromium.org/14373010/diff/15001/LayoutTests/fast/dom/HTML...
LayoutTests/fast/dom/HTMLDialogElement/top-layer-containing-block-scrolled-viewport.html:14:
viewport.
Nit: If you only want to check the scroll offset or position, it seems that a
ref-test is an overkill and you could get away with a checkLayout test (or even
just a plain manual one).

https://codereview.chromium.org/14373010/diff/15001/Source/core/dom/NodeRende...
File Source/core/dom/NodeRenderingContext.cpp (right):

https://codereview.chromium.org/14373010/diff/15001/Source/core/dom/NodeRende...
Source/core/dom/NodeRenderingContext.cpp:93: if (element &&
element->shouldBeReparentedUnderRenderView(m_style.get()) &&
!element->isInTopLayer())
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]

Thanks for the review.

https://codereview.chromium.org/14373010/diff/15001/LayoutTests/fast/dom/HTML...
File
LayoutTests/fast/dom/HTMLDialogElement/top-layer-containing-block-scrolled-viewport.html
(right):

https://codereview.chromium.org/14373010/diff/15001/LayoutTests/fast/dom/HTML...
LayoutTests/fast/dom/HTMLDialogElement/top-layer-containing-block-scrolled-viewport.html:14:
viewport.
On 2013/05/09 18:46:58, Julien Chaffraix wrote:
> Nit: If you only want to check the scroll offset or position, it seems that a
> ref-test is an overkill and you could get away with a checkLayout test (or
even
> just a plain manual one).

Oh, I wasn't aware of checkLayout. I think I'll leave these ones for now, and
keep it in mind for future tests.

https://codereview.chromium.org/14373010/diff/15001/Source/core/dom/NodeRende...
File Source/core/dom/NodeRenderingContext.cpp (right):

https://codereview.chromium.org/14373010/diff/15001/Source/core/dom/NodeRende...
Source/core/dom/NodeRenderingContext.cpp:93: if (element &&
element->shouldBeReparentedUnderRenderView(m_style.get()) &&
!element->isInTopLayer())
On 2013/05/09 18:46:58, Julien Chaffraix wrote:
> 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).

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.

[Julien - ping for review, 2013-05-10 21:04:56]

LGTM.

I would really like if the ref-tests you are adding could be converted to
checkLayout but that shouldn't block this change.

https://codereview.chromium.org/14373010/diff/26001/Source/core/html/HTMLDial...
File Source/core/html/HTMLDialogElement.cpp (right):

https://codereview.chromium.org/14373010/diff/26001/Source/core/html/HTMLDial...
Source/core/html/HTMLDialogElement.cpp:136: if (style && style->position() ==
AbsolutePosition)
This should probably be needsCenteredPositioning to match the rest of the code.

[falken, 2013-05-13 03:04:56]

Thanks for the review! I'll convert the reftests in the next patch.

https://codereview.chromium.org/14373010/diff/26001/Source/core/html/HTMLDial...
File Source/core/html/HTMLDialogElement.cpp (right):

https://codereview.chromium.org/14373010/diff/26001/Source/core/html/HTMLDial...
Source/core/html/HTMLDialogElement.cpp:136: if (style && style->position() ==
AbsolutePosition)
On 2013/05/10 21:04:56, Julien Chaffraix wrote:
> This should probably be needsCenteredPositioning to match the rest of the
code.

needsCenteredPositioning only returns true for auto 'top' and 'bottom'. I think
we want the containing block of all abspos dialogs to be the ICB, even if
top/bottom are not auto.

[commit-bot: I haz the power, 2013-05-13 03:11:50]

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

[commit-bot: I haz the power, 2013-05-13 04:02:28]

Change committed as 150190