Percent height in auto height containers no longer computes as auto

Percent height in auto height containers no longer computes as auto

https://drafts.csswg.org/css2/visudet.html#the-height-property has changed.

See http://lists.w3.org/Archives/Public/www-style/2013May/0201.html

The section on percentage now reads:
 "<percentage>
 Specifies a percentage height. The percentage is calculated with respect to the
 height of the generated box's containing block. If the height of the containing
 block is not specified explicitly (i.e., it depends on content height), and this
 element is not absolutely positioned, the used height is calculated as if 'auto'
 was specified."

The last clause used to read: "the value computes to 'auto'."

So remove our attempt to treat percentage height in auto height containers as
computed in RenderReplaced::computeReplacedLogicalWidth() and only treat actual
computed style of auto as such.

Once we remove this feature we find that when calculating used width for replaced
elements we can end up in a situation where we need to factor in the 'used height'
to our calculation. In certain circumstances, calculating the 'used height'
requires the 'used width'. :) When this situation arises, use an estimate of the
used width based on the intrinsic width of the replaced object if it's available
and failing that use the width made available by its container.

This circularity always existed I think and we always had to assume the 'used'
width in this situation - it's just easier to surface now.

BUG=396717
Committed: https://crrev.com/4f61ad0df953092181eb059d6c119893e4202d34
Cr-Commit-Position: refs/heads/master@{#388878}

[rhogan, 2016-04-11 17:35:30]

Description was changed from

==========
Percent heigh in auto height containers no longer computes as auto

BUG=396717
==========

to

==========
Percent height in auto height containers no longer computes as auto

BUG=396717
==========

[rhogan, 2016-04-11 17:40:09]

Description was changed from

==========
Percent height in auto height containers no longer computes as auto

BUG=396717
==========

to

==========
Percent height in auto height containers no longer computes as auto

https://drafts.csswg.org/css2/visudet.html#the-height-property has changed.

See http://lists.w3.org/Archives/Public/www-style/2013May/0201.html

The section on percentage now reads:
 "<percentage>
 Specifies a percentage height. The percentage is calculated with respect to the
 height of the generated box's containing block. If the height of the containing
 block is not specified explicitly (i.e., it depends on content height), and
this
 element is not absolutely positioned, the used height is calculated as if
'auto'
 was specified."

The last clause used to read: "the value computes to 'auto'."

When calculating used width for replaced elements we can end up in a situation
where we need to factor in the 'used height' to our calculation. In certain
circumstances, calculating the 'used height' requires the 'used width'. :) When
this situation arises, use an estimate of the used width based on the intrinsic
width of the replaced object if it's available and failing that use the width
made available by its container.

BUG=396717
==========

[rhogan, 2016-04-11 17:41:10]

robhogan@gmail.com changed reviewers:
+ davve@opera.com

[rhogan, 2016-04-12 19:19:41]

robhogan@gmail.com changed reviewers:
+ pdr@chromium.org
- davve@opera.com

[rhogan, 2016-04-12 19:26:55]

Description was changed from

==========
Percent height in auto height containers no longer computes as auto

https://drafts.csswg.org/css2/visudet.html#the-height-property has changed.

See http://lists.w3.org/Archives/Public/www-style/2013May/0201.html

The section on percentage now reads:
 "<percentage>
 Specifies a percentage height. The percentage is calculated with respect to the
 height of the generated box's containing block. If the height of the containing
 block is not specified explicitly (i.e., it depends on content height), and
this
 element is not absolutely positioned, the used height is calculated as if
'auto'
 was specified."

The last clause used to read: "the value computes to 'auto'."

When calculating used width for replaced elements we can end up in a situation
where we need to factor in the 'used height' to our calculation. In certain
circumstances, calculating the 'used height' requires the 'used width'. :) When
this situation arises, use an estimate of the used width based on the intrinsic
width of the replaced object if it's available and failing that use the width
made available by its container.

BUG=396717
==========

to

==========
Percent height in auto height containers no longer computes as auto

https://drafts.csswg.org/css2/visudet.html#the-height-property has changed.

See http://lists.w3.org/Archives/Public/www-style/2013May/0201.html

The section on percentage now reads:
 "<percentage>
 Specifies a percentage height. The percentage is calculated with respect to the
 height of the generated box's containing block. If the height of the containing
 block is not specified explicitly (i.e., it depends on content height), and
this
 element is not absolutely positioned, the used height is calculated as if
'auto'
 was specified."

The last clause used to read: "the value computes to 'auto'."

So remove our attempt to treat percentage height in auto height containers as
computed in RenderReplaced::computeReplacedLogicalWidth() and only treat actual
computed style of auto as such.

Once we remove this feature we find that when calculating used width for
replaced
elements we can end up in a situation where we need to factor in the 'used
height'
to our calculation. In certain circumstances, calculating the 'used height'
requires the 'used width'. :) When this situation arises, use an estimate of the
used width based on the intrinsic width of the replaced object if it's available
and failing that use the width made available by its container.

This circularity always existed I think and we always had to assume the 'used'
width in this situation - it's just easier to surface now.

BUG=396717
==========

[rhogan, 2016-04-14 18:21:46]

robhogan@gmail.com changed reviewers:
+ davve@opera.com, leviw@chromium.org

[rhogan, 2016-04-14 18:21:46]

Hey guys - any of you got time to take a look?

[leviw_travelin_and_unemployed, 2016-04-14 22:24:39]

leviw@chromium.org changed reviewers:
+ cbiesinger@chromium.org

[leviw_travelin_and_unemployed, 2016-04-14 22:24:39]

This looks like cbiesinger's wheelhouse to me :)

[cbiesinger, 2016-04-15 21:44:30]

I have a hard time seeing how the spec change relates to this code change. In
fact this new behavior seems closer to what the old version of the spec says,
note use of "computed value" here:
https://drafts.csswg.org/css2/visudet.html#inline-replaced-width

Maybe you can elaborate on that relation a bit more?

I think the code change itself is fine and correct, just don't see how it
relates to that spec change.

[cbiesinger, 2016-04-15 21:44:40]

and one nit:

https://codereview.chromium.org/1868013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutReplaced.cpp (right):

https://codereview.chromium.org/1868013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutReplaced.cpp:581: return width;
I would merge this line with the previous one and not bother with the separate
width variable.

[rhogan, 2016-04-16 14:08:56]

On 2016/04/15 at 21:44:30, cbiesinger wrote:
> I have a hard time seeing how the spec change relates to this code change. In
fact this new behavior seems closer to what the old version of the spec says,
note use of "computed value" here:
https://drafts.csswg.org/css2/visudet.html#inline-replaced-width

When calculating width we treat the computed value of the height as auto if it's
a percent value inside an auto containing block. The spec used to support that:
"If the height of the containing
 block is not specified explicitly (i.e., it depends on content height), and
this element is not absolutely positioned, *the value computes to 'auto'*."

Now that it doesn't say that anymore we can no longer say: bool
computedHeightIsAuto = hasAutoHeightOrContainingBlockWithAutoHeight();

That's the core of my change. The rest is dealing with the circularity noted in
the main CL comment.

[davve, 2016-04-18 15:40:58]

The circularity is indeed a bit confusing. The line of thinking goes something
like "computeReplacedLogicalWidth may only call computeReplacedLogicalHeight if
the height is fixed in such a way that it doesn't depend on width". At least
that's how I understood it when meddling with the code.

To walk this through, assume we have a replaced element with aspect ratio and
height:50% inside an auto height container. Post-patch, computedHeightIsAuto is
false. To compute the used width we'll use the ratio and used height, since
we'll think there is an height to compute the width from. We call
computeReplacedLogicalHeight to get that height, but in there we seem to notice
we have an auto height after all (at least by judging from the comments). Since
we have an aspect ratio, we figure we can compute the height from the width and
ratio!  There your added estimatedUsedWidth comes into play. So we use the ratio
twice to arrive at the width. Feels a bit strange to me...

I don't have more time today to look at this but hopefully I'll be able to
investigate a bit more tomorrow.

[rhogan, 2016-04-18 17:49:29]

On 2016/04/18 at 15:40:58, davve wrote:
> We call computeReplacedLogicalHeight to get that height, but in there we seem
to notice we have an auto height after all (at least by judging from the
comments). 

Strictly speaking we establish that we 'the used height is [to be] calculated as
if 'auto' was specified'.

[cbiesinger, 2016-04-18 21:52:04]

On 2016/04/18 17:49:29, rhogan wrote:
> On 2016/04/18 at 15:40:58, davve wrote:
> > We call computeReplacedLogicalHeight to get that height, but in there we
seem
> to notice we have an auto height after all (at least by judging from the
> comments). 
> 
> Strictly speaking we establish that we 'the used height is [to be] calculated
as
> if 'auto' was specified'.

Aha... I see, I think I understand now. This is complex. I'll let davve finish
his investigation before giving lgtm...

[davve, 2016-04-19 15:20:16]

On 2016/04/18 17:49:29, rhogan wrote:
> On 2016/04/18 at 15:40:58, davve wrote:
> > We call computeReplacedLogicalHeight to get that height, but in there we
seem
> to notice we have an auto height after all (at least by judging from the
> comments). 
> 
> Strictly speaking we establish that we 'the used height is [to be] calculated
as
> if 'auto' was specified'.

Yes... Which brings us to the possibility of depending on width and the
estimated used width. My worry here is that we have to duplicate the entire
following block in estimating used width (the block with the "However, it is
suggested that" comment), pass that to computeReplacedLogicalHeight only to
round-trip around the ratio in this case. Which may be fine, though...

Btw, have you investigated letting
LayoutReplaced::constrainIntrinsicSizeToMinMax() handle this case?

I don't want to block you on this, so if other thinks this is fine I shouldn't
stand in the way.

(Been busy with beta blockers lately...)

[rhogan, 2016-04-19 20:14:44]

On 2016/04/19 at 15:20:16, davve wrote:
> On 2016/04/18 17:49:29, rhogan wrote:
> > On 2016/04/18 at 15:40:58, davve wrote:
> > > We call computeReplacedLogicalHeight to get that height, but in there we
seem
> > to notice we have an auto height after all (at least by judging from the
> > comments). 
> > 
> > Strictly speaking we establish that we 'the used height is [to be]
calculated as
> > if 'auto' was specified'.
> 
> Yes... Which brings us to the possibility of depending on width and the
estimated used width. My worry here is that we have to duplicate the entire
following block in estimating used width (the block with the "However, it is
suggested that" comment), pass that to computeReplacedLogicalHeight only to
round-trip around the ratio in this case. Which may be fine, though...

Applied this thought to a revised CL. Don't have a test for it yet.

> 
> Btw, have you investigated letting
LayoutReplaced::constrainIntrinsicSizeToMinMax() handle this case?
Where it has an intrinsic width, I do - I use constrainedSize.width() as the
estimated width. Is that what you mean?

[davve, 2016-04-20 13:26:54]

On 2016/04/19 20:14:44, rhogan wrote:

> > Yes... Which brings us to the possibility of depending on width and the
> estimated used width. My worry here is that we have to duplicate the entire
> following block in estimating used width (the block with the "However, it is
> suggested that" comment), pass that to computeReplacedLogicalHeight only to
> round-trip around the ratio in this case. Which may be fine, though...
> 
> Applied this thought to a revised CL. Don't have a test for it yet.

I have nothing further. LGTM.

[rhogan, 2016-04-20 18:07:17]

On 2016/04/20 at 13:26:54, davve wrote:
> On 2016/04/19 20:14:44, rhogan wrote:
> 
> > > Yes... Which brings us to the possibility of depending on width and the
> > estimated used width. My worry here is that we have to duplicate the entire
> > following block in estimating used width (the block with the "However, it is
> > suggested that" comment), pass that to computeReplacedLogicalHeight only to
> > round-trip around the ratio in this case. Which may be fine, though...
> > 
> > Applied this thought to a revised CL. Don't have a test for it yet.
> 
> I have nothing further. LGTM.

So cbiesinger: OK to land?

[cbiesinger, 2016-04-21 17:41:34]

On 2016/04/20 18:07:17, rhogan wrote:
> On 2016/04/20 at 13:26:54, davve wrote:
> > On 2016/04/19 20:14:44, rhogan wrote:
> > 
> > > > Yes... Which brings us to the possibility of depending on width and the
> > > estimated used width. My worry here is that we have to duplicate the
entire
> > > following block in estimating used width (the block with the "However, it
is
> > > suggested that" comment), pass that to computeReplacedLogicalHeight only
to
> > > round-trip around the ratio in this case. Which may be fine, though...
> > > 
> > > Applied this thought to a revised CL. Don't have a test for it yet.
> > 
> > I have nothing further. LGTM.
> 
> So cbiesinger: OK to land?

lgtm

[rhogan, 2016-04-21 17:42:07]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2016-04-21 17:42:33]

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

[commit-bot: I haz the power, 2016-04-21 20:33:16]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-22 19:38:41]

Description was changed from

==========
Percent height in auto height containers no longer computes as auto

https://drafts.csswg.org/css2/visudet.html#the-height-property has changed.

See http://lists.w3.org/Archives/Public/www-style/2013May/0201.html

The section on percentage now reads:
 "<percentage>
 Specifies a percentage height. The percentage is calculated with respect to the
 height of the generated box's containing block. If the height of the containing
 block is not specified explicitly (i.e., it depends on content height), and
this
 element is not absolutely positioned, the used height is calculated as if
'auto'
 was specified."

The last clause used to read: "the value computes to 'auto'."

So remove our attempt to treat percentage height in auto height containers as
computed in RenderReplaced::computeReplacedLogicalWidth() and only treat actual
computed style of auto as such.

Once we remove this feature we find that when calculating used width for
replaced
elements we can end up in a situation where we need to factor in the 'used
height'
to our calculation. In certain circumstances, calculating the 'used height'
requires the 'used width'. :) When this situation arises, use an estimate of the
used width based on the intrinsic width of the replaced object if it's available
and failing that use the width made available by its container.

This circularity always existed I think and we always had to assume the 'used'
width in this situation - it's just easier to surface now.

BUG=396717
==========

to

==========
Percent height in auto height containers no longer computes as auto

https://drafts.csswg.org/css2/visudet.html#the-height-property has changed.

See http://lists.w3.org/Archives/Public/www-style/2013May/0201.html

The section on percentage now reads:
 "<percentage>
 Specifies a percentage height. The percentage is calculated with respect to the
 height of the generated box's containing block. If the height of the containing
 block is not specified explicitly (i.e., it depends on content height), and
this
 element is not absolutely positioned, the used height is calculated as if
'auto'
 was specified."

The last clause used to read: "the value computes to 'auto'."

So remove our attempt to treat percentage height in auto height containers as
computed in RenderReplaced::computeReplacedLogicalWidth() and only treat actual
computed style of auto as such.

Once we remove this feature we find that when calculating used width for
replaced
elements we can end up in a situation where we need to factor in the 'used
height'
to our calculation. In certain circumstances, calculating the 'used height'
requires the 'used width'. :) When this situation arises, use an estimate of the
used width based on the intrinsic width of the replaced object if it's available
and failing that use the width made available by its container.

This circularity always existed I think and we always had to assume the 'used'
width in this situation - it's just easier to surface now.

BUG=396717

Committed: https://crrev.com/4f61ad0df953092181eb059d6c119893e4202d34
Cr-Commit-Position: refs/heads/master@{#388878}
==========

[commit-bot: I haz the power, 2016-04-22 19:38:43]

Patchset 5 (id:??) landed as
https://crrev.com/4f61ad0df953092181eb059d6c119893e4202d34
Cr-Commit-Position: refs/heads/master@{#388878}