A height of a table which has position: absolute and box-sizing: border-box ignores half of the bor…

RenderBox::computePositionedLogicalHeightUsing() decides top, bottom and
height of the positioned box.

LogicalHeightLength is a height specified by CSS. So, whether it
contains borders and paddings depends on box-sizing value. There is no
difference between a normal block and a table.

In a table case, the current code overwrites the CSS height using the
height of a RenderBox. The height of a RenderBox always doesn't contain
borders and paddings. If a table has box-sizing border-box, the code
sets the height which doesn't contain borders and paddings to
logicalHeightLength which thinks its value contains borders and
paddings. This makes a result height narrower than an expected height.

Since we already has a height which doesn't contain borders and padding
for a table, we should not call adjustContentBoxLogicalHeightForBoxSizing().

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

[yuki.sekiguchi, 2013-09-30 05:34:41]

Hi

This is my first patch to blink, so if I do wrong, please correct me.

git cl upload says that there is no OWNER and suggests pkasting@.
However, I blamed computePositionedLogicalHeightUsing(), and you reviewed some
patches in the patches.
Therefore, I send request you.

[ojan, 2013-10-02 00:52:58]

On 2013/09/30 05:34:41, yuki.sekiguchi wrote:
> Hi
> 
> This is my first patch to blink, so if I do wrong, please correct me.
> 
> git cl upload says that there is no OWNER and suggests pkasting@.
> However, I blamed computePositionedLogicalHeightUsing(), and you reviewed some
> patches in the patches.
> Therefore, I send request you.

I'm not really sure what the right fix here is. Is the bug that we're calling
adjustContentBoxLogicalHeightForBoxSizing at all? Also, why is this only in the
isTable codepath?

[yuki.sekiguchi, 2013-10-02 07:58:18]

Thank you for reviewing!

On 2013/10/02 00:52:58, ojan wrote:
> On 2013/09/30 05:34:41, yuki.sekiguchi wrote:
> > Hi
> > 
> > This is my first patch to blink, so if I do wrong, please correct me.
> > 
> > git cl upload says that there is no OWNER and suggests pkasting@.
> > However, I blamed computePositionedLogicalHeightUsing(), and you reviewed
some
> > patches in the patches.
> > Therefore, I send request you.
> 
> I'm not really sure what the right fix here is. Is the bug that we're calling
> adjustContentBoxLogicalHeightForBoxSizing at all? Also, why is this only in
the
> isTable codepath?

Yes. Not calling adjustContentBoxLogicalHeightForBoxSizing() for a border-box
table is
also fix this bug. However, I think my fix is better than that for the following
reason.

The logicalHeightLength is a height which is specified by a content CSS. If a
normal block
is content-box, the length doesn't contain borders and paddings. Otherwise, it
contains
borders and padding. In the current code, the logicalHeightLength for a table
always
doesn't contain borders and padding. Since this is different from a normal block
case, I
think we should fix a table case.

[ojan, 2013-10-03 00:03:38]

Julien might have useful insights here since it involves table layout.

On 2013/10/02 07:58:18, yuki.sekiguchi wrote:
> Thank you for reviewing!
> 
> On 2013/10/02 00:52:58, ojan wrote:
> > On 2013/09/30 05:34:41, yuki.sekiguchi wrote:
> > > Hi
> > > 
> > > This is my first patch to blink, so if I do wrong, please correct me.
> > > 
> > > git cl upload says that there is no OWNER and suggests pkasting@.
> > > However, I blamed computePositionedLogicalHeightUsing(), and you reviewed
> some
> > > patches in the patches.
> > > Therefore, I send request you.
> > 
> > I'm not really sure what the right fix here is. Is the bug that we're
calling
> > adjustContentBoxLogicalHeightForBoxSizing at all? Also, why is this only in
> the
> > isTable codepath?
> 
> Yes. Not calling adjustContentBoxLogicalHeightForBoxSizing() for a border-box
> table is
> also fix this bug.

That's not what I meant. I meant can we stop calling
adjustContentBoxLogicalHeightForBoxSizing entirely here. In general, there are
very few places in the code where we need to check box-sizing and adjust for it.
So, adding any new ones is a red flag that it's not the right solution.

> The logicalHeightLength is a height which is specified by a content CSS. If a
> normal block
> is content-box, the length doesn't contain borders and paddings. Otherwise, it
> contains
> borders and padding. In the current code, the logicalHeightLength for a table
> always
> doesn't contain borders and padding. Since this is different from a normal
block
> case, I
> think we should fix a table case.

logicalHeightLength on a table doesn't include border and padding, but on
another type of element it does? I'm not very familiar with table layout, so I
need you to explain it to me so I can do the review. :)

[Julien - ping for review, 2013-10-03 18:04:01]

I think this change is related to the code in
RenderTable::convertStyleLogicalHeightToComputedHeight(), more specifically this
comment:

// HTML tables size as though CSS height includes border/padding, CSS tables do
not.

So I don't think the fix is OK.

https://codereview.chromium.org/25206002/diff/1/LayoutTests/fast/box-sizing/t...
File LayoutTests/fast/box-sizing/table-collapse.html (right):

https://codereview.chromium.org/25206002/diff/1/LayoutTests/fast/box-sizing/t...
LayoutTests/fast/box-sizing/table-collapse.html:30: <table style="top: 200px;">
Can we use a checkLayout test instead of a ref-test? It doesn't really buy
anything and it's slower.

https://codereview.chromium.org/25206002/diff/1/LayoutTests/fast/box-sizing/t...
LayoutTests/fast/box-sizing/table-collapse.html:39: <table style="bottom:
100px;">
You should also test CSS table as they have a different behavior: <div
style="display: table">

[yuki.sekiguchi, 2013-10-04 12:14:38]

Hi ojan,

Thank you for reviewing.

On 2013/10/03 00:03:38, ojan wrote:
> That's not what I meant. I meant can we stop calling
> adjustContentBoxLogicalHeightForBoxSizing entirely here. In general, there are
> very few places in the code where we need to check box-sizing and adjust for
it.
> So, adding any new ones is a red flag that it's not the right solution.

I understand it. 

This function resolves top, bottom, height and so on using CSS value. We
have to use a CSS height, so we have to check box-sizing to adjust the
height. We may be able to cache the resolved height and reuse it from
here, but, I think it is overkill.

> logicalHeightLength on a table doesn't include border and padding, but on
> another type of element it does? I'm not very familiar with table layout, so I
> need you to explain it to me so I can do the review. :)

I added the description of logicalHeightLength to a description.
Summary: No.

[yuki.sekiguchi, 2013-10-04 12:22:12]

Hi Julien,

Thank you for reviewing.

On 2013/10/03 18:04:01, Julien Chaffraix wrote:
> I think this change is related to the code in
> RenderTable::convertStyleLogicalHeightToComputedHeight(), more specifically
this
> comment:
> 
> // HTML tables size as though CSS height includes border/padding, CSS tables
do
> not.
> 
> So I don't think the fix is OK.

I changed the description. Could you check it?

convertStyleLogicalHeightToComputedHeight() converts CSS height to
computed height with considering box-sizing. However, the problem is
converting wrong height to the CSS height. I don't think the difference
between HTML tables and CSS table is a problem. Both of them reproduce
this bug.

>
https://codereview.chromium.org/25206002/diff/1/LayoutTests/fast/box-sizing/t...
> File LayoutTests/fast/box-sizing/table-collapse.html (right):
> 
>
https://codereview.chromium.org/25206002/diff/1/LayoutTests/fast/box-sizing/t...
> LayoutTests/fast/box-sizing/table-collapse.html:30: <table style="top:
200px;">
> Can we use a checkLayout test instead of a ref-test? It doesn't really buy
> anything and it's slower.

Thank you for a good information! I didn't know ckeckLayout test.

Changed to use chackLayout test.
 
>
https://codereview.chromium.org/25206002/diff/1/LayoutTests/fast/box-sizing/t...
> LayoutTests/fast/box-sizing/table-collapse.html:39: <table style="bottom:
> 100px;">
> You should also test CSS table as they have a different behavior: <div
> style="display: table">

Added.

[yuki.sekiguchi, 2013-10-04 12:26:37]

Sorry, I sent this too early..

On 2013/10/04 12:22:12, yuki.sekiguchi wrote:
> convertStyleLogicalHeightToComputedHeight() converts CSS height to
> computed height with considering box-sizing. However, the problem is
> converting wrong height to the CSS height. I don't think the difference
> between HTML tables and CSS table is a problem. Both of them reproduce
> this bug.

I understood that using adjustContentBoxLogicalHeightForBoxSizing() to
adjust box-sizing for table is a wrong way.

Changed the fix not to call adjustContentBoxLogicalHeightForBoxSizing()
in a table case.

[ojan, 2013-10-16 01:50:35]

lgtm

[commit-bot: I haz the power, 2013-10-16 01:51:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yuki.sekiguchi@access-company.com/2520...

[commit-bot: I haz the power, 2013-10-16 10:03:40]

Change committed as 159743