Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(7)

Issue 47923009: Table rows are incorrectly collapsed in case of hidden cells and rowspans. (Closed)

Created:
5 years, 2 months ago by a.suchit
Modified:
4 years, 10 months ago
CC:
blink-reviews, bemjb+rendering_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, zoltan1
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Table rows are incorrectly collapsed in case of hidden cells and rowspans. When all rows in spanning cell are having only row spanning cells with empty cell than None of the rows in spanning cell was not getting any height. Total height of the rows in spanning cell should have at least spanning cell height so apply spanning cell height in the last row of the spanning cells if all rows in spanning cell are have zero height. The total height of the rows in a spanning cell should be at least the height of the spanning cell's itself because content should fit in the cell. This change adds the remaining height to the last row because every row is belong to this cell only. R=jchaffraix@chromium.org BUG=258420, 330564, 341366, 346150 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168326

Patch Set 1 : #

Total comments: 10

Patch Set 2 : Review comments Addressed #

Total comments: 6

Patch Set 3 : Review comments addressed #

Total comments: 7

Patch Set 4 : Review comments addressed #

Patch Set 5 : #

Total comments: 6

Patch Set 6 : Review comments addressed #

Total comments: 9

Patch Set 7 : Added why comment for the change #

Patch Set 8 : Patch updated #

Total comments: 4

Patch Set 9 : Review comments addressed #

Messages

Total messages: 22 (0 generated)
suchit.agrawal
5 years, 2 months ago (2013-10-29 12:45:55 UTC) #1
a.suchit2
5 years, 2 months ago (2013-10-29 12:48:05 UTC) #2
Julien - ping for review
"Total height of the rows in spanning cell should have at least spanning cell height ...
5 years, 2 months ago (2013-10-30 18:12:43 UTC) #3
suchit.agrawal
I found some more scenarios those would not work properly when cell display property set ...
5 years, 2 months ago (2013-10-31 13:43:02 UTC) #4
Julien - ping for review
https://codereview.chromium.org/47923009/diff/320001/LayoutTests/fast/table/table-rowspan-cell-with-empty-cell.html File LayoutTests/fast/table/table-rowspan-cell-with-empty-cell.html (right): https://codereview.chromium.org/47923009/diff/320001/LayoutTests/fast/table/table-rowspan-cell-with-empty-cell.html#newcode9 LayoutTests/fast/table/table-rowspan-cell-with-empty-cell.html:9: .red {background: blue;} .red -> background blue There is ...
5 years, 2 months ago (2013-11-13 07:13:17 UTC) #5
a.suchit
https://codereview.chromium.org/47923009/diff/320001/LayoutTests/fast/table/table-rowspan-cell-with-empty-cell.html File LayoutTests/fast/table/table-rowspan-cell-with-empty-cell.html (right): https://codereview.chromium.org/47923009/diff/320001/LayoutTests/fast/table/table-rowspan-cell-with-empty-cell.html#newcode9 LayoutTests/fast/table/table-rowspan-cell-with-empty-cell.html:9: .red {background: blue;} On 2013/11/13 07:13:17, Julien Chaffraix - ...
5 years, 2 months ago (2013-11-19 11:59:56 UTC) #6
Julien - ping for review
https://codereview.chromium.org/47923009/diff/410001/Source/core/rendering/RenderTableSection.cpp File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/47923009/diff/410001/Source/core/rendering/RenderTableSection.cpp#newcode550 Source/core/rendering/RenderTableSection.cpp:550: // and cell have rowspan=1. I have a hard ...
5 years, 2 months ago (2013-11-21 02:46:23 UTC) #7
suchit.agrawal
https://codereview.chromium.org/47923009/diff/410001/Source/core/rendering/RenderTableSection.cpp File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/47923009/diff/410001/Source/core/rendering/RenderTableSection.cpp#newcode550 Source/core/rendering/RenderTableSection.cpp:550: // and cell have rowspan=1. On 2013/11/21 02:46:23, Julien ...
5 years, 1 month ago (2013-11-22 11:49:41 UTC) #8
a.suchit
On 2013/11/22 11:49:41, suchit.agrawal wrote: > https://codereview.chromium.org/47923009/diff/410001/Source/core/rendering/RenderTableSection.cpp > File Source/core/rendering/RenderTableSection.cpp (right): > > https://codereview.chromium.org/47923009/diff/410001/Source/core/rendering/RenderTableSection.cpp#newcode550 > ...
5 years, 1 month ago (2013-12-17 05:42:17 UTC) #9
Julien - ping for review
> Hi Julien, I was on vacation. > > Please review this issue. Hi Suchit, ...
5 years, 1 month ago (2013-12-18 14:15:52 UTC) #10
a.suchit
On 2013/12/18 14:15:52, Julien Chaffraix - CET wrote: > > Hi Julien, I was on ...
5 years ago (2014-01-10 06:04:29 UTC) #11
a.suchit
On 2014/01/10 06:04:29, a.suchit wrote: > On 2013/12/18 14:15:52, Julien Chaffraix - CET wrote: > ...
4 years, 12 months ago (2014-01-23 07:39:58 UTC) #12
Julien - ping for review
Thanks for the long explanation and sorry for the delay, it seems OK to do ...
4 years, 12 months ago (2014-01-23 23:54:55 UTC) #13
a.suchit
https://codereview.chromium.org/47923009/diff/710001/Source/core/rendering/RenderTableSection.cpp File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/47923009/diff/710001/Source/core/rendering/RenderTableSection.cpp#newcode543 Source/core/rendering/RenderTableSection.cpp:543: // Here we are handling only rows those have ...
4 years, 11 months ago (2014-01-28 11:59:09 UTC) #14
Julien - ping for review
https://codereview.chromium.org/47923009/diff/840001/Source/core/rendering/RenderTableSection.cpp File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/47923009/diff/840001/Source/core/rendering/RenderTableSection.cpp#newcode291 Source/core/rendering/RenderTableSection.cpp:291: if (!spanningRowsHeight.rowWithOnlySpanningCells && !spanningRowsHeight.rowHeight[row]) You misunderstood what I said: ...
4 years, 11 months ago (2014-02-06 20:30:44 UTC) #15
a.suchit
https://codereview.chromium.org/47923009/diff/840001/Source/core/rendering/RenderTableSection.cpp File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/47923009/diff/840001/Source/core/rendering/RenderTableSection.cpp#newcode291 Source/core/rendering/RenderTableSection.cpp:291: if (!spanningRowsHeight.rowWithOnlySpanningCells && !spanningRowsHeight.rowHeight[row]) On 2014/02/06 20:30:45, Julien Chaffraix ...
4 years, 11 months ago (2014-02-18 13:07:10 UTC) #16
a.suchit
On 2014/02/18 13:07:10, a.suchit wrote: > https://codereview.chromium.org/47923009/diff/840001/Source/core/rendering/RenderTableSection.cpp > File Source/core/rendering/RenderTableSection.cpp (right): > > https://codereview.chromium.org/47923009/diff/840001/Source/core/rendering/RenderTableSection.cpp#newcode291 > ...
4 years, 10 months ago (2014-02-25 00:26:27 UTC) #17
Julien - ping for review
lgtm. If there is a need for another of these changes, I will request the ...
4 years, 10 months ago (2014-02-26 22:29:30 UTC) #18
a.suchit
The CQ bit was checked by a.suchit@samsung.com
4 years, 10 months ago (2014-03-03 12:22:49 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a.suchit@samsung.com/47923009/1100001
4 years, 10 months ago (2014-03-03 12:23:01 UTC) #20
a.suchit
https://codereview.chromium.org/47923009/diff/1060001/Source/core/rendering/RenderTableSection.cpp File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/47923009/diff/1060001/Source/core/rendering/RenderTableSection.cpp#newcode292 Source/core/rendering/RenderTableSection.cpp:292: spanningRowsHeight.rowWithOnlySpanningCells = rowHasOnlySpanningCells(actualRow); On 2014/02/26 22:29:31, Julien Chaffraix - ...
4 years, 10 months ago (2014-03-03 12:28:31 UTC) #21
commit-bot: I haz the power
4 years, 10 months ago (2014-03-03 19:59:17 UTC) #22
Message was sent while issue was closed.
Change committed as 168326

Powered by Google App Engine
This is Rietveld 408576698