| Summary: | Cell heights are disproportional when a row-spanning cell contains a block element that determines the height of the rows | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||||||||||||||||||||||||
| Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||
| Status: | REOPENED --- | ||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | ahmad.saleem792, ap, a.suchit, bdakin, bfulgham, buildbot, cdumez, commit-queue, ddkilzer, eflews.bot, esprehn+autocc, glenn, gyuyoung.kim, gyuyoung.kim, hyatt, jamesr, jchaffraix, pkasting, rakuco, rniwa, robert, simon.fraser, suchit.agrawal, webkit-bug-importer | ||||||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||||||||||||||||
| OS: | OS X 10.5 | ||||||||||||||||||||||||||||||||||
| URL: | http://3-1design.com/bug-report.php | ||||||||||||||||||||||||||||||||||
| Bug Depends on: | 116120 | ||||||||||||||||||||||||||||||||||
| Bug Blocks: | 116118 | ||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||
|
Description
Ryosuke Niwa
2011-01-10 18:15:35 PST
Note: WebKit's behavior here doesn't match IE/Fx. 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. Created attachment 200020 [details]
Patch
Comment on attachment 200020 [details] Patch Attachment 200020 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/67672 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 (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. Beth, Please review this patch and share your comment. Thanks. 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 Comment on attachment 200020 [details] Patch Attachment 200020 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/245084 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? (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. Created attachment 200080 [details]
Patch
(In reply to comment #12) > Created an attachment (id=200080) [details] > Patch > Changes in previous patch for compilation issue on mac and efl. Created attachment 200084 [details]
Patch
(In reply to comment #14) > Created an attachment (id=200084) [details] > Patch > Fixed compilation issue on mac and efl, Created attachment 200086 [details]
Patch
(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. 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 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
(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 Created attachment 200316 [details]
Patch
(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. 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 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
(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. Created attachment 200387 [details]
Patch
(In reply to comment #26) > Created an attachment (id=200387) [details] > Patch > Rebaseline failed test cases in previous patch. 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); Created attachment 200402 [details]
Patch
Created attachment 200405 [details]
Patch
(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. Comment on attachment 200405 [details]
Patch
r=me with some trepidation. Please be prepared for regressions if/when they do occur.
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 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 Created attachment 201535 [details]
Patch
Created attachment 201537 [details]
Patch
Comment on attachment 201537 [details]
Patch
This patch is already reviewed by David Hyatt and it is just rebaseline again.
Comment on attachment 201537 [details] Patch Clearing flags on attachment: 201537 Committed r150023: <http://trac.webkit.org/changeset/150023> All reviewed patches have been landed. Closing bug. (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 (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 […] Re-opened since this is blocked by bug 116120 Created attachment 203348 [details]
Patch
(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. (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() 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 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
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. Comment on attachment 203348 [details]
Patch
r- to clear stale patches from the review queue
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! |