A line that ends up naturally in the next column may need to propagate a strut.

A line that ends up naturally in the next column may need to propagate a strut.

Even if the line itself didn't need a strut, we may have to calculate and
propagate one to the block, or we risk violating orphan requirements or
breaking in the middle of the top border, padding or (in case it's a float)
margin.

R=leviw@chromium.org

Committed: https://crrev.com/a293493a93e9718f1a1f40208cca610512d0962d
Cr-Commit-Position: refs/heads/master@{#360798}

[leviw_travelin_and_unemployed, 2015-11-19 20:47:01]

LGTM.

https://codereview.chromium.org/1462913002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1462913002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:769: paginationStrut
+= blockFlow.marginBefore(); // Floats' margins do not collapse with page or
column boundaries.
CS
S!

https://codereview.chromium.org/1462913002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:828: // case it's a
float) margin, we may want to set a strut to the block, so that everything
I think you want "set a strut _on_ the block"

https://codereview.chromium.org/1462913002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:839:
totalLogicalOffset += marginBefore(); // Floats' margins do not collapse with
page or column boundaries.
A nit, but maybe it makes sense to have a small inline method
"marginBeforeIfFloating" that has the comment and you re-use (since you've
duplicated it)

[mstensho (USE GERRIT), 2015-11-20 09:49:34]

https://codereview.chromium.org/1462913002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1462913002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:769: paginationStrut
+= blockFlow.marginBefore(); // Floats' margins do not collapse with page or
column boundaries.
On 2015/11/19 20:47:01, leviw wrote:
> CS
> S!

+----+
|CSS |
|IS  |
|AWESФME
+----+

https://codereview.chromium.org/1462913002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:828: // case it's a
float) margin, we may want to set a strut to the block, so that everything
On 2015/11/19 20:47:01, leviw wrote:
> I think you want "set a strut _on_ the block"

Yeah, let's do that.

That was me wanting to write "may want to propagate a strut to the block". But
"set" is better in this case.

https://codereview.chromium.org/1462913002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:839:
totalLogicalOffset += marginBefore(); // Floats' margins do not collapse with
page or column boundaries.
On 2015/11/19 20:47:01, leviw wrote:
> A nit, but maybe it makes sense to have a small inline method
> "marginBeforeIfFloating" that has the comment and you re-use (since you've
> duplicated it)

Yeah, this has been bothering me. I too have been wanting to add a method for
this for some time, but I was afraid it would be considered too esoteric, and
that it was better to keep this mess locally. But since you're the second person
to suggest this to me (myself being the first one), I'll add a
marginBeforeIfFloating() method in an upcoming CL.

[mstensho (USE GERRIT), 2015-11-20 09:49:55]

The CQ bit was checked by mstensho@opera.com

[mstensho (USE GERRIT), 2015-11-20 09:49:55]

The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org
Link to the patchset: https://codereview.chromium.org/1462913002/#ps20001
(title: "Prepositionize responsibly.")

[commit-bot: I haz the power, 2015-11-20 09:50:09]

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

[commit-bot: I haz the power, 2015-11-20 10:58:24]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-11-20 10:59:12]

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