[LayoutNG] Force legacy layout on the entire subtree when editable.

When we decide to switch over to legacy layout for a block formatting context (and the entire subtree) because there's something editable, we used to only set ForceLegacyLayout on those nodes that create layout objects, but that isn't enough. There may be descendants of box-less nodes that may create layout objects later on, and, since ForceLegacyLayout is an inherited "property", we need to set it on all potential parent nodes. Otherwise we risk inserting NG objects that participate in a legacy block formatting context, which isn't allowed [1]. This matters when we have display:contents (which is very common situation for shadow DOM slot elements, for instance). Checking with the layout parent's style whether to force legacy layout or not fixes the problem. [1] https://chromium-review.googlesource.com/c/chromium/src/+/1127027 enforces this. Bug: 863040 Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng Change-Id: I6133c20493017b4648b153f2c7558c76589c3d89 Reviewed-on: https://chromium-review.googlesource.com/1136445Reviewed-by: Yoshifumi Inoue <yosin@chromium.org> Reviewed-by: Emil A Eklund <eae@chromium.org> Reviewed-by: Rune Lillesveen <futhark@chromium.org> Commit-Queue: Morten Stenshorne <mstensho@chromium.org> Cr-Commit-Position: refs/heads/master@{#579360}

[LayoutNG] Force legacy layout on the entire subtree when editable.
When we decide to switch over to legacy layout for a block formatting context (and the entire subtree) because there's something editable, we used to only set ForceLegacyLayout on those nodes that create layout objects, but that isn't enough. There may be descendants of box-less nodes that may create layout objects later on, and, since ForceLegacyLayout is an inherited "property", we need to set it on all potential parent nodes. Otherwise we risk inserting NG objects that participate in a legacy block formatting context, which isn't allowed [1]. This matters when we have display:contents (which is very common situation for shadow DOM slot elements, for instance). Checking with the layout parent's style whether to force legacy layout or not fixes the problem. [1] https://chromium-review.googlesource.com/c/chromium/src/+/1127027 enforces this. Bug: 863040 Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng Change-Id: I6133c20493017b4648b153f2c7558c76589c3d89 Reviewed-on: https://chromium-review.googlesource.com/1136445Reviewed-by: Yoshifumi Inoue <yosin@chromium.org> Reviewed-by: Emil A Eklund <eae@chromium.org> Reviewed-by: Rune Lillesveen <futhark@chromium.org> Commit-Queue: Morten Stenshorne <mstensho@chromium.org> Cr-Commit-Position: refs/heads/master@{#579360}
5e33dcbe · Morten Stenshorne · Commit Bot · e3e1ca1e · 5e33dcbe · 5e33dcbe
Commit 5e33dcbe authored Jul 31, 2018 by Morten Stenshorne Committed by Commit Bot Jul 31, 2018
3 changed files
--- a/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
+++ b/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
@@ -360,7 +360,6 @@ crbug.com/591099 external/wpt/html/browsers/history/joint-session-history/joint-
 crbug.com/591099 external/wpt/html/browsers/the-window-object/window-open-noopener.html?_parent [ Timeout ]
 crbug.com/591099 external/wpt/html/browsers/the-window-object/window-open-noopener.html?_self [ Timeout ]
 crbug.com/591099 external/wpt/html/editing/focus/sequential-focus-navigation-and-the-tabindex-attribute/focus-tabindex-positive.html [ Timeout ]
-crbug.com/863040 external/wpt/html/editing/focus/tabindex-focus-flag.html [ Crash ]
 crbug.com/591099 external/wpt/html/infrastructure/safe-passing-of-structured-data/shared-array-buffers/serialization-via-idb.any.worker.html [ Failure ]
 crbug.com/591099 external/wpt/html/infrastructure/safe-passing-of-structured-data/shared-array-buffers/window-domain-success.sub.html [ Failure ]
 crbug.com/591099 external/wpt/html/rendering/non-replaced-elements/the-hr-element-0/color.html [ Failure ]

--- a/third_party/WebKit/LayoutTests/fast/block/margin-collapse/display-contents-contenteditable.html
+++ b/third_party/WebKit/LayoutTests/fast/block/margin-collapse/display-contents-contenteditable.html
+<!DOCTYPE html>
+<p>There should be a green square below.</p>
+<div id="container" style="overflow:hidden; width:100px; background:green;" data-expected-height="100">
+  <div style="display:contents;">
+    <div id="initiallyNone" style="display:none; margin:100px 0;">
+      <div style="margin:100px 0;"></div>
+    </div>
+  </div>
+  <div style="margin:100px 0;">
+    <div style="margin:100px 0;"></div>
+  </div>
+  <div contenteditable="true" style="height:0;"></div>
+</div>
+<script src="../../../resources/testharness.js"></script>
+<script src="../../../resources/testharnessreport.js"></script>
+<script src="../../../resources/check-layout-th.js"></script>
+<script>
+  document.body.offsetTop;
+  document.getElementById("initiallyNone").style.display = "block";
+  checkLayout("#container");
+</script>
--- a/third_party/blink/renderer/core/css/resolver/style_adjuster.cc
+++ b/third_party/blink/renderer/core/css/resolver/style_adjuster.cc
@@ -90,11 +90,20 @@ bool IsImageOrVideoElement(const Element* element) {
 }

 bool ShouldForceLegacyLayout(const ComputedStyle& style,
+                             const ComputedStyle& layout_parent_style,
                             const Element& element) {
  // Form controls are not supported yet.
  if (element.ShouldForceLegacyLayout())
    return true;

+  // When the actual parent (to inherit style from) doesn't have a layout box
+  // (display:contents), it may not have been switched over to forcing legacy
+  // layout, even if it's inside a subtree that should use legacy. Check with
+  // the layout parent as well, so that we don't risk switching back to LayoutNG
+  // when we shouldn't.
+  if (layout_parent_style.ForceLegacyLayout())
+    return true;
+
  // TODO(layout-dev): Once LayoutNG handles inline content editable, we
  // should get rid of following code fragment.
  const Document& document = element.GetDocument();
@@ -711,7 +720,8 @@ void StyleAdjuster::AdjustComputedStyle(StyleResolverState& state,
  }

  if (RuntimeEnabledFeatures::LayoutNGEnabled() && !style.ForceLegacyLayout() &&
-      element && ShouldForceLegacyLayout(style, *element)) {
+      element &&
+      ShouldForceLegacyLayout(style, layout_parent_style, *element)) {
    style.SetForceLegacyLayout(true);
  }