[layoutng] Propagate fixed descendants of abs.

Propagate fixed descendants of abs.

Bug: fixed descendants of absolute blocks were not getting positioned.
This fix was a TODO that fell off the radar.

Example:
<div id="rel" style="position:relative;">
  <div id="abs" style="position:absolute;top:100px;">
    <p>absolute</p>
    <div class="fix" style="position:fixed;background-color:gray;"><p>fixed</p></div>
  </div>
</div>

The loop inside NGOutOfFlowLayoutPart::Run had to be modified from
collection iterator to an array, because collections can't be modified
while iterating.

I've tried testing it in vertical-lr, but there are upstream bugs 
that prevent this.

[ng_fixed_in_abs]

BUG=635619
Review-Url: https://codereview.chromium.org/2651393003
Cr-Commit-Position: refs/heads/master@{#448463}
Committed: https://chromium.googlesource.com/chromium/src/+/dc062b30eac5feb37dcdc824cdf114b2aa30d77e

[atotic, 2017-01-26 22:35:07]

Description was changed from

==========
Propagate fixed descendants of abs.

Bug: fixed descendants of absolute blocks were not getting positioned.
This fix was a TODO that fell off the radar.

Example:
<div id="rel" style="position:relative;">
  <div id="abs" style="position:absolute;top:100px;">
    <p>absolute</p>
    <div class="fix"
style="position:fixed;background-color:gray;"><p>fixed</p></div>
  </div>
</div>

BUG=635619
==========

to

==========
Propagate fixed descendants of abs.

Bug: fixed descendants of absolute blocks were not getting positioned.
This fix was a TODO that fell off the radar.

Example:
<div id="rel" style="position:relative;">
  <div id="abs" style="position:absolute;top:100px;">
    <p>absolute</p>
    <div class="fix"
style="position:fixed;background-color:gray;"><p>fixed</p></div>
  </div>
</div>

The loop inside NGOutOfFlowLayoutPart::Run had to be modified from
collection iterator to an array, because collections can't be modified
while iterating.

[ng_fixed_in_abs]

BUG=635619
==========

[atotic, 2017-01-26 22:35:56]

Description was changed from

==========
Propagate fixed descendants of abs.

Bug: fixed descendants of absolute blocks were not getting positioned.
This fix was a TODO that fell off the radar.

Example:
<div id="rel" style="position:relative;">
  <div id="abs" style="position:absolute;top:100px;">
    <p>absolute</p>
    <div class="fix"
style="position:fixed;background-color:gray;"><p>fixed</p></div>
  </div>
</div>

The loop inside NGOutOfFlowLayoutPart::Run had to be modified from
collection iterator to an array, because collections can't be modified
while iterating.

[ng_fixed_in_abs]

BUG=635619
==========

to

==========
Propagate fixed descendants of abs.

Bug: fixed descendants of absolute blocks were not getting positioned.
This fix was a TODO that fell off the radar.

Example:
<div id="rel" style="position:relative;">
  <div id="abs" style="position:absolute;top:100px;">
    <p>absolute</p>
    <div class="fix"
style="position:fixed;background-color:gray;"><p>fixed</p></div>
  </div>
</div>

The loop inside NGOutOfFlowLayoutPart::Run had to be modified from
collection iterator to an array, because collections can't be modified
while iterating.

I've tried testing it in vertical-lr, but there are upstream bugs 
that prevent this.

[ng_fixed_in_abs]

BUG=635619
==========

[atotic, 2017-01-26 22:35:57]

atotic@chromium.org changed reviewers:
+ ikilpatrick@chromium.org

[ikilpatrick, 2017-01-30 00:20:21]

this looks good, can you add a unit test with:

<div id=abs>
  <div id=fixed1>
    <div id=fixed2></div>
  </div>
</div>

?

[ikilpatrick, 2017-01-30 00:22:19]

(basically just something that will test multiple iterations of the outer loop).

[atotic, 2017-01-31 00:34:19]

On 2017/01/30 at 00:22:19, ikilpatrick wrote:
> (basically just something that will test multiple iterations of the outer
loop).

done.

ptal

[ikilpatrick, 2017-01-31 01:12:15]

https://codereview.chromium.org/2651393003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc
(right):

https://codereview.chromium.org/2651393003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2651393003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc:15:
NGOutOfFlowLayoutPartTest(){};
you'll need to flip on the experimental flag to hit the right codepath?

[ikilpatrick, 2017-01-31 01:14:12]

https://codereview.chromium.org/2651393003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc
(right):

https://codereview.chromium.org/2651393003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc:37:
Element* fixed2 = document().getElementById("fixed2");
also might be good just to check there are N fragments in the resulting fragment
here.

[atotic, 2017-01-31 01:15:41]

https://codereview.chromium.org/2651393003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc
(right):

https://codereview.chromium.org/2651393003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/01/31 at 01:12:15, ikilpatrick wrote:
> 2017

done

https://codereview.chromium.org/2651393003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc:15:
NGOutOfFlowLayoutPartTest(){};
On 2017/01/31 at 01:12:15, ikilpatrick wrote:
> you'll need to flip on the experimental flag to hit the right codepath?

huh? Please clarify.

[atotic, 2017-01-31 01:22:35]

https://codereview.chromium.org/2651393003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc
(right):

https://codereview.chromium.org/2651393003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc:37:
Element* fixed2 = document().getElementById("fixed2");
On 2017/01/31 at 01:14:12, ikilpatrick wrote:
> also might be good just to check there are N fragments in the resulting
fragment here.

There are no APIs to get fragment tree from Element. Are you suggesting
I synthesize a call to layout algorithm?

[Gleb Lanbin, 2017-01-31 19:19:34]

glebl@chromium.org changed reviewers:
+ glebl@chromium.org

[Gleb Lanbin, 2017-01-31 19:19:35]

https://codereview.chromium.org/2651393003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc
(right):

https://codereview.chromium.org/2651393003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc:15:
NGOutOfFlowLayoutPartTest(){};
On 2017/01/31 01:15:41, atotic wrote:
> On 2017/01/31 at 01:12:15, ikilpatrick wrote:
> > you'll need to flip on the experimental flag to hit the right codepath?
> 
> huh? Please clarify.

you need something like
NGOutOfFlowLayoutPartTest() {
     RuntimeEnabledFeatures::setLayoutNGEnabled(true);
  }
   ~NGOutOfFlowLayoutPartTest() {  // Optional
     RuntimeEnabledFeatures::setLayoutNGEnabled(false);
   }

https://codereview.chromium.org/2651393003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc:18:
TEST_F(NGOutOfFlowLayoutPartTest, FixedInsideAbs) {
please add a short comment that describes what use case this test covers.

https://codereview.chromium.org/2651393003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc:25:
"id='fixed1'"
you can fit this at the previous line and below (for id='fixed2')

https://codereview.chromium.org/2651393003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc:37:
Element* fixed2 = document().getElementById("fixed2");
On 2017/01/31 01:22:34, atotic wrote:
> On 2017/01/31 at 01:14:12, ikilpatrick wrote:
> > also might be good just to check there are N fragments in the resulting
> fragment here.
> 
> There are no APIs to get fragment tree from Element. Are you suggesting
> I synthesize a call to layout algorithm?

per discussion with Emil/Christian a Rendering test is an addition not a
replacement of our regular LayoutNG tests
See https://codereview.chromium.org/2560233003/ for more details
So we want to test LayoutNG fragment tree first and then we can optionally
verify that information got copied correctly to the old tree.
We have more examples in ng_block_layout_algorithm_test.cc

https://codereview.chromium.org/2651393003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc:38:
EXPECT_EQ(fixed1->offsetTop(), LayoutUnit(99));
please add a short comment that describes how we ended up with 99 here and
below.

https://codereview.chromium.org/2651393003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc:41:
}
}  // namespace
}  // namespace blink

[atotic, 2017-02-01 01:37:19]

ptal

https://codereview.chromium.org/2651393003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc
(right):

https://codereview.chromium.org/2651393003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc:15:
NGOutOfFlowLayoutPartTest(){};

done

https://codereview.chromium.org/2651393003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc:18:
TEST_F(NGOutOfFlowLayoutPartTest, FixedInsideAbs) {
On 2017/01/31 at 19:19:35, Gleb Lanbin wrote:
> please add a short comment that describes what use case this test covers.

done.

https://codereview.chromium.org/2651393003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc:25:
"id='fixed1'"
On 2017/01/31 at 19:19:35, Gleb Lanbin wrote:
> you can fit this at the previous line and below (for id='fixed2')

done

https://codereview.chromium.org/2651393003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc:37:
Element* fixed2 = document().getElementById("fixed2");
On 2017/01/31 at 19:19:35, Gleb Lanbin wrote:
> On 2017/01/31 01:22:34, atotic wrote:
> > On 2017/01/31 at 01:14:12, ikilpatrick wrote:
> > > also might be good just to check there are N fragments in the resulting
> > fragment here.
> > 
> So we want to test LayoutNG fragment tree first and then we can optionally
verify that information got copied correctly to the old tree.
> We have more examples in ng_block_layout_algorithm_test.cc

Fixed elements are currently directly positioned by LegacyLayout (#document
node).
I am unsure what fragments would you like to inspect?

My opinion is that what we is being tested right now is good enough for now.
We can revisit when #document starts using NGLayout.

-

https://codereview.chromium.org/2651393003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc:38:
EXPECT_EQ(fixed1->offsetTop(), LayoutUnit(99));
On 2017/01/31 at 19:19:35, Gleb Lanbin wrote:
> please add a short comment that describes how we ended up with 99 here and
below.

done

[atotic, 2017-02-03 17:09:13]

ping > 48hrs, ptal

[atotic, 2017-02-06 19:38:34]

Added a test for fragment count.

ptal

[ikilpatrick, 2017-02-06 19:46:03]

lgtm w nits.

https://codereview.chromium.org/2651393003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc
(right):

https://codereview.chromium.org/2651393003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc:64:
// Test whether oof fragments have been collected at NG->Legacy boundary
.nit

// Test whether the oof fragments have been collected at the NG->Legacy
boundary.

(the's and period)

https://codereview.chromium.org/2651393003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_out_of_flow_layout_part_test.cc:73:
// Test the final result
.nit period.

[atotic, 2017-02-06 19:55:26]

The CQ bit was checked by atotic@chromium.org

[atotic, 2017-02-06 19:55:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
ikilpatrick@chromium.org
Link to the patchset: https://codereview.chromium.org/2651393003/#ps100001
(title: "CR: final nits")

[commit-bot: I haz the power, 2017-02-06 19:56:10]

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-02-06 20:03:22]

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

[commit-bot: I haz the power, 2017-02-06 20:03:24]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[atotic, 2017-02-06 20:41:07]

The CQ bit was checked by atotic@chromium.org

[atotic, 2017-02-06 20:41:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
ikilpatrick@chromium.org
Link to the patchset: https://codereview.chromium.org/2651393003/#ps120001
(title: "remove raw string literal, does not work on mac")

[commit-bot: I haz the power, 2017-02-06 20:41:40]

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-02-06 20:52:09]

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

[commit-bot: I haz the power, 2017-02-06 20:52:10]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[atotic, 2017-02-06 21:49:40]

atotic@chromium.org changed reviewers:
+ eae@chromium.org

[atotic, 2017-02-06 21:50:36]

eae, can I get an lgtm? 

I need one from owners of WebKit/Source/core/BUILD.gn

[eae, 2017-02-06 21:52:54]

RS LGTM

[atotic, 2017-02-06 22:37:48]

The CQ bit was checked by atotic@chromium.org

[atotic, 2017-02-06 22:37:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
ikilpatrick@chromium.org, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2651393003/#ps140001
(title: "fixed1 is a reserved keyword on a mac")

[commit-bot: I haz the power, 2017-02-06 22:38:43]

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-02-07 00:27:13]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1486420668555080,
"parent_rev": "1c275a64ada3d8043b19aaa974aa36e92357aa81", "commit_rev":
"dc062b30eac5feb37dcdc824cdf114b2aa30d77e"}

[commit-bot: I haz the power, 2017-02-07 00:27:52]

Description was changed from

==========
Propagate fixed descendants of abs.

Bug: fixed descendants of absolute blocks were not getting positioned.
This fix was a TODO that fell off the radar.

Example:
<div id="rel" style="position:relative;">
  <div id="abs" style="position:absolute;top:100px;">
    <p>absolute</p>
    <div class="fix"
style="position:fixed;background-color:gray;"><p>fixed</p></div>
  </div>
</div>

The loop inside NGOutOfFlowLayoutPart::Run had to be modified from
collection iterator to an array, because collections can't be modified
while iterating.

I've tried testing it in vertical-lr, but there are upstream bugs 
that prevent this.

[ng_fixed_in_abs]

BUG=635619
==========

to

==========
Propagate fixed descendants of abs.

Bug: fixed descendants of absolute blocks were not getting positioned.
This fix was a TODO that fell off the radar.

Example:
<div id="rel" style="position:relative;">
  <div id="abs" style="position:absolute;top:100px;">
    <p>absolute</p>
    <div class="fix"
style="position:fixed;background-color:gray;"><p>fixed</p></div>
  </div>
</div>

The loop inside NGOutOfFlowLayoutPart::Run had to be modified from
collection iterator to an array, because collections can't be modified
while iterating.

I've tried testing it in vertical-lr, but there are upstream bugs 
that prevent this.

[ng_fixed_in_abs]

BUG=635619

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

[commit-bot: I haz the power, 2017-02-07 00:27:53]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/dc062b30eac5feb37dcdc824cdf1...