Do not collapse margins with padding/border b/w parent and first/last child

Do not collapse margins with padding/border b/w parent and first/last child

Do not collapse top/bottom margins if there is a padding or border between
first/last in-flow child and its parent.
W3C spec: https://www.w3.org/TR/CSS2/box.html#collapsing-margins

BUG=635619
Committed: https://crrev.com/3c1dea8b946e05f7a48371f8413a23c47acae009
Cr-Commit-Position: refs/heads/master@{#418685}

[Gleb Lanbin, 2016-09-14 18:20:55]

Patchset #1 (id:1) has been deleted

[Gleb Lanbin, 2016-09-14 18:22:44]

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

[ikilpatrick, 2016-09-14 18:45:24]

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

[ikilpatrick, 2016-09-14 18:45:26]

lgtm

https://codereview.chromium.org/2346473003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2346473003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:123: //
Collapse TOP margins if there is no padding or border between
s/TOP/BLOCK-START

https://codereview.chromium.org/2346473003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:124: //
parent(current child) and its first in-flow child.
insert space between "parent ("

https://codereview.chromium.org/2346473003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:131: //
Collapse BOTOM margins if
s/BOTTOM/BLOCK-END

https://codereview.chromium.org/2346473003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:132: // 1)
there is no padding or border between parent(current child) and its
insert space between "parent ("

https://codereview.chromium.org/2346473003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:132: // 1)
there is no padding or border between parent(current child) and its
This this change if box-sizing: border-box is applied? (I'm just curious, it
probably doesn't).

https://codereview.chromium.org/2346473003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h
(right):

https://codereview.chromium.org/2346473003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h:59: const
NGConstraintSpace& space);
astronomically tiny nit: can you make space the first argument?

(to be consistent with ng_length_utils , basically arguments that don't "change"
between subsequent calls go first).

[ikilpatrick, 2016-09-14 18:48:27]

also, this margin collapsing code makes me so happy compared to the old :)

(still lgtm )

[Gleb Lanbin, 2016-09-14 18:58:32]

thanks for the review!

https://codereview.chromium.org/2346473003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2346473003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:123: //
Collapse TOP margins if there is no padding or border between
On 2016/09/14 18:45:25, ikilpatrick wrote:
> s/TOP/BLOCK-START

Done.

https://codereview.chromium.org/2346473003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:124: //
parent(current child) and its first in-flow child.
On 2016/09/14 18:45:25, ikilpatrick wrote:
> insert space between "parent ("

Done.

https://codereview.chromium.org/2346473003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:131: //
Collapse BOTOM margins if
On 2016/09/14 18:45:25, ikilpatrick wrote:
> s/BOTTOM/BLOCK-END

Done.

https://codereview.chromium.org/2346473003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:132: // 1)
there is no padding or border between parent(current child) and its
On 2016/09/14 18:45:25, ikilpatrick wrote:
> insert space between "parent ("

Done.

https://codereview.chromium.org/2346473003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:132: // 1)
there is no padding or border between parent(current child) and its
On 2016/09/14 18:45:25, ikilpatrick wrote:
> This this change if box-sizing: border-box is applied? (I'm just curious, it
> probably doesn't).

Setting box-sizing doesn't have any affect on margin collapsing logic using the
existing code.

https://codereview.chromium.org/2346473003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h
(right):

https://codereview.chromium.org/2346473003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h:59: const
NGConstraintSpace& space);
On 2016/09/14 18:45:26, ikilpatrick wrote:
> astronomically tiny nit: can you make space the first argument?
> 
> (to be consistent with ng_length_utils , basically arguments that don't
"change"
> between subsequent calls go first).

done.
all 3 arguments are const references so the "mutable vs const" arguments order
is correct here. I changed the order to be consistent with ng_length_utils.
thanks.

[Gleb Lanbin, 2016-09-14 18:58:40]

The CQ bit was checked by glebl@chromium.org

[Gleb Lanbin, 2016-09-14 18:58:41]

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/2346473003/#ps40001
(title: "renamed TOP/BOTTOM to BLOCK-{START|END}")

[commit-bot: I haz the power, 2016-09-14 18:59:25]

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

[cbiesinger, 2016-09-14 19:12:18]

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

https://codereview.chromium.org/2346473003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:119:
computeBorderAndPaddingBlockStart(space, *current_child_->Style());
These functions were removed in https://codereview.chromium.org/2330153002/, try
this as a replacement:
   NGBoxStrut border_and_padding =
     computeBorders(style) + computePadding(constraintSpace, style);

[commit-bot: I haz the power, 2016-09-14 19:13:20]

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

[commit-bot: I haz the power, 2016-09-14 19:13:22]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Gleb Lanbin, 2016-09-14 19:41:27]

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

https://codereview.chromium.org/2346473003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:119:
computeBorderAndPaddingBlockStart(space, *current_child_->Style());
On 2016/09/14 19:12:18, cbiesinger wrote:
> These functions were removed in https://codereview.chromium.org/2330153002/,
try
> this as a replacement:
>    NGBoxStrut border_and_padding =
>      computeBorders(style) + computePadding(constraintSpace, style);

Done.

[Gleb Lanbin, 2016-09-14 19:41:35]

The CQ bit was checked by glebl@chromium.org

[Gleb Lanbin, 2016-09-14 19:41:35]

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/2346473003/#ps60001
(title: "synced to the head")

[commit-bot: I haz the power, 2016-09-14 19:42:20]

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, 2016-09-14 20:52:28]

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

[commit-bot: I haz the power, 2016-09-14 20:52:29]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Gleb Lanbin, 2016-09-14 20:54:35]

The CQ bit was checked by glebl@chromium.org

[commit-bot: I haz the power, 2016-09-14 20:55:24]

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, 2016-09-14 21:39:11]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-09-14 21:41:56]

Description was changed from

==========
Do not collapse margins with padding/border b/w parent and first/last child

Do not collapse top/bottom margins if there is a padding or border between
first/last in-flow child and its parent.
W3C spec: https://www.w3.org/TR/CSS2/box.html#collapsing-margins

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

to

==========
Do not collapse margins with padding/border b/w parent and first/last child

Do not collapse top/bottom margins if there is a padding or border between
first/last in-flow child and its parent.
W3C spec: https://www.w3.org/TR/CSS2/box.html#collapsing-margins

BUG=635619

Committed: https://crrev.com/3c1dea8b946e05f7a48371f8413a23c47acae009
Cr-Commit-Position: refs/heads/master@{#418685}
==========

[commit-bot: I haz the power, 2016-09-14 21:41:58]

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