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

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

 /*
  * Copyright (C) 1997 Martin Jones (mjones@kde.org)
  *           (C) 1997 Torben Weis (weis@kde.org)
  *           (C) 1998 Waldo Bastian (bastian@kde.org)
  *           (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  * Copyright (C) 2003, 2004, 2005, 2006, 2008, 2009, 2010, 2013 Apple Inc. All rights reserved.
  * Copyright (C) 2006 Alexey Proskuryakov (ap@nypop.com)
  *
  * This library is free software; you can redistribute it and/or
@@ skipping 98 matching lines @@
     , m_hasMultipleCellLevels(false)
 {
     // init LayoutObject attributes
     setInline(false); // our object is not Inline
 }
 
 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);
     propagateStyleToAnonymousChildren();
 
     // If border was changed, notify table.
     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()
 {
     LayoutTableBoxComponent::willBeRemovedFromTree();
 
     // Preventively invalidate our cells as we may be re-inserted into
     // a new table which would require us to rebuild our structure.
     setNeedsCellRecalc();
 }
@@ skipping 1526 matching lines @@
     // FIXME: The table's direction should determine our row's direction, not the section's (see bug 96691).
     if (!style()->isLeftToRightDirection())
         cellLocation.setX(LayoutUnit(table()->effectiveColumnPositions()[table()->numEffectiveColumns()] - table()->effectiveColumnPositions()[table()->absoluteColumnToEffectiveColumn(cell->absoluteColumnIndex() + cell->colSpan())] + horizontalBorderSpacing));
     else
         cellLocation.setX(LayoutUnit(table()->effectiveColumnPositions()[effectiveColumn] + horizontalBorderSpacing));
 
     cell->setLogicalLocation(cellLocation);
 }
 
 } // namespace blink