Bug 74303 – Regression(99212): table rows get incorrect height after changing some cells' height

• Status: RESOLVED
• Resolution: FIXED
• Component: Tables

[Vsevolod Vlasov]

Steps to reproduce:

1) Open inspector's network panel
2) Load some web page, make sure network log does not have scroll yet (just 1-3 requests)
3) Toggle large resource rows (in the bottom toolbar press fourth button)

This was caused by http://trac.webkit.org/changeset/99212.

Notes: pressing this button adds "small" class to table's container (".data-grid"). Essentially this button switches between following style rules:

.network-log-grid.data-grid td {
    height: 37px;
}

.network-log-grid.data-grid.small td {
    height: 17px;
}

[Julien Chaffraix]

I started investigating this regression and it doesn't look like it is as simple as just switching those 2 styles. There is some CSS & JS that must kick in that prevented me from getting a proper test case.

I am pretty sure this is not a WebInspector-only bug but a more general Table one. The easy fix would be to revert the RenderTableCell part of r99212 and mark the section as needing cell recalculation when height changes (likely an overkill).

[Vsevolod Vlasov]

Created attachment 118898 [details]
Reduction

[Pavel Feldman]

@caseq: could you take a look?

[Julien Chaffraix]

(In reply to comment #3)
> @caseq: could you take a look?

I am on it (which is why I took the bug). I understand the problem and can reproduce the issue. I just need to improve our test coverage of this area which is obviously lacking.

[Julien Chaffraix]

Created attachment 119217 [details]
Proposed fix 1: Use the same row height recalculation algorithm for |rowLogicalHeightChange| and |recalCells|.

[Pavel Feldman]

Sounds like Darin has been reviewing the original issue (http://trac.webkit.org/changeset/99212).

[Darin Adler]

Comment on attachment 119217 [details]
Proposed fix 1: Use the same row height recalculation algorithm for |rowLogicalHeightChange| and |recalCells|.

View in context: https://bugs.webkit.org/attachment.cgi?id=119217&action=review

> Source/WebCore/rendering/RenderTableSection.cpp:1170
> +    for (RenderObject* cell = m_grid[rowIndex].rowRenderer->firstChild(); cell; cell = cell->nextSibling()) {

What guarantees that rowRenderer is non-null?

[Julien Chaffraix]

Comment on attachment 119217 [details]
Proposed fix 1: Use the same row height recalculation algorithm for |rowLogicalHeightChange| and |recalCells|.

View in context: https://bugs.webkit.org/attachment.cgi?id=119217&action=review

>> Source/WebCore/rendering/RenderTableSection.cpp:1170
>> +    for (RenderObject* cell = m_grid[rowIndex].rowRenderer->firstChild(); cell; cell = cell->nextSibling()) {
> 
> What guarantees that rowRenderer is non-null?

This is called only from 2 places:
* RenderTableCell::styleDidChange: we check that rowIndex was set which means that recalcCell was called and filled m_grid. 
* RenderTableRow::styleDidChange: to find |rowIndex|, we check against m_grid.rowRenderer.

In both places, if m_grid was filled by recalcCells, each item should have a rowRenderer (per construction of the RenderTableSection tree).

[WebKit Review Bot]

Comment on attachment 119217 [details]
Proposed fix 1: Use the same row height recalculation algorithm for |rowLogicalHeightChange| and |recalCells|.

Clearing flags on attachment: 119217

Committed r103327: <http://trac.webkit.org/changeset/103327>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.