Wrap text child of display:contents in anonymous if necessary.

A text node gets its style from its flat tree parent. The problem is
that our layout code expects the LayoutObject parent to have the same
inherited styles as the text node. That is not the case if a text node
has display:contents ancestors and the display:contents parent does not
have the same inherited styles as the closest ancestor generating a box.

Example:

  <div>
    <div style="display:contents;font-size:50px">Text</div>
  </div>

We're solving this by inserting an anonymous inline between the
LayoutText and its parent LayoutObject. We do not try to merge inline
wrappers for subsequent text nodes, so for:

  <div style="display:contents:color:pink">A<!-- -->B</div>

we create one wrapper for each of A and B, even though one would
suffice.

We attach and detach these wrappers on-demand, so if, for instance, the
inherited computed styles of the display:contents change so that it
matches the computed styles of the LayoutObject parent, we detach the
wrapper as it's no longer needed.

We also currently detach the wrapper when the computed style of the
display:contents and hence the computed style of the wrapper changes. We
could have optimized this through more checking and SetStyle on the
anonymous wrapper.

Bug: 717460, 706316, 709808, 713019
Change-Id: Ia53b9fe2c0a6067c4600dab49cdf43f23b95b8fa
Tests: see removed lines from TestExpectations.
Reviewed-on: https://chromium-review.googlesource.com/806158
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Robert Hogan <robhogan@gmail.com>
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521673}

third_party/WebKit/LayoutTests/TestExpectations

@@ -2906,10 +2906,6 @@ crbug.com/786249 http/tests/inspector-protocol/network/response-interception-req
 # ====== Begin of display: contents tests ======
 
 crbug.com/181374 external/wpt/css/css-display/display-contents-dynamic-table-001-inline.html [ Failure ]
-crbug.com/717460 external/wpt/css/css-display/display-contents-first-letter-002.html [ Failure ]
-crbug.com/706316 external/wpt/css/css-display/display-contents-first-line-002.html [ Failure ]
-crbug.com/713019 external/wpt/css/css-display/display-contents-line-height.html [ Failure ]
-crbug.com/709808 external/wpt/css/css-display/display-contents-text-inherit.html [ Failure ]
 
 # ====== End of display: contents tests ======
 

third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp

@@ -36,6 +36,7 @@
 #include "core/fullscreen/Fullscreen.h"
 #include "core/html_names.h"
 #include "core/layout/LayoutFullScreen.h"
+#include "core/layout/LayoutInline.h"
 #include "core/layout/LayoutObject.h"
 #include "core/layout/LayoutText.h"
 #include "core/layout/LayoutView.h"
@@ -48,7 +49,7 @@ LayoutTreeBuilderForElement::LayoutTreeBuilderForElement(Element& element,
                                                          ComputedStyle* style)
     : LayoutTreeBuilder(element, nullptr), style_(style) {
   DCHECK(!element.IsActiveSlotOrActiveV0InsertionPoint());
-  // TODO(ecobos): Move the first-letter logic inside parentLayoutObject too?
+  // TODO(ecobos): Move the first-letter logic inside ParentLayoutObject too?
   // It's an extra (unnecessary) check for text nodes, though.
   if (element.IsFirstLetterPseudoElement()) {
     if (LayoutObject* next_layout_object =
@@ -74,7 +75,7 @@ LayoutObject* LayoutTreeBuilderForElement::NextLayoutObject() const {
 
 LayoutObject* LayoutTreeBuilderForElement::ParentLayoutObject() const {
   if (layout_object_parent_) {
-    // FIXME: Guarding this by parentLayoutObject isn't quite right as the spec
+    // FIXME: Guarding this by ParentLayoutObject isn't quite right as the spec
     // for top layer only talks about display: none ancestors so putting a
     // <dialog> inside an <optgroup> seems like it should still work even though
     // this check will prevent it.
@@ -123,7 +124,7 @@ void LayoutTreeBuilderForElement::CreateLayoutObject() {
 
   // Make sure the LayoutObject already knows it is going to be added to a
   // LayoutFlowThread before we set the style for the first time. Otherwise code
-  // using inLayoutFlowThread() in the styleWillChange and styleDidChange will
+  // using IsInsideFlowThread() in the StyleWillChange and StyleDidChange will
   // fail.
   new_layout_object->SetIsInsideFlowThread(
       parent_layout_object->IsInsideFlowThread());
@@ -131,7 +132,7 @@ void LayoutTreeBuilderForElement::CreateLayoutObject() {
   LayoutObject* next_layout_object = NextLayoutObject();
   node_->SetLayoutObject(new_layout_object);
   new_layout_object->SetStyle(
-      &style);  // setStyle() can depend on layoutObject() already being set.
+      &style);  // SetStyle() can depend on LayoutObject() already being set.
 
   if (Fullscreen::IsFullscreenElement(*node_)) {
     new_layout_object = LayoutFullScreen::WrapLayoutObject(
@@ -140,11 +141,34 @@ void LayoutTreeBuilderForElement::CreateLayoutObject() {
       return;
   }
 
-  // Note: Adding newLayoutObject instead of layoutObject(). layoutObject() may
-  // be a child of newLayoutObject.
+  // Note: Adding new_layout_object instead of LayoutObject(). LayoutObject()
+  // may be a child of new_layout_object.
   parent_layout_object->AddChild(new_layout_object, next_layout_object);
 }
 
+LayoutObject*
+LayoutTreeBuilderForText::CreateInlineWrapperForDisplayContentsIfNeeded() {
+  scoped_refptr<ComputedStyle> wrapper_style =
+      ComputedStyle::CreateInheritedDisplayContentsStyleIfNeeded(
+          *style_, layout_object_parent_->StyleRef());
+  if (!wrapper_style)
+    return nullptr;
+
+  // Text nodes which are children of display:contents element which modifies
+  // inherited properties, need to have an anonymous inline wrapper having those
+  // inherited properties because the layout code expects the LayoutObject
+  // parent of text nodes to have the same inherited properties.
+  LayoutObject* inline_wrapper =
+      LayoutInline::CreateAnonymous(&node_->GetDocument());
+  inline_wrapper->SetStyle(wrapper_style);
+  if (!layout_object_parent_->IsChildAllowed(inline_wrapper, *wrapper_style)) {
+    inline_wrapper->Destroy();
+    return nullptr;
+  }
+  layout_object_parent_->AddChild(inline_wrapper, NextLayoutObject());
+  return inline_wrapper;
+}
+
 void LayoutTreeBuilderForText::CreateLayoutObject() {
   ComputedStyle& style = *style_;
 
@@ -152,6 +176,14 @@ void LayoutTreeBuilderForText::CreateLayoutObject() {
          ToElement(LayoutTreeBuilderTraversal::Parent(*node_))
              ->HasDisplayContentsStyle());
 
+  LayoutObject* next_layout_object;
+  if (LayoutObject* wrapper = CreateInlineWrapperForDisplayContentsIfNeeded()) {
+    layout_object_parent_ = wrapper;
+    next_layout_object = nullptr;
+  } else {
+    next_layout_object = NextLayoutObject();
+  }
+
   LayoutText* new_layout_object = node_->CreateTextLayoutObject(style);
   if (!layout_object_parent_->IsChildAllowed(new_layout_object, style)) {
     new_layout_object->Destroy();
@@ -160,14 +192,12 @@ void LayoutTreeBuilderForText::CreateLayoutObject() {
 
   // Make sure the LayoutObject already knows it is going to be added to a
   // LayoutFlowThread before we set the style for the first time. Otherwise code
-  // using inLayoutFlowThread() in the styleWillChange and styleDidChange will
+  // using IsInsideFlowThread() in the StyleWillChange and StyleDidChange will
   // fail.
   new_layout_object->SetIsInsideFlowThread(
       layout_object_parent_->IsInsideFlowThread());
 
-  LayoutObject* next_layout_object = NextLayoutObject();
   node_->SetLayoutObject(new_layout_object);
-  // Parent takes care of the animations, no need to call setAnimatableStyle.
   new_layout_object->SetStyle(&style);
   layout_object_parent_->AddChild(new_layout_object, next_layout_object);
 }

third_party/WebKit/Source/core/dom/LayoutTreeBuilder.h

@@ -75,7 +75,19 @@ class LayoutTreeBuilder {
         layout_object_parent_->GetNode()->NeedsAttach())
       return nullptr;
 
-    return LayoutTreeBuilderTraversal::NextSiblingLayoutObject(*node_);
+    LayoutObject* next =
+        LayoutTreeBuilderTraversal::NextSiblingLayoutObject(*node_);
+
+    // If a text node is wrapped in an anonymous inline for display:contents
+    // (see CreateInlineWrapperForDisplayContents()), use the wrapper as the
+    // next layout object. Otherwise we would need to add code to various
+    // AddChild() implementations to walk up the tree to find the correct
+    // layout tree parent/siblings.
+    if (next && next->IsText() && next->Parent()->IsAnonymous() &&
+        next->Parent()->IsInline()) {
+      return next->Parent();
+    }
+    return next;
   }
 
   Member<NodeType> node_;
@@ -110,8 +122,12 @@ class LayoutTreeBuilderForText : public LayoutTreeBuilder<Text> {
                            ComputedStyle* style_from_parent)
       : LayoutTreeBuilder(text, layout_parent), style_(style_from_parent) {}
 
-  scoped_refptr<ComputedStyle> style_;
   void CreateLayoutObject();
+
+ private:
+  LayoutObject* CreateInlineWrapperForDisplayContentsIfNeeded();
+
+  scoped_refptr<ComputedStyle> style_;
 };
 
 }  // namespace blink

third_party/WebKit/Source/core/dom/Text.cpp

@@ -384,9 +384,20 @@ void Text::ReattachLayoutTreeIfNeeded(const AttachContext& context) {
 
 void Text::RecalcTextStyle(StyleRecalcChange change) {
   if (LayoutTextItem layout_item = LayoutTextItem(GetLayoutObject())) {
-    if (change != kNoChange || NeedsStyleRecalc())
-      layout_item.SetStyle(
-          GetDocument().EnsureStyleResolver().StyleForText(this));
+    if (change != kNoChange || NeedsStyleRecalc()) {
+      scoped_refptr<ComputedStyle> new_style =
+          GetDocument().EnsureStyleResolver().StyleForText(this);
+      const ComputedStyle* layout_parent_style =
+          GetLayoutObject()->Parent()->Style();
+      if (new_style != layout_parent_style &&
+          !new_style->InheritedEqual(*layout_parent_style)) {
+        // The computed style or the need for an anonymous inline wrapper for a
+        // display:contents text child changed.
+        SetNeedsReattachLayoutTree();
+        return;
+      }
+      layout_item.SetStyle(std::move(new_style));
+    }
     if (NeedsStyleRecalc())
       layout_item.SetText(DataImpl());
     ClearNeedsStyleRecalc();

third_party/WebKit/Source/core/layout/LayoutBlock.cpp

@@ -314,7 +314,7 @@ void LayoutBlock::AddChildBeforeDescendant(LayoutObject* new_child,
     // Insert the child into the anonymous block box instead of here.
     if (new_child->IsInline() || new_child->IsFloatingOrOutOfFlowPositioned() ||
         before_descendant->Parent()->SlowFirstChild() != before_descendant)
-      before_descendant->Parent()->AddChild(new_child, before_descendant);
+      before_descendant_container->AddChild(new_child, before_descendant);
     else
       AddChild(new_child, before_descendant->Parent());
     return;

third_party/WebKit/Source/core/layout/LayoutInline.cpp

@@ -408,6 +408,7 @@ void LayoutInline::AddChildIgnoringContinuation(LayoutObject* new_child,
 
 LayoutInline* LayoutInline::Clone() const {
   LayoutInline* clone_inline = new LayoutInline(GetNode());
+  DCHECK(!IsAnonymous());
   clone_inline->SetStyle(MutableStyle());
   clone_inline->SetIsInsideFlowThread(IsInsideFlowThread());
   return clone_inline;
@@ -416,6 +417,8 @@ LayoutInline* LayoutInline::Clone() const {
 void LayoutInline::MoveChildrenToIgnoringContinuation(
     LayoutInline* to,
     LayoutObject* start_child) {
+  DCHECK(!IsAnonymous());
+  DCHECK(!to->IsAnonymous());
   LayoutObject* child = start_child;
   while (child) {
     LayoutObject* current_child = child;
@@ -431,6 +434,7 @@ void LayoutInline::SplitInlines(LayoutBlockFlow* from_block,
                                 LayoutObject* before_child,
                                 LayoutBoxModelObject* old_cont) {
   DCHECK(IsDescendantOf(from_block));
+  DCHECK(!IsAnonymous());
 
   // If we're splitting the inline containing the fullscreened element,
   // |beforeChild| may be the layoutObject for the fullscreened element.

third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp

@@ -21,8 +21,28 @@ using ::testing::Return;
 class LayoutObjectTest : public RenderingTest {
  public:
   LayoutObjectTest() : RenderingTest(EmptyLocalFrameClient::Create()) {}
+
+ protected:
+  template <bool should_have_wrapper>
+  void ExpectAnonymousInlineWrapperFor(Node*);
 };
 
+template <bool should_have_wrapper>
+void LayoutObjectTest::ExpectAnonymousInlineWrapperFor(Node* node) {
+  ASSERT_TRUE(node);
+  EXPECT_TRUE(node->IsTextNode());
+  LayoutObject* text_layout = node->GetLayoutObject();
+  ASSERT_TRUE(text_layout);
+  LayoutObject* text_parent = text_layout->Parent();
+  ASSERT_TRUE(text_parent);
+  if (should_have_wrapper) {
+    EXPECT_TRUE(text_parent->IsAnonymous());
+    EXPECT_TRUE(text_parent->IsInline());
+  } else {
+    EXPECT_FALSE(text_parent->IsAnonymous());
+  }
+}
+
 TEST_F(LayoutObjectTest, LayoutDecoratedNameCalledWithPositionedObject) {
   SetBodyInnerHTML("<div id='div' style='position: fixed'>test</div>");
   Element* div = GetDocument().getElementById(AtomicString("div"));
@@ -439,4 +459,70 @@ TEST_F(LayoutObjectTest, LocationInBackingAndSelectionVisualRect) {
   EXPECT_EQ(LayoutRect(), object->SelectionVisualRect());
 }
 
+TEST_F(LayoutObjectTest, DisplayContentsInlineWrapper) {
+  SetBodyInnerHTML("<div id='div' style='display:contents;color:pink'>A</div>");
+  Element* div = GetDocument().getElementById("div");
+  ASSERT_TRUE(div);
+  Node* text = div->firstChild();
+  ASSERT_TRUE(text);
+  ExpectAnonymousInlineWrapperFor<true>(text);
+}
+
+TEST_F(LayoutObjectTest, DisplayContentsNoInlineWrapper) {
+  SetBodyInnerHTML("<div id='div' style='display:contents'>A</div>");
+  Element* div = GetDocument().getElementById("div");
+  ASSERT_TRUE(div);
+  Node* text = div->firstChild();
+  ASSERT_TRUE(text);
+  ExpectAnonymousInlineWrapperFor<false>(text);
+}
+
+TEST_F(LayoutObjectTest, DisplayContentsAddInlineWrapper) {
+  SetBodyInnerHTML("<div id='div' style='display:contents'>A</div>");
+  Element* div = GetDocument().getElementById("div");
+  ASSERT_TRUE(div);
+  Node* text = div->firstChild();
+  ASSERT_TRUE(text);
+  ExpectAnonymousInlineWrapperFor<false>(text);
+
+  div->SetInlineStyleProperty(CSSPropertyColor, "pink");
+  GetDocument().View()->UpdateAllLifecyclePhases();
+  ExpectAnonymousInlineWrapperFor<true>(text);
+}
+
+TEST_F(LayoutObjectTest, DisplayContentsRemoveInlineWrapper) {
+  SetBodyInnerHTML("<div id='div' style='display:contents;color:pink'>A</div>");
+  Element* div = GetDocument().getElementById("div");
+  ASSERT_TRUE(div);
+  Node* text = div->firstChild();
+  ASSERT_TRUE(text);
+  ExpectAnonymousInlineWrapperFor<true>(text);
+
+  div->RemoveInlineStyleProperty(CSSPropertyColor);
+  GetDocument().View()->UpdateAllLifecyclePhases();
+  ExpectAnonymousInlineWrapperFor<false>(text);
+}
+
+TEST_F(LayoutObjectTest, DisplayContentsWrapperPerTextNode) {
+  // This test checks the current implementation; that text node siblings do not
+  // share inline wrappers. Doing so requires code to handle all situations
+  // where text nodes are no longer layout tree siblings by splitting wrappers,
+  // and merge wrappers when text nodes become layout tree siblings.
+  SetBodyInnerHTML(
+      "<div id='div' style='display:contents;color:pink'>A<!-- -->B</div>");
+  Element* div = GetDocument().getElementById("div");
+  ASSERT_TRUE(div);
+  Node* text1 = div->firstChild();
+  ASSERT_TRUE(text1);
+  Node* text2 = div->lastChild();
+  ASSERT_TRUE(text2);
+  EXPECT_NE(text1, text2);
+
+  ExpectAnonymousInlineWrapperFor<true>(text1);
+  ExpectAnonymousInlineWrapperFor<true>(text2);
+
+  EXPECT_NE(text1->GetLayoutObject()->Parent(),
+            text2->GetLayoutObject()->Parent());
+}
+
 }  // namespace blink

third_party/WebKit/Source/core/style/ComputedStyle.cpp

@@ -122,6 +122,18 @@ scoped_refptr<ComputedStyle> ComputedStyle::CreateAnonymousStyleWithDisplay(
   return new_style;
 }
 
+scoped_refptr<ComputedStyle>
+ComputedStyle::CreateInheritedDisplayContentsStyleIfNeeded(
+    const ComputedStyle& parent_style,
+    const ComputedStyle& layout_parent_style) {
+  if (&parent_style == &layout_parent_style)
+    return nullptr;
+  if (parent_style.InheritedEqual(layout_parent_style))
+    return nullptr;
+  return ComputedStyle::CreateAnonymousStyleWithDisplay(parent_style,
+                                                        EDisplay::kInline);
+}
+
 scoped_refptr<ComputedStyle> ComputedStyle::Clone(const ComputedStyle& other) {
   return base::AdoptRef(new ComputedStyle(other));
 }

third_party/WebKit/Source/core/style/ComputedStyle.h

@@ -195,6 +195,10 @@ class ComputedStyle : public ComputedStyleBase,
   static scoped_refptr<ComputedStyle> CreateAnonymousStyleWithDisplay(
       const ComputedStyle& parent_style,
       EDisplay);
+  static scoped_refptr<ComputedStyle>
+  CreateInheritedDisplayContentsStyleIfNeeded(
+      const ComputedStyle& parent_style,
+      const ComputedStyle& layout_parent_style);
   CORE_EXPORT static scoped_refptr<ComputedStyle> Clone(const ComputedStyle&);
   static const ComputedStyle& InitialStyle() { return MutableInitialStyle(); }
   static void InvalidateInitialStyle();