[css-flexbox] Don't always reuse cached layout for row flex containers

CalculateSizeBasedLayoutCacheStatusWithGeometry checks if the cached layout information can be reused. The problem was that, if a flex container had a percentage height, and the size of its containing block changed, a new layout would only be performed in case it was a column flex container. For row flex containers, the cached layout would be reused. However, row containers may also need a new layout in some cases. This patch removes the check for column flex containers, so that row ones can be included too. Bug: 1103592 TEST=external/wpt/css/css-flexbox/height-percentage-with-dynamic-container-size.html Change-Id: Ib2c119febdfa56b31f5524be3df85c9749dd1f6a Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2302997 Commit-Queue: Oriol Brufau <obrufau@igalia.com> Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org> Cr-Commit-Position: refs/heads/master@{#789788}

[css-flexbox] Don't always reuse cached layout for row flex containers
CalculateSizeBasedLayoutCacheStatusWithGeometry checks if the cached layout information can be reused. The problem was that, if a flex container had a percentage height, and the size of its containing block changed, a new layout would only be performed in case it was a column flex container. For row flex containers, the cached layout would be reused. However, row containers may also need a new layout in some cases. This patch removes the check for column flex containers, so that row ones can be included too. Bug: 1103592 TEST=external/wpt/css/css-flexbox/height-percentage-with-dynamic-container-size.html Change-Id: Ib2c119febdfa56b31f5524be3df85c9749dd1f6a Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2302997 Commit-Queue: Oriol Brufau <obrufau@igalia.com> Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org> Cr-Commit-Position: refs/heads/master@{#789788}
1c6c2396 · Oriol Brufau · Commit Bot · 855048e5 · 1c6c2396 · 1c6c2396
Commit 1c6c2396 authored Jul 19, 2020 by Oriol Brufau Committed by Commit Bot Jul 19, 2020
2 changed files
--- a/third_party/blink/renderer/core/layout/ng/ng_layout_utils.cc
+++ b/third_party/blink/renderer/core/layout/ng/ng_layout_utils.cc
@@ -212,14 +212,17 @@ NGLayoutCacheStatus CalculateSizeBasedLayoutCacheStatusWithGeometry(
      if (old_space.IsFixedBlockSize())
        return NGLayoutCacheStatus::kNeedsLayout;

-      // The intrinsic size of column flex-boxes can depend on the
-      // %-resolution-block-size. This occurs when a flex-box has "max-height:
-      // 100%" or similar on itself.
+      // The intrinsic size of flex-boxes can depend on the %-block-size. This
+      // occurs when:
+      //  - A column flex-box has "max-height: 100%" (or similar) on itself.
+      //  - A row flex-box has "height: 100%" (or similar) and children which
+      //    stretch to this size.
      //
      // Due to this we can't use cached |NGLayoutResult::IntrinsicBlockSize|
      // value, as the following |block_size| calculation would be incorrect.
-      if (style.ResolvedIsColumnFlexDirection() &&
-          layout_result.PhysicalFragment().DependsOnPercentageBlockSize()) {
+      // TODO(dgrogan): We can hit the cache here for row flexboxes when they
+      // don't have stretchy children.
+      if (layout_result.PhysicalFragment().DependsOnPercentageBlockSize()) {
        if (new_space.PercentageResolutionBlockSize() !=
            old_space.PercentageResolutionBlockSize())
          return NGLayoutCacheStatus::kNeedsLayout;

--- a/third_party/blink/web_tests/external/wpt/css/css-flexbox/height-percentage-with-dynamic-container-size.html
+++ b/third_party/blink/web_tests/external/wpt/css/css-flexbox/height-percentage-with-dynamic-container-size.html
+<!DOCTYPE html>
+<meta charset="utf-8">
+<title>CSS Flexbox Test: Flex container with height percentage and dynamic container size</title>
+<link rel="author" title="Oriol Brufau" href="mailto:obrufau@igalia.com">
+<link rel="help" href="http://www.w3.org/TR/css-flexbox-1">
+<meta name="assert" content="Checks that if a flex container has a definite height percentage, but then the height of the container is dynamically changed to be intrinsic, the resolved value of the percentage is correctly updated.">
+<style>
+.container {
+  width: 100px;
+  background: cyan;
+  height: 200px;
+}
+.changed .container {
+  height: auto;
+}
+.flex {
+  display: flex;
+}
+.content {
+  height: 100px;
+  width: 100px;
+}
+</style>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="/resources/check-layout-th.js"></script>
+
+<div id="log"></div>
+
+<pre>height: 0%</pre>
+
+<div class="container" data-expected-height="100">
+  <div class="flex" style="height: 0%" data-expected-height="100">
+    <div data-expected-height="100">
+      <div class="content"></div>
+    </div>
+  </div>
+</div>
+
+<pre>height: 100%</pre>
+
+<div class="container" data-expected-height="100">
+  <div class="flex" style="height: 100%" data-expected-height="100">
+    <div data-expected-height="100">
+      <div class="content"></div>
+    </div>
+  </div>
+</div>
+
+<pre>height: 200%</pre>
+
+<div class="container" data-expected-height="100">
+  <div class="flex" style="height: 200%" data-expected-height="100">
+    <div data-expected-height="100">
+      <div class="content"></div>
+    </div>
+  </div>
+</div>
+
+<script>
+// Force layout
+document.body.offsetLeft;
+
+// Change 'height' to 'auto'.
+document.body.classList.add("changed");
+
+// Check final layout
+checkLayout('.container');
+</script>