[css-tables] Set table and cell widths dirty when section border changes

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

Index: third_party/WebKit/Source/core/layout/LayoutTableSection.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp b/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp
index fc6328921f0d096245b5cf5b09d47034ea2e6f98..fc1b01f22c59c58b9005c0de171d080da16c9102 100644
--- a/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp
@@ -116,6 +116,15 @@ LayoutTableSection::~LayoutTableSection()
 {
 }
 
+// TODO(dgrogan): Where should this go? It is duplicated in LayoutTableRow.cpp.

[mstensho (USE GERRIT), 2016/05/25 12:18:28]

There's a common LayoutTableBoxComponent base class now, so maybe there?

[dgrogan, 2016/05/25 20:01:09]

Yes, good call.

+static bool borderWidthChanged(const ComputedStyle* oldStyle, const ComputedStyle* newStyle)
+{
+    return oldStyle->borderLeftWidth() != newStyle->borderLeftWidth()
+        || oldStyle->borderTopWidth() != newStyle->borderTopWidth()
+        || oldStyle->borderRightWidth() != newStyle->borderRightWidth()
+        || oldStyle->borderBottomWidth() != newStyle->borderBottomWidth();
+}
+
 void LayoutTableSection::styleDidChange(StyleDifference diff, const ComputedStyle* oldStyle)
 {
     LayoutTableBoxComponent::styleDidChange(diff, oldStyle);
@@ -125,6 +134,21 @@ void LayoutTableSection::styleDidChange(StyleDifference diff, const ComputedStyl
     LayoutTable* table = this->table();
     if (table && !table->selfNeedsLayout() && !table->normalChildNeedsLayout() && oldStyle && oldStyle->border() != style()->border())
         table->invalidateCollapsedBorders();
+
+    // TODO(dgrogan): Do we need to setPreferredLogicalWidthsDirty even when !diff.needsFullLayout()?

[mstensho (USE GERRIT), 2016/05/25 12:18:28]

Can we really schedule for layout as part of preferred logical widths
computations? If we can't (and I really hope we can't), I cannot imagine how
that could be necessary. Doing a full layout without recalculating preferred
logical widths could in theory be sensible in some cases, but the opposite seems
strange to me.

[dgrogan, 2016/05/25 20:01:09]

This reasoning makes sense to me. I just didn't know what cases caused
needsFullLayout to be set. I now see that changed border widths sets
needsFullLayout in ComputedStyle::diffNeedsFullLayoutAndPaintInvalidation.

+    if (table && oldStyle && diff.needsFullLayout() && needsLayout() && table->collapseBorders() && borderWidthChanged(oldStyle, style())) {
+        table->setPreferredLogicalWidthsDirty(MarkOnlyThis);
+        for (unsigned r = 0; r < m_grid.size(); ++r) {
+            LayoutTableRow* rowLayoutObject = m_grid[r].rowLayoutObject;
+            if (!rowLayoutObject)
+                continue;
+            for (LayoutBox* childBox = rowLayoutObject->firstChildBox(); childBox; childBox = childBox->nextSiblingBox()) {
+                if (!childBox->isTableCell())
+                    continue;
+                childBox->setPreferredLogicalWidthsDirty(MarkOnlyThis);

[mstensho (USE GERRIT), 2016/05/25 12:18:28]

Why MarkOnlyThis? If that's not necessary (or even wrong), I'd say that this
code also looks very similar to what's in
TableLayoutAlgorithmFixed::willChangeTableLayout().

Maybe we need a LayoutTableSection::setPreferredLogicalWidthsDirtyOnAllCells()
or something like that?

[dgrogan, 2016/05/25 20:01:09]

The idea was that it was an optimization: parents wouldn't need their width
recalculated. But that's wrong for table on line 140. But with this patch
(including MarkOnlyThis) putting the table in a shrink-to-fit floating div
somehow works. I don't yet know why.
https://jsfiddle.net/dgrogan/3p02m578/
I like this idea.

[mstensho (USE GERRIT), 2016/05/26 18:08:48]

Yes, I really think the parents DO need their preferred widths recalculated, or
your fiddle would fail.
Maybe the style change itself automatically marks the entire ancestry? Maybe
that's why it works, even though you use MarkOnlyThis?

[dgrogan, 2016/05/27 00:21:38]

You're right, LayoutObject::styleDidChange marks the container chain's widths as
dirty when diff.needsFullLayout(). So this new code I'm working on only has to
worry about affected descendants, not ancestors.

+            }
+        }
+    }
 }
 
 void LayoutTableSection::willBeRemovedFromTree()