Support display: contents for elements, first-line and first-letter pseudos.

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

Index: third_party/WebKit/Source/core/dom/Text.cpp
diff --git a/third_party/WebKit/Source/core/dom/Text.cpp b/third_party/WebKit/Source/core/dom/Text.cpp
index a105811975803ddc44edf75f845d6386a3f5d22c..a0adf4ea2996e522aea9f0ecc9d27cf550c29788 100644
--- a/third_party/WebKit/Source/core/dom/Text.cpp
+++ b/third_party/WebKit/Source/core/dom/Text.cpp
@@ -254,6 +254,9 @@ static inline bool canHaveWhitespaceChildren(const LayoutObject& parent) {
 
 bool Text::textLayoutObjectIsNeeded(const ComputedStyle& style,
                                     const LayoutObject& parent) const {
+  const LayoutObject* prev = nullptr;
+  bool lookedForPrev = false;
+
   if (!parent.canHaveChildren())
     return false;
 
@@ -269,6 +272,26 @@ bool Text::textLayoutObjectIsNeeded(const ComputedStyle& style,
   if (!containsOnlyWhitespace())
     return true;
 
+  // We may have multiple text layout object siblings as part of a layout tree
+  // affected by display: contents. In the case we have something like:
+  //
+  //   <div>Foo<div style="display: contents"> </div>Bar</div>
+  //
+  // We can't just bail out and don't create a text layout object for the
+  // whitespace text node inside the display: contents element.
+  //
+  // We can check whether our style parent or our previous light-tree sibling
+  // has display contents to to avoid doing the previous layout object lookup

[rune, 2017/01/27 13:09:23]

double "to"

+  // more than necessary.
+  if (style.display() == EDisplay::Contents ||
+      (previousSibling() && previousSibling()->isElementNode() &&
+       toElement(previousSibling())->hasDisplayContentsStyle())) {
+    prev = LayoutTreeBuilderTraversal::previousSiblingLayoutObject(*this);
+    lookedForPrev = true;
+    if (prev && prev->node() && prev->node()->isTextNode())

[rune, 2017/01/27 13:09:23]

Not strictly true if the previous sibling is also a whitespace layout object?

We're already sub-optimal without display:content. I've reported
crbug.com/686016.

Did you fix whitespace attachment partially here to make more of the csswg tests
pass? It would probably be good to contain whitespace attachment fixes in a
separate CL?

[emilio, 2017/01/27 22:13:15]

Right, I guess we could avoid creating it if the previous sibling was also a
whitespace-only text node, if this is what you meant.
Yes, and yes, I can split this out into another CL.

+      return true;
+  }
+
   if (!canHaveWhitespaceChildren(parent))
     return false;
 
@@ -290,9 +313,12 @@ bool Text::textLayoutObjectIsNeeded(const ComputedStyle& style,
   // the number of siblings to visit.
   unsigned maxSiblingsToVisit = 50;
 
-  const LayoutObject* prev =
-      LayoutTreeBuilderTraversal::previousSiblingLayoutObject(
-          *this, maxSiblingsToVisit);
+  if (!lookedForPrev) {
+    lookedForPrev = true;
+    prev = LayoutTreeBuilderTraversal::previousSiblingLayoutObject(
+        *this, maxSiblingsToVisit);
+  }
+
   if (prev && prev->isBR())  // <span><br/> <br/></span>
     return false;
 
@@ -349,12 +375,17 @@ LayoutText* Text::createTextLayoutObject(const ComputedStyle& style) {
 }
 
 void Text::attachLayoutTree(const AttachContext& context) {
-  if (ContainerNode* layoutParent = LayoutTreeBuilderTraversal::parent(*this)) {
-    if (LayoutObject* parentLayoutObject = layoutParent->layoutObject()) {
-      if (textLayoutObjectIsNeeded(*parentLayoutObject->style(),
-                                   *parentLayoutObject))
-        LayoutTreeBuilderForText(*this, parentLayoutObject)
-            .createLayoutObject();
+  ContainerNode* styleParent = LayoutTreeBuilderTraversal::parent(*this);
+  LayoutObject* parentLayoutObject =
+      LayoutTreeBuilderTraversal::parentLayoutObject(*this);
+
+  if (styleParent && parentLayoutObject) {
+    DCHECK(styleParent->computedStyle());
+    if (textLayoutObjectIsNeeded(*styleParent->computedStyle(),
+                                 *parentLayoutObject)) {
+      LayoutTreeBuilderForText(*this, parentLayoutObject,
+                               styleParent->mutableComputedStyle())

[rune, 2017/01/27 13:09:23]

Using mutableComputedStyle() here looks worrying, but I see that we need it to
be non-const to share ComputedStyle with the parent element.

[emilio, 2017/01/27 22:13:15]

Yes, I agree, though this was pre-existing (the previous code used
layoutObject->mutableStyleRef()).

+          .createLayoutObject();
     }
   }
   CharacterData::attachLayoutTree(context);
@@ -362,13 +393,13 @@ void Text::attachLayoutTree(const AttachContext& context) {
 
 void Text::reattachLayoutTreeIfNeeded(const AttachContext& context) {
   bool layoutObjectIsNeeded = false;
-  ContainerNode* layoutParent = LayoutTreeBuilderTraversal::parent(*this);
-  if (layoutParent) {
-    if (LayoutObject* parentLayoutObject = layoutParent->layoutObject()) {
-      if (textLayoutObjectIsNeeded(*parentLayoutObject->style(),
-                                   *parentLayoutObject))
-        layoutObjectIsNeeded = true;
-    }
+  ContainerNode* styleParent = LayoutTreeBuilderTraversal::parent(*this);
+  LayoutObject* parentLayoutObject =
+      LayoutTreeBuilderTraversal::parentLayoutObject(*this);
+  if (styleParent && parentLayoutObject) {
+    DCHECK(styleParent->computedStyle());
+    layoutObjectIsNeeded = textLayoutObjectIsNeeded(
+        *styleParent->computedStyle(), *parentLayoutObject);
   }
 
   if (layoutObjectIsNeeded == !!layoutObject())
@@ -382,9 +413,11 @@ void Text::reattachLayoutTreeIfNeeded(const AttachContext& context) {
 
   if (getStyleChangeType() < NeedsReattachStyleChange)
     detachLayoutTree(reattachContext);
-  if (layoutObjectIsNeeded)
-    LayoutTreeBuilderForText(*this, layoutParent->layoutObject())
+  if (layoutObjectIsNeeded) {
+    LayoutTreeBuilderForText(*this, parentLayoutObject,
+                             styleParent->mutableComputedStyle())
         .createLayoutObject();
+  }
   CharacterData::attachLayoutTree(reattachContext);
 }