[LayoutNG] Fix 'vertical-align: text-top/-bottom' and atomic inline margins

[LayoutNG] Fix 'vertical-align: text-top/-bottom' and atomic inline margins

This patch fixes:
1. 'vertical-align: text-top' and 'text-bottom' to align to the text
   edge, not to the box edge.
2. Use the margin box of atomic inlines, not the border box.

It also:
3. Changes the geometry of NGLineBoxFragment from the text box to the
   line box to simplify. The geometry of RootInlineBox is unchanged.

Positions were fixed in relevant tests, but they were not rendered yet.

A false-positive test was discovered as its reference layout was
improved.

This patch fixes the block progression margins for atomic inline, but:
a. The block progression margin/border/padding for non-atomic inlines
   do not affect layout, only to overflows. Overflows is not implemented
   yet.
b. The inline progression margin for atomic inlines and margin/border/
   padding for non-atomic inlines is not implemented yet. It turned out
   that it's better to work on it after line breaker refactoring.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng

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

[kojii, 2017-05-07 09:34:43]

Description was changed from

==========
ngmargin

BUG=
==========

to

==========
[LayoutNG] Fixes in inline vertical positions

This patch fixes:
1. 'vertical-align: text-top' and 'text-bottom' to align to the text
   edge, not to the box edge.
2. Use the margin box of atomic inlines, not the border box.

It also:
3. Moves baseline position to NGBoxFragment, in preparation of not
   building RootInlineBox when NG has its own paint.
4. Changes the geometry of NGLineBoxFragment from the text box to the
   line box to simplify. The geometry of RootInlineBox is unchanged.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

[kojii, 2017-05-07 09:37:47]

Description was changed from

==========
[LayoutNG] Fixes in inline vertical positions

This patch fixes:
1. 'vertical-align: text-top' and 'text-bottom' to align to the text
   edge, not to the box edge.
2. Use the margin box of atomic inlines, not the border box.

It also:
3. Moves baseline position to NGBoxFragment, in preparation of not
   building RootInlineBox when NG has its own paint.
4. Changes the geometry of NGLineBoxFragment from the text box to the
   line box to simplify. The geometry of RootInlineBox is unchanged.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

to

==========
[LayoutNG] Fix 'vertical-align: text-top/-bottom' and atomic inlines

This patch fixes:
1. 'vertical-align: text-top' and 'text-bottom' to align to the text
   edge, not to the box edge.
2. Use the margin box of atomic inlines, not the border box.

It also:
3. Moves baseline position to NGBoxFragment, in preparation of not
   building RootInlineBox when NG has its own paint.
4. Changes the geometry of NGLineBoxFragment from the text box to the
   line box to simplify. The geometry of RootInlineBox is unchanged.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

[kojii, 2017-05-07 09:39:55]

Description was changed from

==========
[LayoutNG] Fix 'vertical-align: text-top/-bottom' and atomic inlines

This patch fixes:
1. 'vertical-align: text-top' and 'text-bottom' to align to the text
   edge, not to the box edge.
2. Use the margin box of atomic inlines, not the border box.

It also:
3. Moves baseline position to NGBoxFragment, in preparation of not
   building RootInlineBox when NG has its own paint.
4. Changes the geometry of NGLineBoxFragment from the text box to the
   line box to simplify. The geometry of RootInlineBox is unchanged.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

to

==========
[LayoutNG] Fixes in inline vertical positions

This patch fixes:
1. 'vertical-align: text-top' and 'text-bottom' to align to the text
   edge, not to the box edge.
2. Use the margin box of atomic inlines, not the border box.

It also:
3. Moves baseline position to NGBoxFragment, in preparation of not
   building RootInlineBox when NG has its own paint.
4. Changes the geometry of NGLineBoxFragment from the text box to the
   line box to simplify. The geometry of RootInlineBox is unchanged.

Positions were fixed in relevant tests, but they were not rendered yet,
probably due to the lack of ink-overflow support.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

[kojii, 2017-05-07 09:47:30]

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

[commit-bot: I haz the power, 2017-05-07 09:47:38]

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-05-07 11:02:04]

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

[commit-bot: I haz the power, 2017-05-07 11:02:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kojii, 2017-05-07 15:29:22]

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

[commit-bot: I haz the power, 2017-05-07 15:29:26]

Dry run: CQ is trying da patch.

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

[kojii, 2017-05-07 15:29:33]

Patchset #7 (id:120001) has been deleted

[kojii, 2017-05-07 15:45:52]

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

[commit-bot: I haz the power, 2017-05-07 15:46:03]

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-05-07 16:39:54]

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

[commit-bot: I haz the power, 2017-05-07 16:39:55]

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

[kojii, 2017-05-08 01:12:19]

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

[commit-bot: I haz the power, 2017-05-08 01:12:33]

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-05-08 02:28:53]

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

[commit-bot: I haz the power, 2017-05-08 02:28:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kojii, 2017-05-08 03:00:14]

Description was changed from

==========
[LayoutNG] Fixes in inline vertical positions

This patch fixes:
1. 'vertical-align: text-top' and 'text-bottom' to align to the text
   edge, not to the box edge.
2. Use the margin box of atomic inlines, not the border box.

It also:
3. Moves baseline position to NGBoxFragment, in preparation of not
   building RootInlineBox when NG has its own paint.
4. Changes the geometry of NGLineBoxFragment from the text box to the
   line box to simplify. The geometry of RootInlineBox is unchanged.

Positions were fixed in relevant tests, but they were not rendered yet,
probably due to the lack of ink-overflow support.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

to

==========
[LayoutNG] Fixes in inline vertical positions

This patch fixes:
1. 'vertical-align: text-top' and 'text-bottom' to align to the text
   edge, not to the box edge.
2. Use the margin box of atomic inlines, not the border box.

It also:
3. Moves baseline position to NGBoxFragment, in preparation of not
   building RootInlineBox when NG has its own paint.
4. Changes the geometry of NGLineBoxFragment from the text box to the
   line box to simplify. The geometry of RootInlineBox is unchanged.

Positions were fixed in relevant tests, but they were not rendered yet.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

[kojii, 2017-05-08 05:20:58]

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

[commit-bot: I haz the power, 2017-05-08 05:21:17]

Dry run: CQ is trying da patch.

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

[kojii, 2017-05-08 05:45:14]

Description was changed from

==========
[LayoutNG] Fixes in inline vertical positions

This patch fixes:
1. 'vertical-align: text-top' and 'text-bottom' to align to the text
   edge, not to the box edge.
2. Use the margin box of atomic inlines, not the border box.

It also:
3. Moves baseline position to NGBoxFragment, in preparation of not
   building RootInlineBox when NG has its own paint.
4. Changes the geometry of NGLineBoxFragment from the text box to the
   line box to simplify. The geometry of RootInlineBox is unchanged.

Positions were fixed in relevant tests, but they were not rendered yet.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

to

==========
[LayoutNG] Fixes in inline vertical positions

This patch fixes:
1. 'vertical-align: text-top' and 'text-bottom' to align to the text
   edge, not to the box edge.
2. Use the margin box of atomic inlines, not the border box.

It also:
3. Moves baseline position to NGBoxFragment, in preparation of not
   building RootInlineBox when NG has its own paint.
4. Changes the geometry of NGLineBoxFragment from the text box to the
   line box to simplify. The geometry of RootInlineBox is unchanged.

Positions were fixed in relevant tests, but they were not rendered yet.

A false-positive test was discovered by its reference layout was
improved.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

[kojii, 2017-05-08 07:02:24]

Description was changed from

==========
[LayoutNG] Fixes in inline vertical positions

This patch fixes:
1. 'vertical-align: text-top' and 'text-bottom' to align to the text
   edge, not to the box edge.
2. Use the margin box of atomic inlines, not the border box.

It also:
3. Moves baseline position to NGBoxFragment, in preparation of not
   building RootInlineBox when NG has its own paint.
4. Changes the geometry of NGLineBoxFragment from the text box to the
   line box to simplify. The geometry of RootInlineBox is unchanged.

Positions were fixed in relevant tests, but they were not rendered yet.

A false-positive test was discovered by its reference layout was
improved.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

to

==========
[LayoutNG] Fixes in inline vertical positions

This patch fixes:
1. 'vertical-align: text-top' and 'text-bottom' to align to the text
   edge, not to the box edge.
2. Use the margin box of atomic inlines, not the border box.

It also:
3. Moves baseline position to NGBoxFragment, in preparation of not
   building RootInlineBox when NG has its own paint.
4. Changes the geometry of NGLineBoxFragment from the text box to the
   line box to simplify. The geometry of RootInlineBox is unchanged.

Positions were fixed in relevant tests, but they were not rendered yet.

A false-positive test was discovered as its reference layout was
improved.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

[commit-bot: I haz the power, 2017-05-08 07:31:26]

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

[commit-bot: I haz the power, 2017-05-08 07:31:27]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_layout_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[kojii, 2017-05-08 08:04:35]

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

[commit-bot: I haz the power, 2017-05-08 08:05:09]

Dry run: CQ is trying da patch.

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

[kojii, 2017-05-08 09:37:25]

Description was changed from

==========
[LayoutNG] Fixes in inline vertical positions

This patch fixes:
1. 'vertical-align: text-top' and 'text-bottom' to align to the text
   edge, not to the box edge.
2. Use the margin box of atomic inlines, not the border box.

It also:
3. Moves baseline position to NGBoxFragment, in preparation of not
   building RootInlineBox when NG has its own paint.
4. Changes the geometry of NGLineBoxFragment from the text box to the
   line box to simplify. The geometry of RootInlineBox is unchanged.

Positions were fixed in relevant tests, but they were not rendered yet.

A false-positive test was discovered as its reference layout was
improved.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

to

==========
[LayoutNG] Fixes in inline vertical positions

This patch fixes:
1. 'vertical-align: text-top' and 'text-bottom' to align to the text
   edge, not to the box edge.
2. Use the margin box of atomic inlines, not the border box.

It also:
3. Moves BaselinePosition() to NGBoxFragment, in preparation of not
   building RootInlineBox when NG has its own paint.
4. Changes the geometry of NGLineBoxFragment from the text box to the
   line box to simplify. The geometry of RootInlineBox is unchanged.

Positions were fixed in relevant tests, but they were not rendered yet.

A false-positive test was discovered as its reference layout was
improved.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

[commit-bot: I haz the power, 2017-05-08 10:53:53]

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

[commit-bot: I haz the power, 2017-05-08 10:53:54]

Dry run: This issue passed the CQ dry run.

[kojii, 2017-05-08 13:53:18]

Description was changed from

==========
[LayoutNG] Fixes in inline vertical positions

This patch fixes:
1. 'vertical-align: text-top' and 'text-bottom' to align to the text
   edge, not to the box edge.
2. Use the margin box of atomic inlines, not the border box.

It also:
3. Moves BaselinePosition() to NGBoxFragment, in preparation of not
   building RootInlineBox when NG has its own paint.
4. Changes the geometry of NGLineBoxFragment from the text box to the
   line box to simplify. The geometry of RootInlineBox is unchanged.

Positions were fixed in relevant tests, but they were not rendered yet.

A false-positive test was discovered as its reference layout was
improved.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

to

==========
[LayoutNG] Fix 'vertical-align: text-top/-bottom' and atomic inline margins

This patch fixes:
1. 'vertical-align: text-top' and 'text-bottom' to align to the text
   edge, not to the box edge.
2. Use the margin box of atomic inlines, not the border box.

It also:
3. Moves BaselinePosition() to NGBoxFragment, in preparation of not
   building RootInlineBox when NG has its own paint.
4. Changes the geometry of NGLineBoxFragment from the text box to the
   line box to simplify. The geometry of RootInlineBox is unchanged.

Positions were fixed in relevant tests, but they were not rendered yet.

A false-positive test was discovered as its reference layout was
improved.

This patch fixes the block progression margins for atomic inline, but:
a. The block progression margin/border/padding for non-atomic inlines
   do not affect layout, only to overflows. Overflows is not implemented
   yet.
b. The inline progression margin for atomic inlines and
   margin/border/padding for non-atomic inlines is not implemented yet.
   It turned out that it's better to work on it after line breaker
   refactoring.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

[kojii, 2017-05-08 13:53:47]

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

[kojii, 2017-05-08 13:53:48]

PTAL.

[kojii, 2017-05-08 13:54:30]

Description was changed from

==========
[LayoutNG] Fix 'vertical-align: text-top/-bottom' and atomic inline margins

This patch fixes:
1. 'vertical-align: text-top' and 'text-bottom' to align to the text
   edge, not to the box edge.
2. Use the margin box of atomic inlines, not the border box.

It also:
3. Moves BaselinePosition() to NGBoxFragment, in preparation of not
   building RootInlineBox when NG has its own paint.
4. Changes the geometry of NGLineBoxFragment from the text box to the
   line box to simplify. The geometry of RootInlineBox is unchanged.

Positions were fixed in relevant tests, but they were not rendered yet.

A false-positive test was discovered as its reference layout was
improved.

This patch fixes the block progression margins for atomic inline, but:
a. The block progression margin/border/padding for non-atomic inlines
   do not affect layout, only to overflows. Overflows is not implemented
   yet.
b. The inline progression margin for atomic inlines and
   margin/border/padding for non-atomic inlines is not implemented yet.
   It turned out that it's better to work on it after line breaker
   refactoring.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

to

==========
[LayoutNG] Fix 'vertical-align: text-top/-bottom' and atomic inline margins

This patch fixes:
1. 'vertical-align: text-top' and 'text-bottom' to align to the text
   edge, not to the box edge.
2. Use the margin box of atomic inlines, not the border box.

It also:
3. Moves BaselinePosition() to NGBoxFragment, in preparation of not
   building RootInlineBox when NG has its own paint.
4. Changes the geometry of NGLineBoxFragment from the text box to the
   line box to simplify. The geometry of RootInlineBox is unchanged.

Positions were fixed in relevant tests, but they were not rendered yet.

A false-positive test was discovered as its reference layout was
improved.

This patch fixes the block progression margins for atomic inline, but:
a. The block progression margin/border/padding for non-atomic inlines
   do not affect layout, only to overflows. Overflows is not implemented
   yet.
b. The inline progression margin for atomic inlines and margin/border/
   padding for non-atomic inlines is not implemented yet. It turned out
   that it's better to work on it after line breaker refactoring.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

[eae, 2017-05-08 15:43:28]

This is great, thanks for working on this!

https://codereview.chromium.org/2845913002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/inline/ng_inline_box_state.cc
(right):

https://codereview.chromium.org/2845913002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/inline/ng_inline_box_state.cc:42:
baseline_type_ = baseline_type;
Please pass the baseline as an argument or use a context object instead of
storing it on the class instance. Using member variables for some of the local
state makes the code very hard to read and debug.

https://codereview.chromium.org/2845913002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.cc
(right):

https://codereview.chromium.org/2845913002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.cc:530:
const ComputedStyle& style = *item.Style();
Mixing references and pointers like this makes me uneasy.

item.Style() is *not* guaranteed to be set and can be null.
At the very least please add a DCHECK and try to avoid mixing references and
pointers where possible.

https://codereview.chromium.org/2845913002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_box_fragment.cc (right):

https://codereview.chromium.org/2845913002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_box_fragment.cc:43: offset -=
BlockOffset();
Why are we subtracting the block offset here?

[kojii, 2017-05-08 17:57:25]

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

[commit-bot: I haz the power, 2017-05-08 17:58:14]

Dry run: CQ is trying da patch.

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

[kojii, 2017-05-08 19:22:19]

All done, with one reply below.

Win bots are failing to compile but looks like an bot issue.

https://codereview.chromium.org/2845913002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_box_fragment.cc (right):

https://codereview.chromium.org/2845913002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_box_fragment.cc:43: offset -=
BlockOffset();
On 2017/05/08 at 15:43:28, eae wrote:
> Why are we subtracting the block offset here?

Fixed (removed), this was because I thought offsets are relative to ICB, thanks
to Christian to correct me.

[commit-bot: I haz the power, 2017-05-08 19:35:07]

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

[commit-bot: I haz the power, 2017-05-08 19:35:08]

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

[ikilpatrick, 2017-05-08 22:47:05]

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

[ikilpatrick, 2017-05-08 22:47:06]

https://codereview.chromium.org/2845913002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_box_fragment.cc (right):

https://codereview.chromium.org/2845913002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_box_fragment.cc:45: // RunOldLayout.
if we aren't using a line_box we should only are about the primary alignment
baseline?

[kojii, 2017-05-09 01:23:34]

https://codereview.chromium.org/2845913002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_box_fragment.cc (right):

https://codereview.chromium.org/2845913002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_box_fragment.cc:45: // RunOldLayout.
On 2017/05/08 at 22:47:06, ikilpatrick wrote:
> if we aren't using a line_box we should only are about the primary alignment
baseline?

There are some classes that override LayoutBoxModel::BaselinePosition; form
controls are RunOldLayout. Table, Grid, FlexBox, and ListMarker may need to
handle in NG. Should we create a subclass of NGPhsicalBoxFragment that has
custom baseline position? Add baseline position to every NGPhysicalBoxFragment?
I haven't determined yet.

[kojii, 2017-05-09 01:35:27]

https://codereview.chromium.org/2845913002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_box_fragment.cc (right):

https://codereview.chromium.org/2845913002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_box_fragment.cc:45: // RunOldLayout.
and forgot MultiCol, this maybe the first one who needs it? LayoutMultiCol does
not override, but I think it should be, need to double-check the spec.

[ikilpatrick, 2017-05-09 02:18:42]

https://codereview.chromium.org/2845913002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_box_fragment.cc (right):

https://codereview.chromium.org/2845913002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_box_fragment.cc:45: // RunOldLayout.
On 2017/05/09 01:35:27, kojii wrote:
> and forgot MultiCol, this maybe the first one who needs it? LayoutMultiCol
does
> not override, but I think it should be, need to double-check the spec.

Right, I think we actually want each fragment or layout_result to have a member
which is its baseline position. Otherwise w/ caching we'd have to perform a tree
walk to do this correctly.

If it is needed for painting it should be on the fragment, otherwise it should
be on the layout_result.

[kojii, 2017-05-09 02:27:11]

On 2017/05/09 at 02:18:42, ikilpatrick wrote:
> 
> Right, I think we actually want each fragment or layout_result to have a
member which is its baseline position. Otherwise w/ caching we'd have to perform
a tree walk to do this correctly.
> 
> If it is needed for painting it should be on the fragment, otherwise it should
be on the layout_result.

Thanks, sounds good to me. Can this be in separate CL?

I actually wonder, this is not really required for supporting margins of atomic
inline, don't remember why I put this into one CL. Should this be taken out to a
separate CL that does your suggestion?

[kojii, 2017-05-09 02:33:23]

Oh, wait , it's a little more complicated. Parents can request baselines of
different yours than the one used to layout. I'll think a little more.

[kojii, 2017-05-09 02:34:25]

s/yours/types/

[kojii, 2017-05-09 03:02:37]

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

[commit-bot: I haz the power, 2017-05-09 03:04:43]

Dry run: CQ is trying da patch.

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

[kojii, 2017-05-09 03:07:59]

Description was changed from

==========
[LayoutNG] Fix 'vertical-align: text-top/-bottom' and atomic inline margins

This patch fixes:
1. 'vertical-align: text-top' and 'text-bottom' to align to the text
   edge, not to the box edge.
2. Use the margin box of atomic inlines, not the border box.

It also:
3. Moves BaselinePosition() to NGBoxFragment, in preparation of not
   building RootInlineBox when NG has its own paint.
4. Changes the geometry of NGLineBoxFragment from the text box to the
   line box to simplify. The geometry of RootInlineBox is unchanged.

Positions were fixed in relevant tests, but they were not rendered yet.

A false-positive test was discovered as its reference layout was
improved.

This patch fixes the block progression margins for atomic inline, but:
a. The block progression margin/border/padding for non-atomic inlines
   do not affect layout, only to overflows. Overflows is not implemented
   yet.
b. The inline progression margin for atomic inlines and margin/border/
   padding for non-atomic inlines is not implemented yet. It turned out
   that it's better to work on it after line breaker refactoring.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

to

==========
[LayoutNG] Fix 'vertical-align: text-top/-bottom' and atomic inline margins

This patch fixes:
1. 'vertical-align: text-top' and 'text-bottom' to align to the text
   edge, not to the box edge.
2. Use the margin box of atomic inlines, not the border box.

It also:
3. Changes the geometry of NGLineBoxFragment from the text box to the
   line box to simplify. The geometry of RootInlineBox is unchanged.

Positions were fixed in relevant tests, but they were not rendered yet.

A false-positive test was discovered as its reference layout was
improved.

This patch fixes the block progression margins for atomic inline, but:
a. The block progression margin/border/padding for non-atomic inlines
   do not affect layout, only to overflows. Overflows is not implemented
   yet.
b. The inline progression margin for atomic inlines and margin/border/
   padding for non-atomic inlines is not implemented yet. It turned out
   that it's better to work on it after line breaker refactoring.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

[kojii, 2017-05-09 03:10:58]

Split to https://codereview.chromium.org/2867293002 and updated description.

Sorry for putting multiple things into one CL.

[commit-bot: I haz the power, 2017-05-09 05:15:27]

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

[commit-bot: I haz the power, 2017-05-09 05:15:28]

Dry run: This issue passed the CQ dry run.

[eae, 2017-05-09 15:19:13]

LGTM

[kojii, 2017-05-10 04:13:15]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2017-05-10 04:14:04]

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-05-10 06:43:15]

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

[commit-bot: I haz the power, 2017-05-10 06:43:18]

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

[kojii, 2017-05-10 16:11:22]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2017-05-10 16:12:36]

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-05-10 18:12:44]

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

[commit-bot: I haz the power, 2017-05-10 18:12:46]

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

[kojii, 2017-05-11 02:39:02]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2017-05-11 02:40:16]

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-05-11 04:52:10]

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

[commit-bot: I haz the power, 2017-05-11 04:52:11]

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

[kojii, 2017-05-11 05:37:19]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2017-05-11 05:38:07]

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-05-11 08:04:56]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1494481039708480,
"parent_rev": "fc17a86b6a7a678a7125b0caa7c0aa1c1bee6713", "commit_rev":
"fe323e6fc54e4f4ef0460d5052876d6b092c1e9e"}

[commit-bot: I haz the power, 2017-05-11 08:05:06]

Description was changed from

==========
[LayoutNG] Fix 'vertical-align: text-top/-bottom' and atomic inline margins

This patch fixes:
1. 'vertical-align: text-top' and 'text-bottom' to align to the text
   edge, not to the box edge.
2. Use the margin box of atomic inlines, not the border box.

It also:
3. Changes the geometry of NGLineBoxFragment from the text box to the
   line box to simplify. The geometry of RootInlineBox is unchanged.

Positions were fixed in relevant tests, but they were not rendered yet.

A false-positive test was discovered as its reference layout was
improved.

This patch fixes the block progression margins for atomic inline, but:
a. The block progression margin/border/padding for non-atomic inlines
   do not affect layout, only to overflows. Overflows is not implemented
   yet.
b. The inline progression margin for atomic inlines and margin/border/
   padding for non-atomic inlines is not implemented yet. It turned out
   that it's better to work on it after line breaker refactoring.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

to

==========
[LayoutNG] Fix 'vertical-align: text-top/-bottom' and atomic inline margins

This patch fixes:
1. 'vertical-align: text-top' and 'text-bottom' to align to the text
   edge, not to the box edge.
2. Use the margin box of atomic inlines, not the border box.

It also:
3. Changes the geometry of NGLineBoxFragment from the text box to the
   line box to simplify. The geometry of RootInlineBox is unchanged.

Positions were fixed in relevant tests, but they were not rendered yet.

A false-positive test was discovered as its reference layout was
improved.

This patch fixes the block progression margins for atomic inline, but:
a. The block progression margin/border/padding for non-atomic inlines
   do not affect layout, only to overflows. Overflows is not implemented
   yet.
b. The inline progression margin for atomic inlines and margin/border/
   padding for non-atomic inlines is not implemented yet. It turned out
   that it's better to work on it after line breaker refactoring.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng

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

[commit-bot: I haz the power, 2017-05-11 08:05:07]

Committed patchset #9 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/fe323e6fc54e4f4ef0460d505287...

[cbiesinger, 2017-05-11 19:54:19]

On 2017/05/09 02:18:42, ikilpatrick wrote:
>
https://codereview.chromium.org/2845913002/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/ng/ng_box_fragment.cc (right):
> 
>
https://codereview.chromium.org/2845913002/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/ng/ng_box_fragment.cc:45: //
RunOldLayout.
> On 2017/05/09 01:35:27, kojii wrote:
> > and forgot MultiCol, this maybe the first one who needs it? LayoutMultiCol
> does
> > not override, but I think it should be, need to double-check the spec.
> 
> Right, I think we actually want each fragment or layout_result to have a
member
> which is its baseline position. Otherwise w/ caching we'd have to perform a
tree
> walk to do this correctly.
> 
> If it is needed for painting it should be on the fragment, otherwise it should
> be on the layout_result.

Yeah, I agree with this. We may need two members (alphabetic vs ideographic) and
possible four (vertical line vs horizontal)

Also, there's kPositionOfInteriorLineBoxes; I don't really know much about how
that works but that may have to go onto the constraint space?

[kojii, 2017-05-11 22:20:01]

On 2017/05/11 at 19:54:19, cbiesinger wrote:
> On 2017/05/09 02:18:42, ikilpatrick wrote:
> > Right, I think we actually want each fragment or layout_result to have a
member
> > which is its baseline position. Otherwise w/ caching we'd have to perform a
tree
> > walk to do this correctly.
> > 
> > If it is needed for painting it should be on the fragment, otherwise it
should
> > be on the layout_result.
> 
> Yeah, I agree with this. We may need two members (alphabetic vs ideographic)

I agree it's good today, but that doesn't work when we support 8 different
baselines:
https://drafts.csswg.org/css-inline/#dominant-baseline-property

Even more, OpenType may provide per-script baselines, and TextBox needs to
compute how to align between different baseline types.

> and possible four (vertical line vs horizontal)

When orthogonal, baseline is synthesized, right?

> Also, there's kPositionOfInteriorLineBoxes; I don't really know much about how
that works but that may have to go onto the constraint space?

Yeah, I need to find all usages in the legacy and think about how to handle
that. I'm not there yet.