Vertical-aligned of LayoutText should not contribute to a layout.

Vertical-aligned of LayoutText should not contribute to a layout.

According to http://www.w3.org/TR/CSS2/visudet.html#propdef-vertical-align,
the vertical-aligned is a non-inheirted value. However an vertical-aligned value
inherited from a container block is applied to LayoutText as well. This is because
the style used by LayoutText is originated by its container block and it is shared
with LayoutText rather than creating new one. To fix this, we return an initial value
of vertical-align for LayoutText in InlineBox.h.

BUG=496850
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197007

[changseok, 2015-06-10 10:48:30]

shivamidow@gmail.com changed reviewers:
+ chrishtr@chromium.org, eae@chromium.org, leviw@chromium.org,
mstensho@opera.com

[mstensho (USE GERRIT), 2015-06-10 11:40:42]

This looks like a correct fix, but please proofread your commit message.

Also, I'd like to point out that it's not really a right approach in general to
let LayoutText just copy all properties from its parent. Only inheritable ones
should be inherited. But since we don't, we have this bug. I just recently got
bitten by the same problem in the multicol code (got a LayoutText that was
absolutely positioned).

See bug 492537, where I tried to fix this throughout the code, but eventually
decided against it, since it became obvious that it would increase memory usage.

And I suggest that you omit the comment about Gecko and IE. They do it
correctly, and there's no need to point it out, since they're just implementing
the spec correctly.

https://codereview.chromium.org/1178473003/diff/1/LayoutTests/fast/inline/ver...
File LayoutTests/fast/inline/vertical-align-text-inherit.html (right):

https://codereview.chromium.org/1178473003/diff/1/LayoutTests/fast/inline/ver...
LayoutTests/fast/inline/vertical-align-text-inherit.html:6: overflow: hidden;
The background-color, display, overflow, text-overflow, width and white-space
properties are irrelevant, and just cause the text to be clipped with an
ellipsis so that the pass condition text isn't fully visible. Can you remove
them?

https://codereview.chromium.org/1178473003/diff/1/LayoutTests/fast/inline/ver...
LayoutTests/fast/inline/vertical-align-text-inherit.html:14: background-color:
green;
See my comment further below. This inline-block is useful in the test, to
stretch the height of the line box, but it doesn't really need a background
color.

https://codereview.chromium.org/1178473003/diff/1/LayoutTests/fast/inline/ver...
LayoutTests/fast/inline/vertical-align-text-inherit.html:24: This text should be
aligned with the bottom of a left green box.
The baseline of this text, perhaps. Not the bottom of the text. But that's hard
to measure with the naked eye anyway.

How about replacing this text with something like:

<span>This text should</span> be on the same line.

https://codereview.chromium.org/1178473003/diff/1/Source/core/layout/line/Inl...
File Source/core/layout/line/InlineBox.h (right):

https://codereview.chromium.org/1178473003/diff/1/Source/core/layout/line/Inl...
Source/core/layout/line/InlineBox.h:282: EVerticalAlign verticalAlign() const {
return layoutObject().isText() ?
layoutObject().styleRef().initialVerticalAlign() :
layoutObject().style(m_bitfields.firstLine())->verticalAlign(); }
initialVerticalAlign() is static, so you don't need an object to call it.

[changseok, 2015-06-11 04:39:05]

Thanks for your review.

>Also, I'd like to point out that it's not really a right approach in general to
>let LayoutText just copy all properties from its parent. Only inheritable ones
>should be inherited. But since we don't, we have this bug. I just recently got
>bitten by the same problem in the multicol code (got a LayoutText that was
>absolutely positioned).

>See bug 492537, where I tried to fix this throughout the code, but eventually
>decided against it, since it became obvious that it would increase memory
usage.
I also wondered why a text node doesn't have its own style, it just uses its
parent style instead.
You solved my curiosity. Thanks. ;) 

>And I suggest that you omit the comment about Gecko and IE. They do it
>correctly, and there's no need to point it out, since they're just implementing
>the spec correctly.
Yeap, removed.

https://codereview.chromium.org/1178473003/diff/1/LayoutTests/fast/inline/ver...
File LayoutTests/fast/inline/vertical-align-text-inherit.html (right):

https://codereview.chromium.org/1178473003/diff/1/LayoutTests/fast/inline/ver...
LayoutTests/fast/inline/vertical-align-text-inherit.html:6: overflow: hidden;
On 2015/06/10 11:40:41, mstensho wrote:
> The background-color, display, overflow, text-overflow, width and white-space
> properties are irrelevant, and just cause the text to be clipped with an
> ellipsis so that the pass condition text isn't fully visible. Can you remove
> them?

Of course, I can. =) I just mimicked the original bug introduced in
crbug.com/496850

https://codereview.chromium.org/1178473003/diff/1/LayoutTests/fast/inline/ver...
LayoutTests/fast/inline/vertical-align-text-inherit.html:14: background-color:
green;
On 2015/06/10 11:40:41, mstensho wrote:
> See my comment further below. This inline-block is useful in the test, to
> stretch the height of the line box, but it doesn't really need a background
> color.

O.K I see.

https://codereview.chromium.org/1178473003/diff/1/LayoutTests/fast/inline/ver...
LayoutTests/fast/inline/vertical-align-text-inherit.html:24: This text should be
aligned with the bottom of a left green box.
On 2015/06/10 11:40:41, mstensho wrote:
> The baseline of this text, perhaps. Not the bottom of the text. But that's
hard
> to measure with the naked eye anyway.
> 
> How about replacing this text with something like:
> 
> <span>This text should</span> be on the same line.

This is neater.

[mstensho (USE GERRIT), 2015-06-11 07:09:56]

lgtm

https://codereview.chromium.org/1178473003/diff/20001/LayoutTests/fast/inline...
File LayoutTests/fast/inline/vertical-align-text-inherit-expected.html (right):

https://codereview.chromium.org/1178473003/diff/20001/LayoutTests/fast/inline...
LayoutTests/fast/inline/vertical-align-text-inherit-expected.html:4:
vertical-align: top;
Can't you remove this too?

[changseok, 2015-06-11 08:46:00]

https://codereview.chromium.org/1178473003/diff/20001/LayoutTests/fast/inline...
File LayoutTests/fast/inline/vertical-align-text-inherit-expected.html (right):

https://codereview.chromium.org/1178473003/diff/20001/LayoutTests/fast/inline...
LayoutTests/fast/inline/vertical-align-text-inherit-expected.html:4:
vertical-align: top;
On 2015/06/11 07:09:56, mstensho wrote:
> Can't you remove this too?

Yes I can.

[changseok, 2015-06-11 09:02:04]

The CQ bit was checked by shivamidow@gmail.com

[changseok, 2015-06-11 09:02:04]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com
Link to the patchset: https://codereview.chromium.org/1178473003/#ps40001
(title: "Removed vertical-align from an expected result.")

[commit-bot: I haz the power, 2015-06-11 09:03:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1178473003/40001

[commit-bot: I haz the power, 2015-06-11 10:33:28]

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

[commit-bot: I haz the power, 2015-06-11 10:33:29]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)

[leviw_travelin_and_unemployed, 2015-06-11 20:22:24]

On 2015/06/11 at 10:33:29, commit-bot wrote:
> Try jobs failed on following builders:
>   linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)

You're failing tables/mozilla/bugs/bug14323.html

[changseok, 2015-06-12 04:04:00]

On 2015/06/11 20:22:24, leviw wrote:
> On 2015/06/11 at 10:33:29, commit-bot wrote:
> > Try jobs failed on following builders:
> >   linux_blink_rel on tryserver.blink (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)
> 
> You're failing tables/mozilla/bugs/bug14323.html

I think that's an expected result. We let a text not inherit its vertical align
style from its parent so the text 'page' should be aligned to the baseline on
the line.
Actually I checked gecko shows a same behavior. I'll update TextExpectation and
try to land again.

[changseok, 2015-06-12 04:08:10]

On 2015/06/12 04:04:00, changseok wrote:
> On 2015/06/11 20:22:24, leviw wrote:
> > On 2015/06/11 at 10:33:29, commit-bot wrote:
> > > Try jobs failed on following builders:
> > >   linux_blink_rel on tryserver.blink (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)
> > 
> > You're failing tables/mozilla/bugs/bug14323.html
> 
> I think that's an expected result. We let a text not inherit its vertical
align
> style from its parent so the text 'page' should be aligned to the baseline on
> the line.
> Actually I checked gecko shows a same behavior. I'll update TextExpectation
and
> try to land again.

Either IE does.

[changseok, 2015-06-12 05:56:36]

The CQ bit was checked by shivamidow@gmail.com

[changseok, 2015-06-12 05:56:37]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com
Link to the patchset: https://codereview.chromium.org/1178473003/#ps60001
(title: "Add tables/mozilla/bugs/bug14323.html to TestExpectation for
rebasing.")

[commit-bot: I haz the power, 2015-06-12 05:56:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1178473003/60001

[commit-bot: I haz the power, 2015-06-12 07:17:55]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197007

[kojii, 2015-09-29 18:05:16]

I'm looking into bug 537277 and it looks like it's a regression from this patch.

I'm still in the middle of investigation so there might still be some missing
parts, but the description and the fix in this patch doesn't really feel right
to me.

LayoutText is a LayoutObject for a text node. CSS inheritance is defined against
elements (or boxes.) Nodes do not have CSS properties, so when referring the
style of a LayoutText, it should look at its parent element (or box.) From that
perspective, LayoutText to copy styles from its parent is correct.

The bug expectation and the test look correct, but it's not because of
inheritance to LayoutText. Please refer this:
http://www.w3.org/TR/CSS21/visuren.html#anonymous
When a block contains text and inline elements, such text should be put into
anonymous inline boxes, and it's the anonymous inline box that blocks
inheritance of non-inherited properties. The 3rd paragraph of the link above
states this.

This patch forces vertical-align to the initial value regardless it's in an
anonymous inline box or not, so it breaks when a text node is not in an
anonymous inline box. It doesn't break major cases however, because there's an
optimization to check descendantsHaveSameLineHeightAndBaseline(), which is true
in most cases and this code doesn't run.
https://trac.webkit.org/changeset/82280/trunk/Source/WebCore/rendering/Render...

In my quick test, InlineBox that should be wrapped by anonymous inline box has
the same m_style as its parent, that's probably the root cause.

This patch and test helped me a lot to investigate bug 537277 and understand
what is going on, appreciate very much.