When a positioned object's dependence on content height changes we need to layout.

When a positioned object's dependence on content height changes we need to layout.

If top or bottom become auto/non-auto then it means we either have to solve height based
on the content or stop doing so (http://www.w3.org/TR/CSS2/visudet.html#abs-non-replaced-height) 
- either way requires a layout. This is a partial rowback on the code removed in https://codereview.chromium.org/722643002.

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

[rhogan, 2015-01-28 19:21:20]

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

[pdr., 2015-01-30 00:02:20]

pdr@chromium.org changed reviewers:
+ eae@chromium.org, jchaffraix@chromium.org
- pdr@chromium.org

[pdr., 2015-01-30 00:02:37]

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

[pdr., 2015-01-30 00:02:38]

Emil or Julien, could you take a peek at this?

[mstensho (USE GERRIT), 2015-01-30 09:28:46]

mstensho@opera.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2015-01-30 09:28:46]

This could certainly work, but first I'd like to figure out why layout doesn't
deal with this properly (which https://codereview.chromium.org/722643002 claims
that it should). It works fine when you do the same thing with left and right
(e.g. left:0; right:50% and then change right to 'auto').

Looks like RenderBlock::tryLayoutDoingPositionedMovementOnly() (called from
simplifiedLayout()) is to blame. It's good at detecting width changes, but not
so good with heights.

It used to just do an updateLogicalHeight(), and then return true (prior to
https://codereview.chromium.org/802813003 ), but now it's conditioned on
updateLogicalHeight() resulting in a height change. But there'll be no height
change detected in this case, because updateLogicalHeight() just calls
computeLogicalHeight(). computeLogicalHeight() will return the same height,
because it's now auto, so it will think that the current height is based on
contents, but it's actually a stale value from back when height wasn't auto
(because back then, the combination of the top and bottom properties was enough
to resolve the height).

[rhogan, 2015-01-30 19:25:24]

On 2015/01/30 at 09:28:46, mstensho wrote:
> This could certainly work, but first I'd like to figure out why layout doesn't
deal with this properly (which https://codereview.chromium.org/722643002 claims
that it should). It works fine when you do the same thing with left and right
(e.g. left:0; right:50% and then change right to 'auto').
> 
> Looks like RenderBlock::tryLayoutDoingPositionedMovementOnly() (called from
simplifiedLayout()) is to blame. It's good at detecting width changes, but not
so good with heights.
> 
> It used to just do an updateLogicalHeight(), and then return true (prior to
https://codereview.chromium.org/802813003 ), but now it's conditioned on
updateLogicalHeight() resulting in a height change. But there'll be no height
change detected in this case, because updateLogicalHeight() just calls
computeLogicalHeight(). computeLogicalHeight() will return the same height,
because it's now auto, so it will think that the current height is based on
contents, but it's actually a stale value from back when height wasn't auto
(because back then, the combination of the top and bottom properties was enough
to resolve the height).

That's it in a nutshell. A full layout resets the block's height to zero,
calculates the height of its contents, then performs updateLogicalHeight() with
that information. So we still need to do that full height calculation for a
positioned object if it is coming from or going a set of offset values that
requires the height of the contents to be known.

[rhogan, 2015-01-30 19:37:45]

On 2015/01/30 at 19:25:24, rhogan wrote:
> On 2015/01/30 at 09:28:46, mstensho wrote:
> > This could certainly work, but first I'd like to figure out why layout
doesn't deal with this properly (which https://codereview.chromium.org/722643002
claims that it should). It works fine when you do the same thing with left and
right (e.g. left:0; right:50% and then change right to 'auto').
> > 
> > Looks like RenderBlock::tryLayoutDoingPositionedMovementOnly() (called from
simplifiedLayout()) is to blame. It's good at detecting width changes, but not
so good with heights.
> > 
> > It used to just do an updateLogicalHeight(), and then return true (prior to
https://codereview.chromium.org/802813003 ), but now it's conditioned on
updateLogicalHeight() resulting in a height change. But there'll be no height
change detected in this case, because updateLogicalHeight() just calls
computeLogicalHeight(). computeLogicalHeight() will return the same height,
because it's now auto, so it will think that the current height is based on
contents, but it's actually a stale value from back when height wasn't auto
(because back then, the combination of the top and bottom properties was enough
to resolve the height).
> 
> That's it in a nutshell. A full layout resets the block's height to zero,
calculates the height of its contents, then performs updateLogicalHeight() with
that information. So we still need to do that full height calculation for a
positioned object if it is coming from or going a set of offset values that
requires the height of the contents to be known.

Specifically, see
https://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderBox.cpp
c.3369. Unless we are in a full layout and have reset logicalHeight() to
borderAndPadding() 'content height' will be the stale logicalHeight().

[mstensho (USE GERRIT), 2015-02-02 10:00:48]

https://codereview.chromium.org/882173002/diff/20001/LayoutTests/fast/block/p...
File
LayoutTests/fast/block/positioning/positioned-movement-layout-when-bottom-changes-to-and-from-auto.html
(right):

https://codereview.chromium.org/882173002/diff/20001/LayoutTests/fast/block/p...
LayoutTests/fast/block/positioning/positioned-movement-layout-when-bottom-changes-to-and-from-auto.html:16:

Superfluous blank line.

https://codereview.chromium.org/882173002/diff/20001/LayoutTests/fast/block/p...
LayoutTests/fast/block/positioning/positioned-movement-layout-when-bottom-changes-to-and-from-auto.html:21:
test.style['bottom'] = '50%';
Any reason why you shouldn't just let it be '50%' initially and only change it
to 'auto' here?

https://codereview.chromium.org/882173002/diff/20001/LayoutTests/fast/block/p...
LayoutTests/fast/block/positioning/positioned-movement-layout-when-bottom-changes-to-and-from-auto.html:28:
<div id="test" data-expected-height=0>
Strange indentation.

Maybe you could also add a child with absolute height?

https://codereview.chromium.org/882173002/diff/20001/LayoutTests/fast/block/p...
LayoutTests/fast/block/positioning/positioned-movement-layout-when-bottom-changes-to-and-from-auto.html:30:
Tests positioned movement layout when its height is changed by top/bottom
changing from auto to fixed and back again.
If you agree to make the change that I suggested above, you need to update this
text as well. Would probably also look nice if you move this above the DIV.

https://codereview.chromium.org/882173002/diff/20001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyle.cpp (right):

https://codereview.chromium.org/882173002/diff/20001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.cpp:363: if (a.top().isAuto() !=
b.top().isAuto() || a.bottom().isAuto() != b.bottom().isAuto())
Just "return blablabla", instead of "if (blablabla) return true; return false"?

But shouldn't you really be using logicalTop() and logicalBottom() here? Adding
a test for vertical writing mode would be nice, in any case.

Also, this may result in false positives (mark for layout when it's not really
needed) in over-constrained situations (when height != auto), but I don't think
we need to worry about that.

[rhogan, 2015-02-02 19:22:49]

https://codereview.chromium.org/882173002/diff/20001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyle.cpp (right):

https://codereview.chromium.org/882173002/diff/20001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.cpp:363: if (a.top().isAuto() !=
b.top().isAuto() || a.bottom().isAuto() != b.bottom().isAuto())
On 2015/02/02 at 10:00:47, mstensho wrote:

> Also, this may result in false positives (mark for layout when it's not really
needed) in over-constrained situations (when height != auto), but I don't think
we need to worry about that.

We would still need to resolve top/bottom in computeLogicalHeight if they were
becoming auto and the height was non-auto. So maybe not *that* many false
positives. :)

[mstensho (USE GERRIT), 2015-02-02 19:34:03]

lgtm

https://codereview.chromium.org/882173002/diff/40001/LayoutTests/fast/block/p...
File
LayoutTests/fast/block/positioning/positioned-movement-layout-when-bottom-changes-to-and-from-auto.html
(right):

https://codereview.chromium.org/882173002/diff/40001/LayoutTests/fast/block/p...
LayoutTests/fast/block/positioning/positioned-movement-layout-when-bottom-changes-to-and-from-auto.html:8:
background-color: green;
Without the changes I suggested, regarding adding a child to stretch the final
height of #test (rather than ending up with zero), it's pretty meaningless to
have a green background color here. Green will only appear if the test fails. :)

[rhogan, 2015-02-03 18:47:10]

On 2015/02/02 at 19:34:03, mstensho wrote:
> lgtm
> 
>
https://codereview.chromium.org/882173002/diff/40001/LayoutTests/fast/block/p...
> File
LayoutTests/fast/block/positioning/positioned-movement-layout-when-bottom-changes-to-and-from-auto.html
(right):
> 
>
https://codereview.chromium.org/882173002/diff/40001/LayoutTests/fast/block/p...
>
LayoutTests/fast/block/positioning/positioned-movement-layout-when-bottom-changes-to-and-from-auto.html:8:
background-color: green;
> Without the changes I suggested, regarding adding a child to stretch the final
height of #test (rather than ending up with zero), it's pretty meaningless to
have a green background color here. Green will only appear if the test fails. :)

Oh, that's why - I wasn't sure why you wanted that. I will address it in a
followup and TBR you. I don't want to clobber my local tree.

[rhogan, 2015-02-03 18:47:18]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2015-02-03 18:48:09]

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

[commit-bot: I haz the power, 2015-02-03 18:51:53]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189434