Bug 52185 – Cell heights are disproportional when a row-spanning cell contains a block element that determines the height of the rows

• Status: REOPENED
• Component: CSS

[Ryosuke Niwa]

<table border="0" cellpadding="0" cellspacing="0">
	<tr>
		<td style="border: 1px #F00 solid;">1</td>
		<td rowspan="3">
			<div style="background: #AEE; width: 100px; height: 200px;"></div>
		</td>
	</tr>
	<tr><td style="border: 1px #0F0 solid;">2</td></tr>
	<tr><td style="border: 1px #00F solid;">3</td></tr>
</table>

And only the cell with 3 stretches to the end but we expect each cell to have the same height.

[Peter Kasting]

Note: WebKit's behavior here doesn't match IE/Fx.

[Julien Chaffraix]

For the record, I spend some time investigating this bug and here is the analysis. It's not a trivial bug to fix as our current rowspan algorithm is too simple. We just add the extra rowspan height to the last row of a rowspan which matches other browsers in some cases but not all cases.

To properly implement that, I think we need to do something akin to FF and implement a 2 pass-algorithm: one without the rowspans and then we patch the logical heights to account for them.

See nsTableRowGroupFrame::CalculateRowHeights and more importantly http://mxr.mozilla.org/mozilla-central/source/layout/tables/nsTableRowGroupFrame.cpp#682 about how FF handles rowspans.

[Suchit Agrawal]

Created attachment 200020 [details]
Patch

[EFL EWS Bot]

Comment on attachment 200020 [details]
Patch

Attachment 200020 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/67672

[EFL EWS Bot]

Comment on attachment 200020 [details]
Patch

Attachment 200020 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/67674

[Suchit Agrawal]

(In reply to comment #3)
> Created an attachment (id=200020) [details]
> Patch

Cells heights are not proper when rowspan cell have its own height which is more then the height of the rows under rowspan. After calculating the logical height, we need to recalculate height of rows under rowspan.

After calculating logical height of the rows in the table, we are recalculating the height of the rows those are under rowspan. Based on the ratio of row's logical height, we are distributing rowspan cell height in the rows under rowspan.

We are storing all cell which have rowspan and then we are rearraging them in the order in which we need to calculate the height.
First rowspan cell in the table processed first but if any nested rowspan cell present then it will be calculated first. If 2 or more rowspan cell have same start index and end index then only one rowspan cell will be processed which would have highest height.
After sorting them in the order, we are processing one by one rowspan cell :
1. We takes the heights of rows in rowspan.
2. if any row contains only rowspan cells then height of that row will be zero then in this case, we are taking one part of the rowspan cell height.
3. Based on the ratio, we calculate new height of the rows if rowspan height is more then total rows height in the rowspan.

Beth,
some time back, we had submiited the patch for bug 18092, that code is removed because that code logic is moved down for fixing this bug.

[Suchit Agrawal]

Beth, Please review this patch and share your comment. Thanks.

[Build Bot]

Comment on attachment 200020 [details]
Patch

Attachment 200020 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/195722

[Build Bot]

Comment on attachment 200020 [details]
Patch

Attachment 200020 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/245084

[Tim Horton]

Comment on attachment 200020 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        fter calculating logical height of the rows in the table, we are recalculating the height

fter?

[Suchit Agrawal]

(In reply to comment #10)
> (From update of attachment 200020 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200020&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        fter calculating logical height of the rows in the table, we are recalculating the height
> 
> fter?
> 
It is 'After'. By mistake it happened.

[Suchit Agrawal]

Created attachment 200080 [details]
Patch

[Suchit Agrawal]

(In reply to comment #12)
> Created an attachment (id=200080) [details]
> Patch
> 
Changes in previous patch for compilation issue on mac and efl.

[Suchit Agrawal]

Created attachment 200084 [details]
Patch

[Suchit Agrawal]

(In reply to comment #14)
> Created an attachment (id=200084) [details]
> Patch
> 
Fixed compilation issue on mac and efl,

[Suchit Agrawal]

Created attachment 200086 [details]
Patch

[Suchit Agrawal]

(In reply to comment #16)
> Created an attachment (id=200086) [details]
> Patch
> 
With this patch, WebKit behaves same as IE and Firefox. In some cases Firefox is not doing well.

[Build Bot]

Comment on attachment 200086 [details]
Patch

Attachment 200086 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/238830

New failing tests:
fast/table/table-rowspan-height-distribution-in-rows.html
accessibility/table-attributes.html
fast/css/vertical-align-baseline-rowspan-010.html
fast/css/vertical-align-baseline-rowspan-002.htm
css2.1/20110323/table-height-algorithm-012.htm
fast/css/vertical-align-baseline-rowspan-001.htm
fast/css/vertical-align-baseline-rowspan-008.htm
fast/css/vertical-align-baseline-rowspan-005.htm
accessibility/table-cells.html
fast/css/vertical-align-baseline-rowspan-011.html
fast/css/vertical-align-baseline-rowspan-007.htm
fast/css/vertical-align-baseline-rowspan-006.htm

[Build Bot]

Created attachment 200101 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Suchit Agrawal]

(In reply to comment #17)
> (In reply to comment #16)
> > Created an attachment (id=200086) [details] [details]
> > Patch
> > 
> With this patch, WebKit behaves same as IE and Firefox. In some cases Firefox is not doing well.
> 
Hello Beth, Please check the patch and share your comments. Thanks you

[Suchit Agrawal]

Created attachment 200316 [details]
Patch

[Suchit Agrawal]

(In reply to comment #21)
> Created an attachment (id=200316) [details]
> Patch
> 
Handled baseline issue in the patch. Now all failed test cases are working fine.
Updated some of the test cases those are required.

fast/css/vertical-align-baseline-rowspan-007.htm
Purpose of this above test case is not valid now. I compared the result with IE and FF.
So I updated the test case and its reference test case.
This test case does not have any purpose now so we can remove it also.

[Build Bot]

Comment on attachment 200316 [details]
Patch

Attachment 200316 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/290163

New failing tests:
fast/table/table-rowspan-height-distribution-in-rows.html
accessibility/table-cells.html
accessibility/table-attributes.html

[Build Bot]

Created attachment 200320 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Suchit Agrawal]

(In reply to comment #23)
> (From update of attachment 200316 [details])
> Attachment 200316 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.appspot.com/results/290163
> 
> New failing tests:
> fast/table/table-rowspan-height-distribution-in-rows.html
> accessibility/table-cells.html
> accessibility/table-attributes.html
> 
First test case is newly added for this patch.
Other two test cases need to be rebaseline.

[Suchit Agrawal]

Created attachment 200387 [details]
Patch

[Suchit Agrawal]

(In reply to comment #26)
> Created an attachment (id=200387) [details]
> Patch
> 
Rebaseline failed test cases in previous patch.

[Dave Hyatt]

Comment on attachment 200387 [details]
Patch

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

Looks pretty good. A couple of comments.

> Source/WebCore/rendering/RenderTableSection.cpp:279
> +    Vector<RenderTableCell*> rowspanCells;

This should be rowSpanCells (capitalize the S).

I imagine this is empty or small most of the time, so maybe give it a small inline capacity, e.g.,

Vector<RenderTableCell*, 2> rowSpanCells;

> Source/WebCore/rendering/RenderTableSection.cpp:346
> +    if (rowspanCells.size() > 0) {

We should pull all this code into a helper function rather than making calcRowLogicalHeight so much bigger and just call the helper function if rowspanCells is non-empty.

if (!rowSpanCells.isEmpty())
    callHelperFunction(rowSpanCells);

[Suchit Agrawal]

Created attachment 200402 [details]
Patch

[Suchit Agrawal]

Created attachment 200405 [details]
Patch

[Suchit Agrawal]

(In reply to comment #28)
> (From update of attachment 200387 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200387&action=review
> 
> Looks pretty good. A couple of comments.
> 
> > Source/WebCore/rendering/RenderTableSection.cpp:279
> > +    Vector<RenderTableCell*> rowspanCells;
> 
> This should be rowSpanCells (capitalize the S).
> 
> I imagine this is empty or small most of the time, so maybe give it a small inline capacity, e.g.,
> 
3-4 rowSpan cells are very common so I used 5 for inline capacity.

> Vector<RenderTableCell*, 2> rowSpanCells;
> 
> > Source/WebCore/rendering/RenderTableSection.cpp:346
> > +    if (rowspanCells.size() > 0) {
> 
> We should pull all this code into a helper function rather than making calcRowLogicalHeight so much bigger and just call the helper function if rowspanCells is non-empty.
> 
> if (!rowSpanCells.isEmpty())
>     callHelperFunction(rowSpanCells);
> 
I put all this code in new function 'distributeRowSpanHeightToRows()' and did changes as per your comment.

[Dave Hyatt]

Comment on attachment 200405 [details]
Patch

r=me with some trepidation. Please be prepared for regressions if/when they do occur.

[WebKit Commit Bot]

Comment on attachment 200405 [details]
Patch

Rejecting attachment 200405 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 200405, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
g file LayoutTests/platform/qt/tables/mozilla_expected_failures/bugs/bug58402-2-expected.txt
patching file LayoutTests/platform/qt/tables/mozilla_expected_failures/bugs/bug6933-expected.txt
patching file LayoutTests/tables/mozilla/core/bloomberg-expected.txt
patching file LayoutTests/tables/mozilla_expected_failures/bugs/bug23847-expected.txt

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'David Hyatt']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/339371

[WebKit Commit Bot]

Comment on attachment 200405 [details]
Patch

Rejecting attachment 200405 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 200405, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
g file LayoutTests/platform/qt/tables/mozilla_expected_failures/bugs/bug58402-2-expected.txt
patching file LayoutTests/platform/qt/tables/mozilla_expected_failures/bugs/bug6933-expected.txt
patching file LayoutTests/tables/mozilla/core/bloomberg-expected.txt
patching file LayoutTests/tables/mozilla_expected_failures/bugs/bug23847-expected.txt

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'David Hyatt']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/464150

[Suchit Agrawal]

Created attachment 201535 [details]
Patch

[Suchit Agrawal]

Created attachment 201537 [details]
Patch

[Suchit Agrawal]

Comment on attachment 201537 [details]
Patch

This patch is already reviewed by David Hyatt and it is just rebaseline again.

[WebKit Commit Bot]

Comment on attachment 201537 [details]
Patch

Clearing flags on attachment: 201537

Committed r150023: <http://trac.webkit.org/changeset/150023>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

(In reply to comment #38)
> (From update of attachment 201537 [details])
> Clearing flags on attachment: 201537
> 
> Committed r150023: <http://trac.webkit.org/changeset/150023>

This caused a regression:

Bug 116118: REGRESSION (r150023): Table cells on buildbot waterfall page are squashed

[David Kilzer (:ddkilzer)]

(In reply to comment #40)
> (In reply to comment #38)
> > (From update of attachment 201537 [details] [details])
> > Clearing flags on attachment: 201537
> > 
> > Committed r150023: <http://trac.webkit.org/changeset/150023>
> 
> This caused a regression:
> 
> Bug 116118: REGRESSION (r150023): Table cells on buildbot waterfall page are squashed

r150023 also causes crashes on some buildbot pages (happened on an internal Safari buildbot page):

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x000000010ccf6e82 WebCore::RenderTableSection::distributeRowSpanHeightToRows(WTF::Vector<WebCore::RenderTableCell*, 5ul, WTF::CrashOnOverflow>&) + 2402
1   com.apple.WebCore             	0x000000010c280dab WebCore::RenderTableSection::calcRowLogicalHeight() + 1755
2   com.apple.WebCore             	0x000000010c27c9eb WebCore::RenderTable::layout() + 747
3   com.apple.WebCore             	0x000000010cc31733 WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&) + 723
4   com.apple.WebCore             	0x000000010cc2d8cb WebCore::RenderBlock::layoutBlockChildren(bool, WebCore::LayoutUnit&) + 747
5   com.apple.WebCore             	0x000000010cc2cd61 WebCore::RenderBlock::layoutBlock(bool, WebCore::LayoutUnit) + 1041
6   com.apple.WebCore             	0x000000010c1f9bf4 WebCore::RenderBlock::layout() + 52
7   com.apple.WebCore             	0x000000010cc31733 WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&) + 723
8   com.apple.WebCore             	0x000000010cc2d8cb WebCore::RenderBlock::layoutBlockChildren(bool, WebCore::LayoutUnit&) + 747
9   com.apple.WebCore             	0x000000010cc2cd61 WebCore::RenderBlock::layoutBlock(bool, WebCore::LayoutUnit) + 1041
10  com.apple.WebCore             	0x000000010c1f9bf4 WebCore::RenderBlock::layout() + 52
11  com.apple.WebCore             	0x000000010cc31733 WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&) + 723
12  com.apple.WebCore             	0x000000010cc2d8cb WebCore::RenderBlock::layoutBlockChildren(bool, WebCore::LayoutUnit&) + 747
13  com.apple.WebCore             	0x000000010cc2cd61 WebCore::RenderBlock::layoutBlock(bool, WebCore::LayoutUnit) + 1041
14  com.apple.WebCore             	0x000000010c1f9bf4 WebCore::RenderBlock::layout() + 52
15  com.apple.WebCore             	0x000000010cc31733 WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&) + 723
16  com.apple.WebCore             	0x000000010cc2d8cb WebCore::RenderBlock::layoutBlockChildren(bool, WebCore::LayoutUnit&) + 747
17  com.apple.WebCore             	0x000000010cc2cd61 WebCore::RenderBlock::layoutBlock(bool, WebCore::LayoutUnit) + 1041
18  com.apple.WebCore             	0x000000010c1f9bf4 WebCore::RenderBlock::layout() + 52
19  com.apple.WebCore             	0x000000010c1f9b32 WebCore::RenderView::layout() + 1186
20  com.apple.WebCore             	0x000000010c1f8e25 WebCore::FrameView::layout(bool) + 1765
21  com.apple.WebCore             	0x000000010c1eee4e WebCore::Document::implicitClose() + 750
22  com.apple.WebCore             	0x000000010c1eeae1 WebCore::FrameLoader::checkCompleted() + 337
23  com.apple.WebCore             	0x000000010c63ee52 WebCore::CachedResourceLoader::loadDone(WebCore::CachedResource*) + 66
24  com.apple.WebCore             	0x000000010cdc2462 WebCore::SubresourceLoader::releaseResources() + 82
25  com.apple.WebCore             	0x000000010c2506dd WebCore::SubresourceLoader::didFinishLoading(double) + 189
[…]

[WebKit Commit Bot]

Re-opened since this is blocked by bug 116120

[Suchit Agrawal]

Created attachment 203348 [details]
Patch

[Suchit Agrawal]

(In reply to comment #43)
> Created an attachment (id=203348) [details]
> Patch

Fix regression issues and tested. There was problem in rearranging rowSpan cells and small modification in calculating row height.

[Suchit Agrawal]

(In reply to comment #44)
> (In reply to comment #43)
> > Created an attachment (id=203348) [details] [details]
> > Patch
> 
> Fix regression issues and tested. There was problem in rearranging rowSpan cells and small modification in calculating row height.

Also divided a big function (distributeRowSpanHeightToRows) into small-small functions :
distributeRowSpanHeightToRows()
findEffectiveColsBetweenRows()
numberOfRowSpanCellsInRow()
updateRowsPosWithChangedHeight()
getRowsHeightInRowSpan()
distributeRowSpanHeightToRows()

[Build Bot]

Comment on attachment 203348 [details]
Patch

Attachment 203348 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/684165

New failing tests:
fast/dom/location-new-window-no-crash.html

[Build Bot]

Created attachment 203350 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Suchit Agrawal]

I have divided this patch in small-small test case and landed in blink.

First patch gives many regression and following next patches fix all the regressions.

I want to land those patched here also. If it is OK then I can start putting one by one patch.

[Brady Eidson]

Comment on attachment 203348 [details]
Patch

r- to clear stale patches from the review queue

[Ahmad Saleem]

Created attachment 461225 [details]
Safari 15.5 differs from other browsers

I am able to reproduce this bug in Safari 15.6 on macOS 12.5 using test case from Comment 0 converted into following JSFiddle:

Link - https://jsfiddle.net/u1kg9s8n/show

As can be seen from the attached screenshot, Safari 15.6 on macOS 12.5 differs from other browsers (Chrome Canary 106 and Firefox Nightly 104). Just wanted to update latest testing results. Thanks!

[Radar WebKit Bug Importer]

<rdar://problem/97641903>