Collapse margins before child that uses legacy layout.

Collapse margins before child that uses legacy layout.

Due to the fact that legacy layout doesn't have a notion of MarginStrut
we can't calculate margins correctly for use cases that need to use both
layout engines.

This patch collapses margins and update the parent BFC if the next child
cannot be layed out using LayoutNG(ex. tables).

BUG=635619
TESTS=bunch of fixed tests in CSS2.1 and fast/block test suites

Review-Url: https://codereview.chromium.org/2835233004
Cr-Commit-Position: refs/heads/master@{#466820}
Committed: https://chromium.googlesource.com/chromium/src/+/da6a43e5fabf5aba0ee611e593d71809851727b0

[Gleb Lanbin, 2017-04-24 19:28:41]

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

[Gleb Lanbin, 2017-04-24 20:01:17]

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

[commit-bot: I haz the power, 2017-04-24 20:02:57]

Dry run: CQ is trying da patch.

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

[ikilpatrick, 2017-04-24 20:27:50]

lgtm

https://codereview.chromium.org/2835233004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2835233004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:347: //
TODO(glebl): Handle the case when a legacy block has border/padding.
This should be fine right? As these should always be new FCs?

[Gleb Lanbin, 2017-04-24 21:13:25]

Description was changed from

==========
Collapse margins before child that uses legacy layout.

Due to the fact that legacy layout doesn't have a notion of MarginStrut
we can't calculate margins correctly for use cases that need to use both
layout engines.

This patch collapses margins and update the parent BFC if the next child
cannot be layed out using LayoutNG(ex. tables). Please note it doesn't
support the use case with a legacy block child and border/padding.

BUG=635619
TESTS=bunch of fixed tests in CSS2.1 and fast/block test suites
==========

to

==========
Collapse margins before child that uses legacy layout.

Due to the fact that legacy layout doesn't have a notion of MarginStrut
we can't calculate margins correctly for use cases that need to use both
layout engines.

This patch collapses margins and update the parent BFC if the next child
cannot be layed out using LayoutNG(ex. tables).

BUG=635619
TESTS=bunch of fixed tests in CSS2.1 and fast/block test suites
==========

[Gleb Lanbin, 2017-04-24 21:15:34]

https://codereview.chromium.org/2835233004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2835233004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:347: //
TODO(glebl): Handle the case when a legacy block has border/padding.
On 2017/04/24 20:27:50, ikilpatrick wrote:
> This should be fine right? As these should always be new FCs?

yes, on a second thought, I think this TODO is not needed here.

[Gleb Lanbin, 2017-04-24 21:30:10]

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

[commit-bot: I haz the power, 2017-04-24 21:31:10]

Dry run: 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-04-24 23:16:45]

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

[commit-bot: I haz the power, 2017-04-24 23:16:46]

Dry run: This issue passed the CQ dry run.

[Gleb Lanbin, 2017-04-24 23:17:29]

The CQ bit was checked by glebl@chromium.org

[Gleb Lanbin, 2017-04-24 23:17:30]

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/2835233004/#ps40001
(title: "margin-collapse/100.html is still broken on Windows because of
LayoutInline")

[commit-bot: I haz the power, 2017-04-24 23:17:58]

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-04-24 23:23:44]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493075849770640,
"parent_rev": "7b90edb0875a7d2b662c43ba1f3ac9add9ca8c82", "commit_rev":
"da6a43e5fabf5aba0ee611e593d71809851727b0"}

[commit-bot: I haz the power, 2017-04-24 23:24:16]

Description was changed from

==========
Collapse margins before child that uses legacy layout.

Due to the fact that legacy layout doesn't have a notion of MarginStrut
we can't calculate margins correctly for use cases that need to use both
layout engines.

This patch collapses margins and update the parent BFC if the next child
cannot be layed out using LayoutNG(ex. tables).

BUG=635619
TESTS=bunch of fixed tests in CSS2.1 and fast/block test suites
==========

to

==========
Collapse margins before child that uses legacy layout.

Due to the fact that legacy layout doesn't have a notion of MarginStrut
we can't calculate margins correctly for use cases that need to use both
layout engines.

This patch collapses margins and update the parent BFC if the next child
cannot be layed out using LayoutNG(ex. tables).

BUG=635619
TESTS=bunch of fixed tests in CSS2.1 and fast/block test suites

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

[commit-bot: I haz the power, 2017-04-24 23:24:18]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/da6a43e5fabf5aba0ee611e593d7...