Correctly position abspos content inside relpos inlines [spv2]

Correctly position abspos content inside relpos inlines [spv2]

This patch fixes a class of bugs where absolutely positioned content
in a relative inline container would be positioned relative to the
origin instead of relative to the inline. This largely mirrors the logic
in PaintLayer::updateLayerPosition which used
LayoutInline::offsetForInFlowPositionedInline to position PaintLayers.

BUG=614257
Committed: https://crrev.com/0079320b1c8fdad99ef3cb31b92ef09da6ecd2e0
Cr-Commit-Position: refs/heads/master@{#396324}

[pdr., 2016-05-26 05:38:07]

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

[pdr., 2016-05-26 05:38:18]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org, trchen@chromium.org

[commit-bot: I haz the power, 2016-05-26 05:38:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2000423009/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2000423009/1

[pdr., 2016-05-26 05:53:18]

pdr@chromium.org changed reviewers:
- chrishtr@chromium.org

[pdr., 2016-05-26 05:53:19]

I'd like to split this fix into two patches because the special-case logic will
be a little involved (I think we'll need a second paintOffsetForAbsolutePosition
for static content).

Just FYI: In writing this I noticed the table row special-case logic in
deriveBorderBoxFromContainerContext. This looks very close to the table row
logic in PaintLayer::updateLayerPosition but slightly different--this may be a
bug.

[commit-bot: I haz the power, 2016-05-26 10:09:07]

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

[commit-bot: I haz the power, 2016-05-26 10:09:08]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[trchen, 2016-05-26 17:51:51]

trchen@chromium.org changed reviewers:
+ wangxianzhu@google.com

[trchen, 2016-05-26 17:51:51]

lgtm. However I'm not that familiar with block-direction flipping that kind of
stuff. +cc xianzhu for double check.

[chrishtr, 2016-05-26 17:53:26]

chrishtr@chromium.org changed reviewers:
+ chrishtr@chromium.org, szager@chromium.org, wangxianzhu@chromium.org
- wangxianzhu@google.com

[chrishtr, 2016-05-26 17:53:26]

Stefan could you also look?

[Xianzhu, 2016-05-26 18:02:27]

https://codereview.chromium.org/2000423009/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2000423009/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:350: }
It's better to use the same algorithm as in PaintInvalidationState
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
and LayoutBox::mapToVisualRectInAncestorSpace() about absolute-position under
relative inline.

Why do we need to care about statically-positioned absolute-position? Hasn't it
be already layouted at the proper place?

[pdr., 2016-05-26 21:30:30]

Thanks for the reviews, PTAL.

On 2016/05/26 at 18:02:27, wangxianzhu wrote:
>
https://codereview.chromium.org/2000423009/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
(right):
> 
>
https://codereview.chromium.org/2000423009/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:350: }
> It's better to use the same algorithm as in PaintInvalidationState
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
and LayoutBox::mapToVisualRectInAncestorSpace() about absolute-position under
relative inline.

The problem was that we didn't have access to both the relpos container and the
abspos child at the same time, so we couldn't call
offsetForInFlowPositionedInline directly without an expensive O(n) lookup of the
container. I went ahead and added a pointer to the relpos container and then
updated the code to use offsetForInFlowPositionedInline.

> 
> Why do we need to care about statically-positioned absolute-position? Hasn't
it be already layouted at the proper place?

"Static position" is being used in a way I was not familiar with, but it means
that the top/left/right/bottom values are "auto" which means that the abspos
container should be positioned as if it was inline and not abspos. There's some
special handling of negative offsets that must also be taken into account in
this situation. The comment in LayoutInline::offsetForInFlowPositionedInline
sort of explains this.

It's true that everything has already been laid out in the proper place, but we
don't have easy access to the x and y coordinates of an abspos object. In spv1
this is handled using the position of the PaintLayer which is calculated through
PaintLayer::updateLayerPosition; the code in this patch is the equivalent code
for spv2.

[pdr., 2016-05-26 21:31:04]

Description was changed from

==========
Correctly position abspos content inside relpos inlines [spv2]

This patch fixes a class of bugs where absolutely positioned content
in a relative inline container would be positioned relative to the
origin instead of relative to the inline. This largely mirrors the logic
in PaintLayer::updateLayerPosition which used
LayoutInline::offsetForInFlowPositionedInline to position PaintLayers.

A followup patch will handle a special case with static positioned
abspos content in relpos inlines which should let us mark all of
virtual/spv2/fast/block/positioning as passing.

BUG=614257
==========

to

==========
Correctly position abspos content inside relpos inlines [spv2]

This patch fixes a class of bugs where absolutely positioned content
in a relative inline container would be positioned relative to the
origin instead of relative to the inline. This largely mirrors the logic
in PaintLayer::updateLayerPosition which used
LayoutInline::offsetForInFlowPositionedInline to position PaintLayers.

BUG=614257
==========

[pdr., 2016-05-26 21:31:10]

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

[commit-bot: I haz the power, 2016-05-26 21:31:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2000423009/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2000423009/20001

[Xianzhu, 2016-05-26 21:40:55]

lgtm

https://codereview.chromium.org/2000423009/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2000423009/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:388: {
Nit: by putting '{' at the end of the previous line, and '}' after 'break' you
can save a level of indentation.

[pdr., 2016-05-26 21:45:51]

On 2016/05/26 at 21:40:55, wangxianzhu wrote:
> lgtm
> 
>
https://codereview.chromium.org/2000423009/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
(right):
> 
>
https://codereview.chromium.org/2000423009/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:388: {
> Nit: by putting '{' at the end of the previous line, and '}' after 'break' you
can save a level of indentation.

Done

[commit-bot: I haz the power, 2016-05-26 21:45:53]

Dry run: None

[pdr., 2016-05-26 21:47:23]

@trchen, could you take another look too?

[pdr., 2016-05-26 21:47:56]

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

[commit-bot: I haz the power, 2016-05-26 21:48:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2000423009/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2000423009/40001

[szager1, 2016-05-26 22:33:24]

lgtm

[trchen, 2016-05-26 23:47:08]

The CQ bit was unchecked by trchen@chromium.org

[trchen, 2016-05-26 23:47:14]

The CQ bit was checked by trchen@chromium.org

[trchen, 2016-05-26 23:47:15]

The patchset sent to the CQ was uploaded after l-g-t-m from trchen@chromium.org,
wangxianzhu@chromium.org
Link to the patchset: https://codereview.chromium.org/2000423009/#ps40001
(title: "Less indentation is more")

[commit-bot: I haz the power, 2016-05-26 23:47:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2000423009/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2000423009/40001

[commit-bot: I haz the power, 2016-05-27 00:07:05]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-27 00:08:49]

Description was changed from

==========
Correctly position abspos content inside relpos inlines [spv2]

This patch fixes a class of bugs where absolutely positioned content
in a relative inline container would be positioned relative to the
origin instead of relative to the inline. This largely mirrors the logic
in PaintLayer::updateLayerPosition which used
LayoutInline::offsetForInFlowPositionedInline to position PaintLayers.

BUG=614257
==========

to

==========
Correctly position abspos content inside relpos inlines [spv2]

This patch fixes a class of bugs where absolutely positioned content
in a relative inline container would be positioned relative to the
origin instead of relative to the inline. This largely mirrors the logic
in PaintLayer::updateLayerPosition which used
LayoutInline::offsetForInFlowPositionedInline to position PaintLayers.

BUG=614257

Committed: https://crrev.com/0079320b1c8fdad99ef3cb31b92ef09da6ecd2e0
Cr-Commit-Position: refs/heads/master@{#396324}
==========

[commit-bot: I haz the power, 2016-05-27 00:08:50]

Patchset 3 (id:??) landed as
https://crrev.com/0079320b1c8fdad99ef3cb31b92ef09da6ecd2e0
Cr-Commit-Position: refs/heads/master@{#396324}