Diff
Modified: trunk/LayoutTests/ChangeLog (132611 => 132612)
--- trunk/LayoutTests/ChangeLog 2012-10-26 11:08:26 UTC (rev 132611)
+++ trunk/LayoutTests/ChangeLog 2012-10-26 11:13:46 UTC (rev 132612)
@@ -1,3 +1,19 @@
+2012-10-26 Julien Chaffraix <[email protected]>
+
+ RenderTableCol::computePreferredLogicalWidths and RenderTableCol::layout should never be called
+ https://bugs.webkit.org/show_bug.cgi?id=99861
+
+ Reviewed by Ojan Vafai.
+
+ Added 2 tests:
+ - 1 to cover the code in RenderTableCol::updateFromElement.
+ - 1 to cover simplified layout on a table.
+
+ * fast/table/col-span-change-relayout-expected.txt: Added.
+ * fast/table/col-span-change-relayout.html: Added.
+ * fast/table/simplified-layout-table-expected.html: Added.
+ * fast/table/simplified-layout-table.html: Added.
+
2012-10-26 Balazs Kelemen <[email protected]>
[Qt] Pixel tests need rebaseline
Added: trunk/LayoutTests/fast/table/col-span-change-relayout-expected.txt (0 => 132612)
--- trunk/LayoutTests/fast/table/col-span-change-relayout-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/table/col-span-change-relayout-expected.txt 2012-10-26 11:13:46 UTC (rev 132612)
@@ -0,0 +1,7 @@
+This test checks that dynamically changing a column's 'span' properly recompute the cell's widths.
+
+The table below shrink to 200px after changing the column's span.
+
+PASS
+PASS
+
Added: trunk/LayoutTests/fast/table/col-span-change-relayout.html (0 => 132612)
--- trunk/LayoutTests/fast/table/col-span-change-relayout.html (rev 0)
+++ trunk/LayoutTests/fast/table/col-span-change-relayout.html 2012-10-26 11:13:46 UTC (rev 132612)
@@ -0,0 +1,58 @@
+<!DOCTYPE html>
+<html>
+ <head>
+ <script src=""
+ <style>
+ table
+ {
+ border-collapse: collapse;
+ table-layout: fixed;
+ width: 200px;
+ }
+ td
+ {
+ background: green;
+ height: 1em;
+ }
+ </style>
+ <script>
+ if (window.testRunner)
+ testRunner.dumpAsText();
+
+ function log(msg)
+ {
+ var console = document.getElementById('console');
+ console.appendChild(document.createTextNode(msg));
+ console.appendChild(document.createElement('br'));
+ }
+
+ function changeColSpan()
+ {
+ document.getElementById('table').setAttribute('data-expected-width', 300);
+ checkLayout('table');
+
+ var col = document.getElementById('column');
+ col.setAttribute("span", "1");
+ document.getElementById('table').setAttribute('data-expected-width', 200);
+ checkLayout('table');
+ }
+ </script>
+ </head>
+ <body _onload_="changeColSpan();">
+ <p>This test checks that dynamically changing a column's 'span' properly recompute the cell's widths.</p>
+ <p>The table below shrink to 200px after changing the column's span.</p>
+ <table id="table">
+ <colgroup>
+ <col span="2" id="column" style="width: 100px"></col>
+ <col style="width: 100px;"></col>
+ </colgroup>
+ <tr>
+ <td id="cell"></td>
+ <td></td>
+ <td></td>
+ </tr>
+ </table>
+ <p id="console"></p>
+ </body>
+</html>
+
Added: trunk/LayoutTests/fast/table/simplified-layout-table-expected.html (0 => 132612)
--- trunk/LayoutTests/fast/table/simplified-layout-table-expected.html (rev 0)
+++ trunk/LayoutTests/fast/table/simplified-layout-table-expected.html 2012-10-26 11:13:46 UTC (rev 132612)
@@ -0,0 +1,18 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+div {
+ height: 100px;
+ width: 100px;
+ background-color: green;
+ opacity: 0.8;
+ padding: 0px;
+ -webkit-transform: translate(50px, 50px);
+}
+</style>
+</head>
+<p>Test for <a href="" and RenderTableCol::layout should never be called.</p>
+<p>This test checks that simplified layout works on a table with a <col>.</p>
+<div></div>
+</html>
Property changes on: trunk/LayoutTests/fast/table/simplified-layout-table-expected.html
___________________________________________________________________
Added: svn:executable
Added: trunk/LayoutTests/fast/table/simplified-layout-table.html (0 => 132612)
--- trunk/LayoutTests/fast/table/simplified-layout-table.html (rev 0)
+++ trunk/LayoutTests/fast/table/simplified-layout-table.html 2012-10-26 11:13:46 UTC (rev 132612)
@@ -0,0 +1,40 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+table {
+ border-spacing: 0px;
+}
+col {
+ width: 100px;
+}
+td {
+ height: 100px;
+ background-color: green;
+ opacity: 0.8;
+ padding: 0px;
+}
+td.transformed {
+ -webkit-transform: translate(50px, 50px);
+}
+</style>
+<script>
+function simplifiedLayout()
+{
+ var cell = document.getElementsByTagName("td")[0];
+ // Force layout.
+ cell.offsetTop;
+ cell.classList.add("transformed");
+}
+window.addEventListener("load", simplifiedLayout);
+</script>
+</head>
+<p>Test for <a href="" and RenderTableCol::layout should never be called.</p>
+<p>This test checks that simplified layout works on a table with a <col>.</p>
+<table>
+ <col></col>
+ <tbody>
+ <tr><td></td></tr>
+ </tbody>
+</table>
+</html>
Property changes on: trunk/LayoutTests/fast/table/simplified-layout-table.html
___________________________________________________________________
Added: svn:executable
Modified: trunk/Source/WebCore/ChangeLog (132611 => 132612)
--- trunk/Source/WebCore/ChangeLog 2012-10-26 11:08:26 UTC (rev 132611)
+++ trunk/Source/WebCore/ChangeLog 2012-10-26 11:13:46 UTC (rev 132612)
@@ -1,3 +1,46 @@
+2012-10-26 Julien Chaffraix <[email protected]>
+
+ RenderTableCol::computePreferredLogicalWidths and RenderTableCol::layout should never be called
+ https://bugs.webkit.org/show_bug.cgi?id=99861
+
+ Reviewed by Ojan Vafai.
+
+ RenderTableCol's computePreferredLogicalWidths and layout's only purpose were to clear the preferred logical
+ widths dirty / layout flag so that we would properly propagate the information to our containing table. This
+ led to clunky code where the table layout code had to forcefully call RenderTableCol::computePreferredLogicalWidths
+ or else we would ignore the next layout hint on the <col> or <colgroup>.
+
+ Tests: fast/table/col-span-change-relayout.html
+ fast/table/simplified-layout-table.html
+
+ * rendering/AutoTableLayout.cpp:
+ (WebCore::AutoTableLayout::recalcColumn):
+ * rendering/RenderTable.cpp:
+ (WebCore::RenderTable::layout):
+ Simplified the code now that we only need to iterate over the sections.
+
+ * rendering/FixedTableLayout.cpp:
+ (WebCore::FixedTableLayout::calcWidthArray):
+ Removed call to computePreferredLogicalWidths.
+
+ * rendering/RenderTableCol.cpp:
+ (WebCore::RenderTableCol::styleDidChange):
+ (WebCore::RenderTableCol::updateFromElement):
+ Forward a layout hint to the table so that we properly recompute the cell's logical withs.
+
+ (WebCore::RenderTableCol::computePreferredLogicalWidths):
+ (WebCore::RenderTableCol::layout):
+ Change our implementations of those 2 methods to be no-ops, while enforcing that they are
+ never called when it was possible.
+
+ (WebCore::RenderTableCol::propagateLayoutCueToTable):
+ New helper function that forward any layout cue to the containing table, this works around
+ us not clearing the flags which confuses RenderObject markContainingBlocksForLayout and
+ invalidateContainerPreferredLogicalWidths.
+
+ * rendering/RenderTableCol.h:
+ Made the function that we are not expected to be called private.
+
2012-10-26 Sheriff Bot <[email protected]>
Unreviewed, rolling out r132601.
Modified: trunk/Source/WebCore/rendering/AutoTableLayout.cpp (132611 => 132612)
--- trunk/Source/WebCore/rendering/AutoTableLayout.cpp 2012-10-26 11:08:26 UTC (rev 132611)
+++ trunk/Source/WebCore/rendering/AutoTableLayout.cpp 2012-10-26 11:13:46 UTC (rev 132612)
@@ -49,81 +49,77 @@
RenderTableCell* fixedContributor = 0;
RenderTableCell* maxContributor = 0;
- for (RenderObject* child = m_table->children()->firstChild(); child; child = child->nextSibling()) {
- if (child->isRenderTableCol())
- toRenderTableCol(child)->computePreferredLogicalWidths();
- else if (child->isTableSection()) {
- RenderTableSection* section = toRenderTableSection(child);
- unsigned numRows = section->numRows();
- for (unsigned i = 0; i < numRows; i++) {
- RenderTableSection::CellStruct current = section->cellAt(i, effCol);
- RenderTableCell* cell = current.primaryCell();
-
- if (current.inColSpan || !cell)
- continue;
+ for (RenderTableSection* section = m_table->topNonEmptySection(); section; section = m_table->sectionBelow(section, SkipEmptySections)) {
+ unsigned numRows = section->numRows();
+ ASSERT(numRows);
+ for (unsigned i = 0; i < numRows; i++) {
+ RenderTableSection::CellStruct current = section->cellAt(i, effCol);
+ RenderTableCell* cell = current.primaryCell();
- bool cellHasContent = cell->children()->firstChild() || cell->style()->hasBorder() || cell->style()->hasPadding();
- if (cellHasContent)
- columnLayout.emptyCellsOnly = false;
+ if (current.inColSpan || !cell)
+ continue;
- // A cell originates in this column. Ensure we have
- // a min/max width of at least 1px for this column now.
- columnLayout.minLogicalWidth = max<int>(columnLayout.minLogicalWidth, cellHasContent ? 1 : 0);
- columnLayout.maxLogicalWidth = max<int>(columnLayout.maxLogicalWidth, 1);
+ bool cellHasContent = cell->children()->firstChild() || cell->style()->hasBorder() || cell->style()->hasPadding();
+ if (cellHasContent)
+ columnLayout.emptyCellsOnly = false;
- if (cell->colSpan() == 1) {
- if (cell->preferredLogicalWidthsDirty())
- cell->computePreferredLogicalWidths();
- columnLayout.minLogicalWidth = max<int>(cell->minPreferredLogicalWidth(), columnLayout.minLogicalWidth);
- if (cell->maxPreferredLogicalWidth() > columnLayout.maxLogicalWidth) {
- columnLayout.maxLogicalWidth = cell->maxPreferredLogicalWidth();
- maxContributor = cell;
- }
+ // A cell originates in this column. Ensure we have
+ // a min/max width of at least 1px for this column now.
+ columnLayout.minLogicalWidth = max<int>(columnLayout.minLogicalWidth, cellHasContent ? 1 : 0);
+ columnLayout.maxLogicalWidth = max<int>(columnLayout.maxLogicalWidth, 1);
- // All browsers implement a size limit on the cell's max width.
- // Our limit is based on KHTML's representation that used 16 bits widths.
- // FIXME: Other browsers have a lower limit for the cell's max width.
- const int cCellMaxWidth = 32760;
- Length cellLogicalWidth = cell->styleOrColLogicalWidth();
- if (cellLogicalWidth.value() > cCellMaxWidth)
- cellLogicalWidth.setValue(cCellMaxWidth);
- if (cellLogicalWidth.isNegative())
- cellLogicalWidth.setValue(0);
- switch (cellLogicalWidth.type()) {
- case Fixed:
- // ignore width=0
- if (cellLogicalWidth.isPositive() && !columnLayout.logicalWidth.isPercent()) {
- LayoutUnit logicalWidth = cell->adjustBorderBoxLogicalWidthForBoxSizing(cellLogicalWidth.value());
- if (columnLayout.logicalWidth.isFixed()) {
- // Nav/IE weirdness
- if ((logicalWidth > columnLayout.logicalWidth.value())
- || ((columnLayout.logicalWidth.value() == logicalWidth) && (maxContributor == cell))) {
- columnLayout.logicalWidth.setValue(Fixed, logicalWidth);
- fixedContributor = cell;
- }
- } else {
+ if (cell->colSpan() == 1) {
+ if (cell->preferredLogicalWidthsDirty())
+ cell->computePreferredLogicalWidths();
+ columnLayout.minLogicalWidth = max<int>(cell->minPreferredLogicalWidth(), columnLayout.minLogicalWidth);
+ if (cell->maxPreferredLogicalWidth() > columnLayout.maxLogicalWidth) {
+ columnLayout.maxLogicalWidth = cell->maxPreferredLogicalWidth();
+ maxContributor = cell;
+ }
+
+ // All browsers implement a size limit on the cell's max width.
+ // Our limit is based on KHTML's representation that used 16 bits widths.
+ // FIXME: Other browsers have a lower limit for the cell's max width.
+ const int cCellMaxWidth = 32760;
+ Length cellLogicalWidth = cell->styleOrColLogicalWidth();
+ if (cellLogicalWidth.value() > cCellMaxWidth)
+ cellLogicalWidth.setValue(cCellMaxWidth);
+ if (cellLogicalWidth.isNegative())
+ cellLogicalWidth.setValue(0);
+ switch (cellLogicalWidth.type()) {
+ case Fixed:
+ // ignore width=0
+ if (cellLogicalWidth.isPositive() && !columnLayout.logicalWidth.isPercent()) {
+ LayoutUnit logicalWidth = cell->adjustBorderBoxLogicalWidthForBoxSizing(cellLogicalWidth.value());
+ if (columnLayout.logicalWidth.isFixed()) {
+ // Nav/IE weirdness
+ if ((logicalWidth > columnLayout.logicalWidth.value())
+ || ((columnLayout.logicalWidth.value() == logicalWidth) && (maxContributor == cell))) {
columnLayout.logicalWidth.setValue(Fixed, logicalWidth);
fixedContributor = cell;
}
+ } else {
+ columnLayout.logicalWidth.setValue(Fixed, logicalWidth);
+ fixedContributor = cell;
}
- break;
- case Percent:
- m_hasPercent = true;
- if (cellLogicalWidth.isPositive() && (!columnLayout.logicalWidth.isPercent() || cellLogicalWidth.value() > columnLayout.logicalWidth.value()))
- columnLayout.logicalWidth = cellLogicalWidth;
- break;
- case Relative:
- // FIXME: Need to understand this case and whether it makes sense to compare values
- // which are not necessarily of the same type.
- if (cellLogicalWidth.value() > columnLayout.logicalWidth.value())
- columnLayout.logicalWidth = cellLogicalWidth;
- default:
- break;
}
- } else if (!effCol || section->primaryCellAt(i, effCol - 1) != cell) {
- // This spanning cell originates in this column. Insert the cell into spanning cells list.
- insertSpanCell(cell);
+ break;
+ case Percent:
+ m_hasPercent = true;
+ if (cellLogicalWidth.isPositive() && (!columnLayout.logicalWidth.isPercent() || cellLogicalWidth.value() > columnLayout.logicalWidth.value()))
+ columnLayout.logicalWidth = cellLogicalWidth;
+ break;
+ case Relative:
+ // FIXME: Need to understand this case and whether it makes sense to compare values
+ // which are not necessarily of the same type.
+ if (cellLogicalWidth.value() > columnLayout.logicalWidth.value())
+ columnLayout.logicalWidth = cellLogicalWidth;
+ default:
+ break;
}
+ } else if (!effCol || section->primaryCellAt(i, effCol - 1) != cell) {
+ // This spanning cell originates in this column. Insert the cell into spanning cells list.
+ insertSpanCell(cell);
}
}
}
Modified: trunk/Source/WebCore/rendering/FixedTableLayout.cpp (132611 => 132612)
--- trunk/Source/WebCore/rendering/FixedTableLayout.cpp 2012-10-26 11:08:26 UTC (rev 132611)
+++ trunk/Source/WebCore/rendering/FixedTableLayout.cpp 2012-10-26 11:13:46 UTC (rev 132612)
@@ -88,8 +88,6 @@
unsigned currentEffectiveColumn = 0;
for (RenderTableCol* col = m_table->firstColumn(); col; col = col->nextColumn()) {
- col->computePreferredLogicalWidths();
-
// Width specified by column-groups that have column child does not affect column width in fixed layout tables
if (col->isTableColumnGroupWithColumnChildren())
continue;
Modified: trunk/Source/WebCore/rendering/RenderTable.cpp (132611 => 132612)
--- trunk/Source/WebCore/rendering/RenderTable.cpp 2012-10-26 11:08:26 UTC (rev 132611)
+++ trunk/Source/WebCore/rendering/RenderTable.cpp 2012-10-26 11:13:46 UTC (rev 132612)
@@ -378,20 +378,15 @@
bool collapsing = collapseBorders();
- for (RenderObject* child = firstChild(); child; child = child->nextSibling()) {
- if (child->isTableSection()) {
- RenderTableSection* section = toRenderTableSection(child);
- if (m_columnLogicalWidthChanged)
- section->setChildNeedsLayout(true, MarkOnlyThis);
- section->layoutIfNeeded();
- totalSectionLogicalHeight += section->calcRowLogicalHeight();
- if (collapsing)
- section->recalcOuterBorder();
- ASSERT(!section->needsLayout());
- } else if (child->isRenderTableCol()) {
- child->layoutIfNeeded();
- ASSERT(!child->needsLayout());
- }
+ // We ignore table col / colgroup in this iteration as they are only used to size the cell's widths during auto / fixed table layout.
+ for (RenderTableSection* section = topSection(); section; section = sectionBelow(section)) {
+ if (m_columnLogicalWidthChanged)
+ section->setChildNeedsLayout(true, MarkOnlyThis);
+ section->layoutIfNeeded();
+ totalSectionLogicalHeight += section->calcRowLogicalHeight();
+ if (collapsing)
+ section->recalcOuterBorder();
+ ASSERT(!section->needsLayout());
}
// If any table section moved vertically, we will just repaint everything from that
Modified: trunk/Source/WebCore/rendering/RenderTableCol.cpp (132611 => 132612)
--- trunk/Source/WebCore/rendering/RenderTableCol.cpp 2012-10-26 11:08:26 UTC (rev 132611)
+++ trunk/Source/WebCore/rendering/RenderTableCol.cpp 2012-10-26 11:13:46 UTC (rev 132612)
@@ -49,6 +49,9 @@
{
RenderBox::styleDidChange(diff, oldStyle);
+ if (diff == StyleDifferenceLayout)
+ propagateLayoutCueToTable();
+
// If border was changed, notify table.
if (parent()) {
RenderTable* table = this->table();
@@ -66,8 +69,9 @@
m_span = tc->span();
} else
m_span = !(style() && style()->display() == TABLE_COLUMN_GROUP);
- if (m_span != oldSpan && style() && parent())
- setNeedsLayoutAndPrefWidthsRecalc();
+
+ if (m_span != oldSpan && style())
+ propagateLayoutCueToTable();
}
void RenderTableCol::willBeRemovedFromTree()
@@ -114,12 +118,28 @@
void RenderTableCol::computePreferredLogicalWidths()
{
- setPreferredLogicalWidthsDirty(false);
+ // <col> and <colgroup> don't have preferred logical widths as they have
+ // no content so computing our preferred logical widths is wasteful.
+ ASSERT_NOT_REACHED();
+}
- for (RenderObject* child = firstChild(); child; child = child->nextSibling())
- child->setPreferredLogicalWidthsDirty(false);
+void RenderTableCol::layout()
+{
+ // There is no need to layout table <col> or <colgroup> as they have no content.
+ // We cannot ASSERT_NOT_REACHED here as simplified normal flow layout forces
+ // layout on any renderer.
}
+void RenderTableCol::propagateLayoutCueToTable() const
+{
+ // Forward any layout hint to the table: this is required as the table is
+ // the one to layout / compute preferred logical widths on all the cells.
+ if (RenderTable* table = this->table()) {
+ table->setChildNeedsLayout(true);
+ table->setPreferredLogicalWidthsDirty(true);
+ }
+}
+
RenderTable* RenderTableCol::table() const
{
RenderObject* table = parent();
Modified: trunk/Source/WebCore/rendering/RenderTableCol.h (132611 => 132612)
--- trunk/Source/WebCore/rendering/RenderTableCol.h 2012-10-26 11:08:26 UTC (rev 132611)
+++ trunk/Source/WebCore/rendering/RenderTableCol.h 2012-10-26 11:13:46 UTC (rev 132612)
@@ -40,8 +40,6 @@
const RenderObjectChildList* children() const { return &m_children; }
RenderObjectChildList* children() { return &m_children; }
- virtual void computePreferredLogicalWidths();
-
unsigned span() const { return m_span; }
void setSpan(unsigned span) { m_span = span; }
@@ -77,6 +75,9 @@
virtual RenderObjectChildList* virtualChildren() { return children(); }
virtual const RenderObjectChildList* virtualChildren() const { return children(); }
+ virtual void computePreferredLogicalWidths() OVERRIDE;
+ virtual void layout() OVERRIDE;
+
virtual const char* renderName() const { return "RenderTableCol"; }
virtual bool isRenderTableCol() const OVERRIDE { return true; }
virtual void updateFromElement();
@@ -92,6 +93,8 @@
virtual void styleDidChange(StyleDifference, const RenderStyle* oldStyle);
+ void propagateLayoutCueToTable() const;
+
RenderTable* table() const;
RenderObjectChildList m_children;
