Fix inline outline in a corner case

Fix inline outline in a corner case

In the corner case, an anoymous block contains an inline which has
outline and continuation. Previously when we calculated visual overflow
of the anonymous block during layout, the layout of continuation might
not be up-to-date causing inaccurate outline visual overflow.

Now re-add visual overflow of inline children of the anonymous block
when calculating parent's overflow which happens when layout of
continuation is clean.

This CL causes addOverflowFromInlineChildren() to be called twice in
each overflow recalc or layout cycle for each block with
containsInlineWithOutlineAndContinuation(), but this keeps the change
simplest for the rare case.

BUG=703403
Review-Url: https://codereview.chromium.org/2794663002
Cr-Commit-Position: refs/heads/master@{#461260}
Committed: https://chromium.googlesource.com/chromium/src/+/eefd3fc93296ee6b3ef9c5055d024573d8c7f63d

[Xianzhu, 2017-03-31 20:34:47]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[Xianzhu, 2017-03-31 20:35:11]

wangxianzhu@chromium.org changed reviewers:
+ trchen@chromium.org, wkorman@chromium.org

[commit-bot: I haz the power, 2017-03-31 20:35:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[trchen, 2017-03-31 21:13:08]

It sounds like a band-aid. Is it possible to mark the anonymous block's overflow
as dirty when the inline continuation finishes layout?

[commit-bot: I haz the power, 2017-03-31 21:58:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 21:58:25]

Dry run: This issue passed the CQ dry run.

[Xianzhu, 2017-03-31 21:59:13]

On 2017/03/31 21:13:08, trchen wrote:
> It sounds like a band-aid. Is it possible to mark the anonymous block's
overflow
> as dirty when the inline continuation finishes layout?

In that way we would need to schedule a visual update and wait for the next
document lifecycle update to get correct visual overflow. The dirty overflow
would exist until the next update, and might be painted and rasterized to be
visible before it gets cleaned. I think this violates lifecycle cleanness rules
by introducing dirty overflow after style and clean, and exposes the issue in a
bigger scope.

A cleaner way might be to move computeOverflow out of layout into a dedicated
tree walk after layout (recalcOverflowAfterLayout, like
recalcOverflowAfterStyleChange). However, it seems not worth it just for such a
rare case of focus ring.

[trchen, 2017-03-31 22:06:09]

On 2017/03/31 21:59:13, Xianzhu wrote:
> On 2017/03/31 21:13:08, trchen wrote:
> > It sounds like a band-aid. Is it possible to mark the anonymous block's
> overflow
> > as dirty when the inline continuation finishes layout?
> 
> In that way we would need to schedule a visual update and wait for the next
> document lifecycle update to get correct visual overflow. The dirty overflow
> would exist until the next update, and might be painted and rasterized to be
> visible before it gets cleaned. I think this violates lifecycle cleanness
rules
> by introducing dirty overflow after style and clean, and exposes the issue in
a
> bigger scope.

Ah, that's right. The dirty bits are only for style-induced (transforms in
particular) overflow quick path.

But why is the child anonymous block has a stale overflow after returning from
its layout?
Is its layout never invoked because its inline children are injected into it
directly? Then maybe we should invoke computeOverflow on the anonymous block
there?

> A cleaner way might be to move computeOverflow out of layout into a dedicated
> tree walk after layout (recalcOverflowAfterLayout, like
> recalcOverflowAfterStyleChange). However, it seems not worth it just for such
a
> rare case of focus ring.

[Xianzhu, 2017-03-31 22:30:47]

On 2017/03/31 22:06:09, trchen wrote:
> On 2017/03/31 21:59:13, Xianzhu wrote:
> > On 2017/03/31 21:13:08, trchen wrote:
> > > It sounds like a band-aid. Is it possible to mark the anonymous block's
> > overflow
> > > as dirty when the inline continuation finishes layout?
> > 
> > In that way we would need to schedule a visual update and wait for the next
> > document lifecycle update to get correct visual overflow. The dirty overflow
> > would exist until the next update, and might be painted and rasterized to be
> > visible before it gets cleaned. I think this violates lifecycle cleanness
> rules
> > by introducing dirty overflow after style and clean, and exposes the issue
in
> a
> > bigger scope.
> 
> Ah, that's right. The dirty bits are only for style-induced (transforms in
> particular) overflow quick path.
> 
> But why is the child anonymous block has a stale overflow after returning from
> its layout?
> Is its layout never invoked because its inline children are injected into it
> directly? Then maybe we should invoke computeOverflow on the anonymous block
> there?
> 

In short: the problem for a rare case: focus ring of inline element containing
block children. Inline focus ring needs to cover continuations. The problem
happens when computeOverflow() of the anonymous container of the inline happens
before layout of the continuations.

More details:

For the following case:
<div style="position: absolute">
  <span style="outline: auto">
    <div style="position: relative; width: 100px; height: 100px"></div>
  </span>
</div>

the layout tree is:

LayoutBlockFlow style="position: absolute"
  LayoutBlockFlow anonymous
    LayoutInline style="outline: auto"
  LayoutBlockFlow anonymous
    LayoutBlockFlow style="position: relative ..."
  LayoutBlockFlow anonymous
    LayoutInline style="outline: auto" (an empty continuation)

For historical reasons, the focus ring (and thus visual overflow) of layout
inline needs to cover the position:relative child in original DOM order.
However, during layout, computeOverflow() of the first anonymous container
happens before layout of the second anonymous container and its content. The
visual overflow is calculated based on the focus ring outline rects which are
collected using the pre-layout geometry of the second anonymous container.

This CL forces addOverflowFromInlineChild of the first anonymous container when
its overflow is added into its parent. (The result visual overflow might be
bigger if the pre-layout geometry of the second anonymous container was bigger,
but this won't cause incorrect rendering.)

The bug doesn't reproduce if the outer div doesn't have "position:absolute" or
the inner div doesn't have "position:relative". I'm not sure about the exact
reason. It seems that the first anonymous container's overflow is somehow
calculated again after layout of continuations.

[trchen, 2017-03-31 22:40:02]

On 2017/03/31 22:30:47, Xianzhu wrote:
> On 2017/03/31 22:06:09, trchen wrote:
> > On 2017/03/31 21:59:13, Xianzhu wrote:
> > > On 2017/03/31 21:13:08, trchen wrote:
> > > > It sounds like a band-aid. Is it possible to mark the anonymous block's
> > > overflow
> > > > as dirty when the inline continuation finishes layout?
> > > 
> > > In that way we would need to schedule a visual update and wait for the
next
> > > document lifecycle update to get correct visual overflow. The dirty
overflow
> > > would exist until the next update, and might be painted and rasterized to
be
> > > visible before it gets cleaned. I think this violates lifecycle cleanness
> > rules
> > > by introducing dirty overflow after style and clean, and exposes the issue
> in
> > a
> > > bigger scope.
> > 
> > Ah, that's right. The dirty bits are only for style-induced (transforms in
> > particular) overflow quick path.
> > 
> > But why is the child anonymous block has a stale overflow after returning
from
> > its layout?
> > Is its layout never invoked because its inline children are injected into it
> > directly? Then maybe we should invoke computeOverflow on the anonymous block
> > there?
> > 
> 
> In short: the problem for a rare case: focus ring of inline element containing
> block children. Inline focus ring needs to cover continuations. The problem
> happens when computeOverflow() of the anonymous container of the inline
happens
> before layout of the continuations.
> 
> More details:
> 
> For the following case:
> <div style="position: absolute">
>   <span style="outline: auto">
>     <div style="position: relative; width: 100px; height: 100px"></div>
>   </span>
> </div>
> 
> the layout tree is:
> 
> LayoutBlockFlow style="position: absolute"
>   LayoutBlockFlow anonymous
>     LayoutInline style="outline: auto"
>   LayoutBlockFlow anonymous
>     LayoutBlockFlow style="position: relative ..."
>   LayoutBlockFlow anonymous
>     LayoutInline style="outline: auto" (an empty continuation)
> 
> For historical reasons, the focus ring (and thus visual overflow) of layout
> inline needs to cover the position:relative child in original DOM order.
> However, during layout, computeOverflow() of the first anonymous container
> happens before layout of the second anonymous container and its content. The
> visual overflow is calculated based on the focus ring outline rects which are
> collected using the pre-layout geometry of the second anonymous container.
> 
> This CL forces addOverflowFromInlineChild of the first anonymous container
when
> its overflow is added into its parent. (The result visual overflow might be
> bigger if the pre-layout geometry of the second anonymous container was
bigger,
> but this won't cause incorrect rendering.)
> 
> The bug doesn't reproduce if the outer div doesn't have "position:absolute" or
> the inner div doesn't have "position:relative". I'm not sure about the exact
> reason. It seems that the first anonymous container's overflow is somehow
> calculated again after layout of continuations.

Ah, now it makes sense. Thanks for the thorough explanation! Yea, I can't think
of a cleaner way than your CL. lgtm.

[Xianzhu, 2017-03-31 22:44:23]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2017-03-31 22:45:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 22:50:17]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491000263851500,
"parent_rev": "eea21ff7adff4afee6a76b16977ca892b87ca5c7", "commit_rev":
"eefd3fc93296ee6b3ef9c5055d024573d8c7f63d"}

[commit-bot: I haz the power, 2017-03-31 22:51:06]

Description was changed from

==========
Fix inline outline in a corner case

In the corner case, an anoymous block contains an inline which has
outline and continuation. Previously when we calculated visual overflow
of the anonymous block during layout, the layout of continuation might
not be up-to-date causing inaccurate outline visual overflow.

Now re-add visual overflow of inline children of the anonymous block
when calculating parent's overflow which happens when layout of
continuation is clean.

This CL causes addOverflowFromInlineChildren() to be called twice in
each overflow recalc or layout cycle for each block with
containsInlineWithOutlineAndContinuation(), but this keeps the change
simplest for the rare case.

BUG=703403
==========

to

==========
Fix inline outline in a corner case

In the corner case, an anoymous block contains an inline which has
outline and continuation. Previously when we calculated visual overflow
of the anonymous block during layout, the layout of continuation might
not be up-to-date causing inaccurate outline visual overflow.

Now re-add visual overflow of inline children of the anonymous block
when calculating parent's overflow which happens when layout of
continuation is clean.

This CL causes addOverflowFromInlineChildren() to be called twice in
each overflow recalc or layout cycle for each block with
containsInlineWithOutlineAndContinuation(), but this keeps the change
simplest for the rare case.

BUG=703403

Review-Url: https://codereview.chromium.org/2794663002
Cr-Commit-Position: refs/heads/master@{#461260}
Committed:
https://chromium.googlesource.com/chromium/src/+/eefd3fc93296ee6b3ef9c5055d02...
==========

[commit-bot: I haz the power, 2017-03-31 22:51:08]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/eefd3fc93296ee6b3ef9c5055d02...