Add NeedsPreferredWidthsRecalculation() for block

Add NeedsPreferredWidthsRecalculation() for block

In the case the block has relative height and width is auto and
it has an image which should keep the aspect ratio,
it needs preferred width recalculation if the parent changes the height.
To cover this, add NeedsPreferredWidthsRecalculation() for LayoutBlock.

BUG=707151, 514663, 709233
Review-Url: https://codereview.chromium.org/2828453002
Cr-Commit-Position: refs/heads/master@{#466934}
Committed: https://chromium.googlesource.com/chromium/src/+/76e307f44292b2999535aa8c5605cfd9129e9c12

[Sunghoon Kim, 2017-04-18 05:05:18]

shoon.kim@lge.com changed reviewers:
+ fs@opera.com, schenney@chromium.org

[Sunghoon Kim, 2017-04-18 05:05:19]

Could you please to review this?
Thanks.

[fs, 2017-04-18 08:01:25]

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

[fs, 2017-04-18 08:01:25]

+mstensho

[rhogan, 2017-04-18 18:46:29]

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

[rhogan, 2017-04-18 18:46:30]

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

https://codereview.chromium.org/2828453002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:2942:
child.SetPreferredLogicalWidthsDirty(kMarkContainerChain);
If someone called SetPreferredLogicalWidthsDirty on child, why didn't they mark
it with kMarkContainerChain at the time? I think that's the place to
investigate.

[Sunghoon Kim, 2017-04-19 00:51:30]

If the block is inline-block and the preferred width of child needs to be
recalculated, the preferred width of inline-block also should be recalculated.
So, I changed the code as you commented.
Thanks.

[Sunghoon Kim, 2017-04-19 04:12:00]

Description was changed from

==========
Make inline-block container's width update

Make inline-block container's preferred width update
if its child's preferred width needs to be recalculated.

BUG=707151, 692874
==========

to

==========
Make inline-block container's width update

Make inline-block container's preferred width update
if its child's preferred width needs to be recalculated.

BUG=707151, 514663
==========

[mstensho (USE GERRIT), 2017-04-19 10:58:47]

https://codereview.chromium.org/2828453002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/inline-block/inline-block-update-width.html
(right):

https://codereview.chromium.org/2828453002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/inline-block/inline-block-update-width.html:13:
width: auto;
width and height are initially auto, so no need to be explicit about it.

https://codereview.chromium.org/2828453002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/inline-block/inline-block-update-width.html:31:
<script src="../../resources/check-layout.js"></script>
Please consider using testharness when writing check-layout tests. I.e. include
the following scripts instead:

<script src="../../resources/testharness.js"></script>
<script src="../../resources/testharnessreport.js"></script>
<script src="../../resources/check-layout-th.js"></script>

That will eliminate the need for the -expected.txt counterpart.

https://codereview.chromium.org/2828453002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/inline-block/inline-block-update-width.html:34:
function runTest() {
Seems that you can just do all these things right away, rather than hooking them
on an onload event. So no need for the wrapper function.

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

https://codereview.chromium.org/2828453002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:610:
IsInlineBlockOrInlineTable() ? kMarkContainerChain : kMarkOnlyThis);
Marking the container chain during layout seems wrong. This situation needs to
be detected at the beginning of layout of the shrink-to-fit container, not when
laying out its children.

Furthermore:

1. The inline-block may not be the immediate parent of the child that
NeedsPreferredWidthsRecalculation().

And:

2. inline-block isn't the only characteristic we should check for. Floats with
auto width will have similar issues. Table cells too, probably. And possibly
more.

Here's a test that demonstrates both #1 and #2. The suggested CL doesn't fix it:

<!DOCTYPE html>
<p>No red should be seen.</p>
<div id="parent" style="height:300px; background:green;">
    <div id="stf" style="float:left; height:100%; background:red;">
        <div id="middle" style="height:100%;">
            <img style="display:block; height:100%; background:yellow;"
src='data:image/svg+xml;utf8,<svg height="2px" width="1px"
xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
version="1.1"></svg>'></img>
        </div>
    </div>
</div>
<script>
    document.body.offsetTop;
    document.getElementById("parent").style.height = "150px";
</script>

The object that needs to have its preferred logical widths marked dirty is #stf
(it's a float, not an inline-block). The parent of the object that
NeedsPreferredWidthsRecalculation() is #middle.

[Yiorsi, 2017-04-20 05:36:40]

What about inline-flex(-webkit-inline-box) or inline-grid?

[Sunghoon Kim, 2017-04-20 06:50:25]

Description was changed from

==========
Make inline-block container's width update

Make inline-block container's preferred width update
if its child's preferred width needs to be recalculated.

BUG=707151, 514663
==========

to

==========
Add NeedsPreferredWidthRecalculation() for block

In the case the block has relative height and width is auto and
it has an mage which should keep the aspect ratio,
it needs preferred width recalculation if the parent changes the height.
To cover this, add NeedsPreferredWidthRecalculation() for LayoutBlock.

BUG=707151, 514663
==========

[Sunghoon Kim, 2017-04-20 23:19:14]

The problem was, i think, the block which has relative height and width auto was
not resized when the parent changed its height if the child of the block had
changed its size.
Here, override NeedsPreferredWidthCalculation() and make preferred width to be
recalculated when the block has relative height and width auto like
LayoutReplaced.

Could you review this CL?
Thanks.

[mstensho (USE GERRIT), 2017-04-24 09:08:35]

This looks like an excellent solution!

Before landing, though:

Could you proof-read the commit message? It's not called
"NeedsPreferredWidthRecalculation()", but rather
"NeedsPreferredWidthsRecalculation()". Also, "mage" -> "image"

Could you also add another, slightly more evil, test, which uses nested blocks
with percentage height, e.g. like in my example in #msg9 .

[Sunghoon Kim, 2017-04-25 00:21:38]

Description was changed from

==========
Add NeedsPreferredWidthRecalculation() for block

In the case the block has relative height and width is auto and
it has an mage which should keep the aspect ratio,
it needs preferred width recalculation if the parent changes the height.
To cover this, add NeedsPreferredWidthRecalculation() for LayoutBlock.

BUG=707151, 514663
==========

to

==========
Add NeedsPreferredWidthsRecalculation() for block

In the case the block has relative height and width is auto and
it has an image which should keep the aspect ratio,
it needs preferred width recalculation if the parent changes the height.
To cover this, add NeedsPreferredWidthsRecalculation() for LayoutBlock.

BUG=707151, 514663
==========

[Sunghoon Kim, 2017-04-25 00:23:34]

Thanks for your review.
I added nested block case in the tc and tc moved to fast/block.
Also, i modified the commit message as you commented.

Any other comments are welcome.

[Sunghoon Kim, 2017-04-25 02:50:50]

Description was changed from

==========
Add NeedsPreferredWidthsRecalculation() for block

In the case the block has relative height and width is auto and
it has an image which should keep the aspect ratio,
it needs preferred width recalculation if the parent changes the height.
To cover this, add NeedsPreferredWidthsRecalculation() for LayoutBlock.

BUG=707151, 514663
==========

to

==========
Add NeedsPreferredWidthsRecalculation() for block

In the case the block has relative height and width is auto and
it has an image which should keep the aspect ratio,
it needs preferred width recalculation if the parent changes the height.
To cover this, add NeedsPreferredWidthsRecalculation() for LayoutBlock.

BUG=707151, 514663, 709233
==========

[mstensho (USE GERRIT), 2017-04-25 07:52:51]

lgtm with nits.

https://codereview.chromium.org/2828453002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/block/block-width-recalc-with-relative-height.html
(right):

https://codereview.chromium.org/2828453002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/block/block-width-recalc-with-relative-height.html:28:
<img src='data:image/svg+xml;utf8,<svg height="2px" width="1px"
xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
version="1.1"></svg>'></img>
IMG doesn't need an end tag.

https://codereview.chromium.org/2828453002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/block/block-width-recalc-with-relative-height.html:36:
<img src='data:image/svg+xml;utf8,<svg height="2px" width="1px"
xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
version="1.1"></svg>'></img>
Ditto.

https://codereview.chromium.org/2828453002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/block/block-width-recalc-with-relative-height.html:54:
checkLayout("#middle", results);
You can omit this. checkLayout() processes the entire subtree, so #middle is
already tested as part of testing #stylefloat.

[Sunghoon Kim, 2017-04-25 08:06:56]

Thanks for review.
Ran CQ.

[Sunghoon Kim, 2017-04-25 08:07:07]

The CQ bit was checked by shoon.kim@lge.com

[Sunghoon Kim, 2017-04-25 08:07:08]

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/2828453002/#ps120001
(title: "Add NeedsPreferredWidthsRecalculation() for block")

[commit-bot: I haz the power, 2017-04-25 08:07:23]

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, 2017-04-25 09:37:44]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1493107627800340,
"parent_rev": "9e8f21af25c5425136583d7658850e4a7db2146e", "commit_rev":
"76e307f44292b2999535aa8c5605cfd9129e9c12"}

[commit-bot: I haz the power, 2017-04-25 09:37:54]

Description was changed from

==========
Add NeedsPreferredWidthsRecalculation() for block

In the case the block has relative height and width is auto and
it has an image which should keep the aspect ratio,
it needs preferred width recalculation if the parent changes the height.
To cover this, add NeedsPreferredWidthsRecalculation() for LayoutBlock.

BUG=707151, 514663, 709233
==========

to

==========
Add NeedsPreferredWidthsRecalculation() for block

In the case the block has relative height and width is auto and
it has an image which should keep the aspect ratio,
it needs preferred width recalculation if the parent changes the height.
To cover this, add NeedsPreferredWidthsRecalculation() for LayoutBlock.

BUG=707151, 514663, 709233

Review-Url: https://codereview.chromium.org/2828453002
Cr-Commit-Position: refs/heads/master@{#466934}
Committed:
https://chromium.googlesource.com/chromium/src/+/76e307f44292b2999535aa8c5605...
==========

[commit-bot: I haz the power, 2017-04-25 09:37:55]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/76e307f44292b2999535aa8c5605...