Use logicalBottom when computing baselines in vertical-lr inline-blocks

Use logicalBottom when computing baselines in vertical-lr inline-blocks

When computing the baseline position of inline-block elements we use the
InlineFlow logicalTop and the FontMetrics ascent. The issue comes from
the fact that these units are incompatible. The logicalTop of a
vertical-lr element is offset to the left edge, while the ascent is the
distance from the right edge.

We need to either use logical value for the FontMetrics ascent so we can
compute the correctly the baselines of vertical-lr elements, or just using
the logicalBottom for these cases.

The approach based on a logicalAscent API for FontMetrics would require
a lot of work because inline-block logic assumes everything is vertical-rl
and at some point, flips the elements along the block-axis in case of
vertical-lr mode.

While it'd be desirable to get rid of this flipping logic, this patch tries
first the simpler approach of using logicalBottom, which aligns with the
currently implemented logic.

BUG=664386
Committed: https://crrev.com/e2226caaef64115627bddea776cac031d065fd5a
Cr-Commit-Position: refs/heads/master@{#438502}

[jfernandez, 2016-11-22 23:38:11]

Description was changed from

==========
Define a logicalAscent to deal with baselines in vertical-lr modes

When computing the baseline position of inline-block elements we use the
InlineFlow logicalTop and the FontMetrics ascent. The issue comes from
the fact that these units are incompatible. The logicalTop of a
vertical-lr element is offset to the left edge, while the ascent is the
distance from the right edge.

We need a logical value for the FontMetrics ascent so we can compute the
correctly the baselines of vertical-lr elements.

BUG=664386
==========

to

==========
Define a logicalAscent to deal with baselines in vertical-lr modes

When computing the baseline position of inline-block elements we use the
InlineFlow logicalTop and the FontMetrics ascent. The issue comes from
the fact that these units are incompatible. The logicalTop of a
vertical-lr element is offset to the left edge, while the ascent is the
distance from the right edge.

We need a logical value for the FontMetrics ascent so we can compute the
correctly the baselines of vertical-lr elements.

BUG=664386
==========

[jfernandez, 2016-11-22 23:38:11]

jfernandez@igalia.com changed reviewers:
+ kojih@chromium.org

[jfernandez, 2016-11-22 23:43:54]

jfernandez@igalia.com changed reviewers:
+ kojii@chromium.org
- kojih@chromium.org

[jfernandez, 2016-11-22 23:43:55]

Hi @kojii I hope you could give me some feedback in this patch. It's still a
preliminary approach, but I think it goes in the right direction.

I've found out that InlineFlowBox::placeBoxesInBlockDirection flips the
vertical-lr inline-block elements once they are positioned in their
container. Hence, I don't think we need to implement a logicalAscent
function in the FontMetrics, but using InlineBox::logicalBottom 
instead of logicalTop for vertical-lr boxes. 

This implies that we assume everything is vertical-rl, hence we use
ascent, right margins, ... and finally, we flip the elements in the
block-axis, if needed.

All the tests pass except 2, but I'm not really sure if such failures
are actual regressions caused by my patch or that those tests
need to be rebaselined.

[kojii, 2016-11-24 03:41:26]

Thank you for your investigation and continued efforts on this topic. In short,
I think this looks good to me.

With your words, now I remember we have marginOver/Under that has the same
assumption baked in; everything is vertical-rl. So we need to change the code
depends on whether the target is before the line flipping or after. That is very
unfortunate of our current design, can't wait for us to get rid of the flipping.
We tried to get rid of it before but it was more work than anticipated, so we
probably won't get it done until LayoutNG.

I was misunderstanding that the change was in firstLineBoxBaseline() until this
point. inlineBlockBaseline() is overridden in many classes but used only in
LayoutTextControlSingleLine, as far as I codesearched it? Did you find any other
uses I missed? If not, it's safe to change, the innerEditor should have been
laid out before this function is called, and also we don't support vertical in
form controls today.

That said, I don't understand why this change affects these two tests either. I
think it's fine ot rebaseline them; no visual changes, we have inline block
baseline alignment tests in improted/csswg-test/css-writing-modes that are still
pass, and the change is limited to isFlippedLinesWritingMode().

[jfernandez, 2016-11-24 09:25:54]

On 2016/11/24 03:41:26, kojii wrote:
> Thank you for your investigation and continued efforts on this topic. In
short,
> I think this looks good to me.
> 

Thank you for the feedback. 

> With your words, now I remember we have marginOver/Under that has the same
> assumption baked in; everything is vertical-rl. So we need to change the code
> depends on whether the target is before the line flipping or after. That is
very
> unfortunate of our current design, can't wait for us to get rid of the
flipping.
> We tried to get rid of it before but it was more work than anticipated, so we
> probably won't get it done until LayoutNG.

Yes, the "Over/Under" concepts also complicate things. I also tried to ge t rid
of the flipping logic, but it caused many tests to fail. Once I've got a valid
approach
to make inline-block baseline alignment work in the vertical-lr cases, even
using
the flipping logic, I'll give a another try to the removal approach.

> 
> I was misunderstanding that the change was in firstLineBoxBaseline() until
this
> point. inlineBlockBaseline() is overridden in many classes but used only in
> LayoutTextControlSingleLine, as far as I codesearched it? Did you find any
other
> uses I missed? If not, it's safe to change, the innerEditor should have been
> laid out before this function is called, and also we don't support vertical in
> form controls today.
> 

No misunderstanding, at all. When we talked about this issues with vertical-lr 
issues and baselines we were analyzing Flexbox cases, which were relying
in firstLineBoxBaseline() for aligning the flex items. However, in the case of
inline-block items, that function is not used and  inlineBlockBaseline() is
called
instead.

> That said, I don't understand why this change affects these two tests either.
I
> think it's fine ot rebaseline them; no visual changes, we have inline block
> baseline alignment tests in improted/csswg-test/css-writing-modes that are
still
> pass, and the change is limited to isFlippedLinesWritingMode().

I have the impression that the change affects, some how, to the empty spaces
generated between inline-blocks because of the HTML indentation. I'm not sure 
why, but if you agree with me that a rebasline is allowed for this case, I'll
proceed.

[jfernandez, 2016-11-24 11:15:14]

Description was changed from

==========
Define a logicalAscent to deal with baselines in vertical-lr modes

When computing the baseline position of inline-block elements we use the
InlineFlow logicalTop and the FontMetrics ascent. The issue comes from
the fact that these units are incompatible. The logicalTop of a
vertical-lr element is offset to the left edge, while the ascent is the
distance from the right edge.

We need a logical value for the FontMetrics ascent so we can compute the
correctly the baselines of vertical-lr elements.

BUG=664386
==========

to

==========
Use logicalBottom when computing baselines in vertical-lr inline-blocks

When computing the baseline position of inline-block elements we use the
InlineFlow logicalTop and the FontMetrics ascent. The issue comes from
the fact that these units are incompatible. The logicalTop of a
vertical-lr element is offset to the left edge, while the ascent is the
distance from the right edge.

We need to either use logical value for the FontMetrics ascent so we can
compute the correctly the baselines of vertical-lr elements, or just using
the logicalBottom for these cases.

The approach based on a logicaAscent API for FontMetrics would require
a lot of work because inline-block logic assumes everything is vertical-rl
and at some point, flips the elements along the block-axis in case of
vertical-lr mode.

While it'd be desirable to get rid of this flipping logic, this patch tries
first the simpler approach of using logicalBottom, which aligns with the
currently implemented logic.

BUG=664386
==========

[jfernandez, 2016-11-24 18:10:05]

Patchset #2 (id:20001) has been deleted

[jfernandez, 2016-11-24 23:30:21]

jfernandez@igalia.com changed reviewers:
+ cbiesinger@chromium.org, mstensho@opera.com

[jfernandez, 2016-11-24 23:30:50]

Adding more layout people to help with the review.

[mstensho (USE GERRIT), 2016-12-01 10:33:07]

Please add a minimal test.

https://codereview.chromium.org/2523573003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/2523573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:2567: // case of
verticalLR mode, so we can assume verticalRL for now.
I don't understand this comment about assuming verticalRL.

[jfernandez, 2016-12-01 10:50:16]

Yeah, sorry. I ust wanted to verify the approach to address
verticalLR cases was correct. 

I'll submit a new patch with the required test cases.

https://codereview.chromium.org/2523573003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/2523573003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:2567: // case of
verticalLR mode, so we can assume verticalRL for now.
On 2016/12/01 10:33:07, mstensho wrote:
> I don't understand this comment about assuming verticalRL.

Even that we would need the "descent", instead of the "ascent"
for verticalLR, the rest of the code assume "everything is
verticalRL, so it uses terms like Above/Over which uses
"right" edges for vertical cases (which is incorrect for
verticalLR cases).

So, we return the "ascent" assuming that the logic implemented 
in placeBoxesInBlockDirection will flip the boxes after
determining the baselines.

[jfernandez, 2016-12-02 01:28:35]

Added a new LayoutTest.

[mstensho (USE GERRIT), 2016-12-02 09:02:40]

On 2016/12/02 01:28:35, jfernandez wrote:
> Added a new LayoutTest.

Please don't add PNG tests, unless you have very good reasons for it. Use
reftests or testharness tests, and keep it simple, please.

[jfernandez, 2016-12-02 10:34:45]

On 2016/12/02 09:02:40, mstensho wrote:
> On 2016/12/02 01:28:35, jfernandez wrote:
> > Added a new LayoutTest.
> 
> Please don't add PNG tests, unless you have very good reasons for it. Use
> reftests or testharness tests, and keep it simple, please.

I try to avoid them, indeed. However, this case is complex to verify using
testharness tests, because of the font family and sizes. It's not easy to
make such tests platform independent. Using Ahem as font-family helps on that
issue, but it makes harder to verify visually the result, since the Ahem font
uses a central/middle baseline instead of the alphabetic one. I guess I can
use a monospace font-family, but I've got doubts about whether it's 
platform independent or not.

Reftests are also hard to achieve for baseline use cases. Actually, I need
this patch to land to be able to define reftests for flexbox and grid
baseline use cases. I can't use floats or absolute positioning because it's
very complex to achieve the baseline alignment with those models, and in
any case, the reftest results would depend on the font-size, so they likely 
will fail in different platforms.

I guess the less bad approach, if we want to avoid the png tests, would
be to use Ahem, and assuming we won't be able to verify the test result
visually. 

However, if you can confirm that using a monospace font-family doesn't
cause any platform-dependent issue, that would be definitively the best
solution.

Finally, I'm not really sure what do you mean with "keep it simple"; IMHO
the test is pretty simple. Do you mean I should remove the text I use to
describe each test case ?

[mstensho (USE GERRIT), 2016-12-02 11:38:03]

> Finally, I'm not really sure what do you mean with "keep it simple"; IMHO
> the test is pretty simple. Do you mean I should remove the text I use to
> describe each test case ?

It looks more like a test suite, rather than a test. :) That's what I meant. But
maybe that's what we need at this point.

With your explanation, I think it's justified to use PNG tests here, to provide
a baseline. Future tests can hopefully use something "better".

https://codereview.chromium.org/2523573003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/2523573003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:2568: return
((style()->isFlippedLinesWritingMode()
This expression now became very complex. Maybe you could split it into bite
sizes?

https://codereview.chromium.org/2523573003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp (right):

https://codereview.chromium.org/2523573003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp:772: //
InlineFlowBox::placeBoxesInBlockDirection will flip lines in
InlineFlowBox::placeBoxesInBlockDirection happens to be this method. :)

So, maybe "We will flip ...".

[jfernandez, 2016-12-05 23:57:28]

https://codereview.chromium.org/2523573003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/2523573003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:2568: return
((style()->isFlippedLinesWritingMode()
On 2016/12/02 11:38:03, mstensho wrote:
> This expression now became very complex. Maybe you could split it into bite
> sizes?

Acknowledged.

https://codereview.chromium.org/2523573003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp (right):

https://codereview.chromium.org/2523573003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/line/InlineFlowBox.cpp:772: //
InlineFlowBox::placeBoxesInBlockDirection will flip lines in
On 2016/12/02 11:38:03, mstensho wrote:
> InlineFlowBox::placeBoxesInBlockDirection happens to be this method. :)
> 
> So, maybe "We will flip ...".

:) Yes, better said that way.

[mstensho (USE GERRIT), 2016-12-06 08:03:55]

lgtm - typo in description: "logicaAscent"

[jfernandez, 2016-12-06 09:16:46]

Description was changed from

==========
Use logicalBottom when computing baselines in vertical-lr inline-blocks

When computing the baseline position of inline-block elements we use the
InlineFlow logicalTop and the FontMetrics ascent. The issue comes from
the fact that these units are incompatible. The logicalTop of a
vertical-lr element is offset to the left edge, while the ascent is the
distance from the right edge.

We need to either use logical value for the FontMetrics ascent so we can
compute the correctly the baselines of vertical-lr elements, or just using
the logicalBottom for these cases.

The approach based on a logicaAscent API for FontMetrics would require
a lot of work because inline-block logic assumes everything is vertical-rl
and at some point, flips the elements along the block-axis in case of
vertical-lr mode.

While it'd be desirable to get rid of this flipping logic, this patch tries
first the simpler approach of using logicalBottom, which aligns with the
currently implemented logic.

BUG=664386
==========

to

==========
Use logicalBottom when computing baselines in vertical-lr inline-blocks

When computing the baseline position of inline-block elements we use the
InlineFlow logicalTop and the FontMetrics ascent. The issue comes from
the fact that these units are incompatible. The logicalTop of a
vertical-lr element is offset to the left edge, while the ascent is the
distance from the right edge.

We need to either use logical value for the FontMetrics ascent so we can
compute the correctly the baselines of vertical-lr elements, or just using
the logicalBottom for these cases.

The approach based on a logicalAscent API for FontMetrics would require
a lot of work because inline-block logic assumes everything is vertical-rl
and at some point, flips the elements along the block-axis in case of
vertical-lr mode.

While it'd be desirable to get rid of this flipping logic, this patch tries
first the simpler approach of using logicalBottom, which aligns with the
currently implemented logic.

BUG=664386
==========

[jfernandez, 2016-12-12 09:11:26]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2016-12-12 09:11: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, 2016-12-12 10:50:05]

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

[commit-bot: I haz the power, 2016-12-12 10:50:06]

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...)

[jfernandez, 2016-12-14 09:35:19]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2016-12-14 09:35: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, 2016-12-14 13:39:12]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481708119332740,
"parent_rev": "1da5147f3934a4fa1553f12fc5f1a8cc669631df", "commit_rev":
"081915f44caa2484c938b1cffe76a224f4090e5e"}

[commit-bot: I haz the power, 2016-12-14 13:39:41]

Description was changed from

==========
Use logicalBottom when computing baselines in vertical-lr inline-blocks

When computing the baseline position of inline-block elements we use the
InlineFlow logicalTop and the FontMetrics ascent. The issue comes from
the fact that these units are incompatible. The logicalTop of a
vertical-lr element is offset to the left edge, while the ascent is the
distance from the right edge.

We need to either use logical value for the FontMetrics ascent so we can
compute the correctly the baselines of vertical-lr elements, or just using
the logicalBottom for these cases.

The approach based on a logicalAscent API for FontMetrics would require
a lot of work because inline-block logic assumes everything is vertical-rl
and at some point, flips the elements along the block-axis in case of
vertical-lr mode.

While it'd be desirable to get rid of this flipping logic, this patch tries
first the simpler approach of using logicalBottom, which aligns with the
currently implemented logic.

BUG=664386
==========

to

==========
Use logicalBottom when computing baselines in vertical-lr inline-blocks

When computing the baseline position of inline-block elements we use the
InlineFlow logicalTop and the FontMetrics ascent. The issue comes from
the fact that these units are incompatible. The logicalTop of a
vertical-lr element is offset to the left edge, while the ascent is the
distance from the right edge.

We need to either use logical value for the FontMetrics ascent so we can
compute the correctly the baselines of vertical-lr elements, or just using
the logicalBottom for these cases.

The approach based on a logicalAscent API for FontMetrics would require
a lot of work because inline-block logic assumes everything is vertical-rl
and at some point, flips the elements along the block-axis in case of
vertical-lr mode.

While it'd be desirable to get rid of this flipping logic, this patch tries
first the simpler approach of using logicalBottom, which aligns with the
currently implemented logic.

BUG=664386

Review-Url: https://codereview.chromium.org/2523573003
==========

[commit-bot: I haz the power, 2016-12-14 13:39:43]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-12-14 13:41:30]

Description was changed from

==========
Use logicalBottom when computing baselines in vertical-lr inline-blocks

When computing the baseline position of inline-block elements we use the
InlineFlow logicalTop and the FontMetrics ascent. The issue comes from
the fact that these units are incompatible. The logicalTop of a
vertical-lr element is offset to the left edge, while the ascent is the
distance from the right edge.

We need to either use logical value for the FontMetrics ascent so we can
compute the correctly the baselines of vertical-lr elements, or just using
the logicalBottom for these cases.

The approach based on a logicalAscent API for FontMetrics would require
a lot of work because inline-block logic assumes everything is vertical-rl
and at some point, flips the elements along the block-axis in case of
vertical-lr mode.

While it'd be desirable to get rid of this flipping logic, this patch tries
first the simpler approach of using logicalBottom, which aligns with the
currently implemented logic.

BUG=664386

Review-Url: https://codereview.chromium.org/2523573003
==========

to

==========
Use logicalBottom when computing baselines in vertical-lr inline-blocks

When computing the baseline position of inline-block elements we use the
InlineFlow logicalTop and the FontMetrics ascent. The issue comes from
the fact that these units are incompatible. The logicalTop of a
vertical-lr element is offset to the left edge, while the ascent is the
distance from the right edge.

We need to either use logical value for the FontMetrics ascent so we can
compute the correctly the baselines of vertical-lr elements, or just using
the logicalBottom for these cases.

The approach based on a logicalAscent API for FontMetrics would require
a lot of work because inline-block logic assumes everything is vertical-rl
and at some point, flips the elements along the block-axis in case of
vertical-lr mode.

While it'd be desirable to get rid of this flipping logic, this patch tries
first the simpler approach of using logicalBottom, which aligns with the
currently implemented logic.

BUG=664386

Committed: https://crrev.com/e2226caaef64115627bddea776cac031d065fd5a
Cr-Commit-Position: refs/heads/master@{#438502}
==========

[commit-bot: I haz the power, 2016-12-14 13:41:32]

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