Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
47923009:
Table rows are incorrectly collapsed in case of hidden cells and rowspans. (Closed)
|
Created:
7 years, 1 month ago by a.suchit Modified:
6 years, 9 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. |
DescriptionTable 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)
"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."
I have a hard time understanding this (even though I read the code). That comes
partly because some English use are off ("should be", not "should have" I
think?). Here is my take, feel free to modify as appropriate:
The total height of the rows in a spanning cell should be at least the height of
the spanning cell's itself. This change adds the remaining height to the last
row (note: why is that desirable or even correct?)
https://codereview.chromium.org/47923009/diff/90001/LayoutTests/fast/table/ta...
File LayoutTests/fast/table/table-rowspan-cell-with-empty-cell.html (right):
https://codereview.chromium.org/47923009/diff/90001/LayoutTests/fast/table/ta...
LayoutTests/fast/table/table-rowspan-cell-with-empty-cell.html:7: .red
{background: red;}
Red should be reserved for failures.
https://codereview.chromium.org/47923009/diff/90001/LayoutTests/fast/table/ta...
LayoutTests/fast/table/table-rowspan-cell-with-empty-cell.html:13: <h4>When all
rows in row spanning cell are having only spanning cells with empty cell/cells
then None of the rows in row spanning cell was not getting any height.</h4>
None -> none (this is not a Python program :))
I think we could make the sentence more palatable: A spanning cell whose rows
have only empty cell(s) shouldn't have a non-zero height.
https://codereview.chromium.org/47923009/diff/90001/LayoutTests/fast/table/ta...
LayoutTests/fast/table/table-rowspan-cell-with-empty-cell.html:14: <h4>A2 row
should not collapse with other row in the table.</h4>
Can't we make that a check-layout.js test? It seems *very* easy to check that
something is not 0 or has the right size.
https://codereview.chromium.org/47923009/diff/90001/Source/core/rendering/Ren...
File Source/core/rendering/RenderTableSection.cpp (right):
https://codereview.chromium.org/47923009/diff/90001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:549: if
(spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing) {
This 'if' is unneeded: to get into this branch, you have to satisfy the exact
same condition above.
https://codereview.chromium.org/47923009/diff/90001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:550:
spanningRowsHeight.totalRowsHeight =
spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing +
borderSpacingForRow(rowIndex + rowSpan -1);
Please add a comment about what this line is supposed to do. It seems like the
next branch will definitely be impacted by this line (you change
spanningRowsHeight.totalRowsHeight to be non-zero above).
Also why do we do it here and not when populating spanningRowsHeight?
I found some more scenarios those would not work properly when cell display property set as none. If it looks good to you then let it go and later we can come with other scenarios and fix them. But those scenarios would be rarely seen in real. those are just mixture of rowspan cells and cells with property display:none. https://codereview.chromium.org/47923009/diff/90001/LayoutTests/fast/table/ta... File LayoutTests/fast/table/table-rowspan-cell-with-empty-cell.html (right): https://codereview.chromium.org/47923009/diff/90001/LayoutTests/fast/table/ta... LayoutTests/fast/table/table-rowspan-cell-with-empty-cell.html:7: .red {background: red;} On 2013/10/30 18:12:44, Julien Chaffraix - PST wrote: > Red should be reserved for failures. Changed to blue. https://codereview.chromium.org/47923009/diff/90001/LayoutTests/fast/table/ta... LayoutTests/fast/table/table-rowspan-cell-with-empty-cell.html:13: <h4>When all rows in row spanning cell are having only spanning cells with empty cell/cells then None of the rows in row spanning cell was not getting any height.</h4> On 2013/10/30 18:12:44, Julien Chaffraix - PST wrote: > None -> none (this is not a Python program :)) > > I think we could make the sentence more palatable: A spanning cell whose rows > have only empty cell(s) shouldn't have a non-zero height. Done. https://codereview.chromium.org/47923009/diff/90001/LayoutTests/fast/table/ta... LayoutTests/fast/table/table-rowspan-cell-with-empty-cell.html:14: <h4>A2 row should not collapse with other row in the table.</h4> On 2013/10/30 18:12:44, Julien Chaffraix - PST wrote: > Can't we make that a check-layout.js test? It seems *very* easy to check that > something is not 0 or has the right size. Done. https://codereview.chromium.org/47923009/diff/90001/Source/core/rendering/Ren... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/47923009/diff/90001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:549: if (spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing) { On 2013/10/30 18:12:44, Julien Chaffraix - PST wrote: > This 'if' is unneeded: to get into this branch, you have to satisfy the exact > same condition above. Previous condition is wrong. Forgot to remove it from there when it is added inside. Thanks https://codereview.chromium.org/47923009/diff/90001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:550: spanningRowsHeight.totalRowsHeight = spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing + borderSpacingForRow(rowIndex + rowSpan -1); On 2013/10/30 18:12:44, Julien Chaffraix - PST wrote: > Please add a comment about what this line is supposed to do. It seems like the > next branch will definitely be impacted by this line (you change > spanningRowsHeight.totalRowsHeight to be non-zero above). > > Also why do we do it here and not when populating spanningRowsHeight? spanningRowsHeight.totalRowsHeight change is not required because just after that continue is used.
https://codereview.chromium.org/47923009/diff/320001/LayoutTests/fast/table/t... File LayoutTests/fast/table/table-rowspan-cell-with-empty-cell.html (right): https://codereview.chromium.org/47923009/diff/320001/LayoutTests/fast/table/t... LayoutTests/fast/table/table-rowspan-cell-with-empty-cell.html:9: .red {background: blue;} .red -> background blue There is probably a mismatch in the class name here. https://codereview.chromium.org/47923009/diff/320001/Source/core/rendering/Re... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/47923009/diff/320001/Source/core/rendering/Re... Source/core/rendering/RenderTableSection.cpp:551: if (spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing) Please add a comment as to why we do this, ideally including some simple example. https://codereview.chromium.org/47923009/diff/320001/Source/core/rendering/Re... Source/core/rendering/RenderTableSection.cpp:552: m_rowPos[rowIndex + rowSpan] += spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing + borderSpacingForRow(rowIndex + rowSpan -1); Coding style violation: there should be a space after '-'.
https://codereview.chromium.org/47923009/diff/320001/LayoutTests/fast/table/t... File LayoutTests/fast/table/table-rowspan-cell-with-empty-cell.html (right): https://codereview.chromium.org/47923009/diff/320001/LayoutTests/fast/table/t... LayoutTests/fast/table/table-rowspan-cell-with-empty-cell.html:9: .red {background: blue;} On 2013/11/13 07:13:17, Julien Chaffraix - AEDT wrote: > .red -> background blue > > There is probably a mismatch in the class name here. Done. https://codereview.chromium.org/47923009/diff/320001/Source/core/rendering/Re... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/47923009/diff/320001/Source/core/rendering/Re... Source/core/rendering/RenderTableSection.cpp:551: if (spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing) On 2013/11/13 07:13:17, Julien Chaffraix - AEDT wrote: > Please add a comment as to why we do this, ideally including some simple > example. Done. https://codereview.chromium.org/47923009/diff/320001/Source/core/rendering/Re... Source/core/rendering/RenderTableSection.cpp:552: m_rowPos[rowIndex + rowSpan] += spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing + borderSpacingForRow(rowIndex + rowSpan -1); On 2013/11/13 07:13:17, Julien Chaffraix - AEDT wrote: > Coding style violation: there should be a space after '-'. Done.
https://codereview.chromium.org/47923009/diff/410001/Source/core/rendering/Re... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/47923009/diff/410001/Source/core/rendering/Re... Source/core/rendering/RenderTableSection.cpp:550: // and cell have rowspan=1. I have a hard time putting this line in context. Based on your following example, it seems that you should just say that "we only take into account non-empty rows, which are rows with at least a visible (display != none) cell. https://codereview.chromium.org/47923009/diff/410001/Source/core/rendering/Re... Source/core/rendering/RenderTableSection.cpp:562: // display. I don't think I understand how this relates to the algorithm. If a cell has 'display: none' then it doesn't have a renderer so there is no height to spread anyway. https://codereview.chromium.org/47923009/diff/410001/Source/core/rendering/Re... Source/core/rendering/RenderTableSection.cpp:566: // 2 conditions. So C rowspan cell is getting 0 height. Could you explain what the rowspan cell is? Is this the empty row that is spanned? https://codereview.chromium.org/47923009/diff/410001/Source/core/rendering/Re... Source/core/rendering/RenderTableSection.cpp:569: // expected. FIXMEs are great but they are better when they are _actionable_. Here I have no clue what a scenario where we would be wrong is.
https://codereview.chromium.org/47923009/diff/410001/Source/core/rendering/Re... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/47923009/diff/410001/Source/core/rendering/Re... Source/core/rendering/RenderTableSection.cpp:550: // and cell have rowspan=1. On 2013/11/21 02:46:23, Julien Chaffraix - AEDT wrote: > I have a hard time putting this line in context. > > Based on your following example, it seems that you should just say that "we only > take into account non-empty rows, which are rows with at least a visible > (display != none) cell. Thanks, I am tring to explain same thing here. https://codereview.chromium.org/47923009/diff/410001/Source/core/rendering/Re... Source/core/rendering/RenderTableSection.cpp:562: // display. On 2013/11/21 02:46:23, Julien Chaffraix - AEDT wrote: > I don't think I understand how this relates to the algorithm. If a cell has > 'display: none' then it doesn't have a renderer so there is no height to spread > anyway. Here you have explained my 1 above and 2 below lines in small sentence. D and E are not have any height to spread in C. So C height remains 0. So here we are applying C logical height in last row of rowspan cell. https://codereview.chromium.org/47923009/diff/410001/Source/core/rendering/Re... Source/core/rendering/RenderTableSection.cpp:569: // expected. On 2013/11/21 02:46:23, Julien Chaffraix - AEDT wrote: > FIXMEs are great but they are better when they are _actionable_. Here I have no > clue what a scenario where we would be wrong is. x---x---x---x---x | A | B | C | D | row1 x---x---x---x---x | | | G | row2 | | F X---x | | | | row3 | E x---x H x | | | | row4 | | I x---x | | | J | row5 x---x---x---x I was thinking for like this scenarios if G and J are hidden (display:none). Here, rowspan cell's logical height goes in the last row of rowspan cell. Assume F, H and I heights are 10 each. H should be part of row3 and row4 but in our case it would be fully part of only row4. F should be part of row2 and row3 but in our case it would be part of only row3. I should be part of row4 and row5 but in our case it would be part of only row4. Then display will come like below :- x---x---x---x---x | A | B | C | D | row1 x---x---x---x---x row2 height would be 0. | | | | | F X row3 | | | | E x---x---x | | | | | | I x H x row4 | | | | x---x---x---x row5 height would be 0. But it should look like :- x---x---x---x---x | A | B | C | D | row1 x---x---x---x---x row2 height would be 0. | | | | | | F | | row3 | | | | | E x---x H x | | | | | | I | | row4 | | | | x---x---x---x row5 height would be 0.
On 2013/11/22 11:49:41, suchit.agrawal wrote: > https://codereview.chromium.org/47923009/diff/410001/Source/core/rendering/Re... > File Source/core/rendering/RenderTableSection.cpp (right): > > https://codereview.chromium.org/47923009/diff/410001/Source/core/rendering/Re... > Source/core/rendering/RenderTableSection.cpp:550: // and cell have rowspan=1. > On 2013/11/21 02:46:23, Julien Chaffraix - AEDT wrote: > > I have a hard time putting this line in context. > > > > Based on your following example, it seems that you should just say that "we > only > > take into account non-empty rows, which are rows with at least a visible > > (display != none) cell. > > Thanks, I am tring to explain same thing here. > > https://codereview.chromium.org/47923009/diff/410001/Source/core/rendering/Re... > Source/core/rendering/RenderTableSection.cpp:562: // display. > On 2013/11/21 02:46:23, Julien Chaffraix - AEDT wrote: > > I don't think I understand how this relates to the algorithm. If a cell has > > 'display: none' then it doesn't have a renderer so there is no height to > spread > > anyway. > > Here you have explained my 1 above and 2 below lines in small sentence. > D and E are not have any height to spread in C. So C height remains 0. > So here we are applying C logical height in last row of rowspan cell. > > https://codereview.chromium.org/47923009/diff/410001/Source/core/rendering/Re... > Source/core/rendering/RenderTableSection.cpp:569: // expected. > On 2013/11/21 02:46:23, Julien Chaffraix - AEDT wrote: > > FIXMEs are great but they are better when they are _actionable_. Here I have > no > > clue what a scenario where we would be wrong is. > > x---x---x---x---x > | A | B | C | D | row1 > x---x---x---x---x > | | | G | row2 > | | F X---x > | | | | row3 > | E x---x H x > | | | | row4 > | | I x---x > | | | J | row5 > x---x---x---x > > I was thinking for like this scenarios if G and J are hidden (display:none). > Here, rowspan cell's logical height goes in the last row of rowspan cell. > Assume > F, H and I heights are 10 each. > H should be part of row3 and row4 but in our case it would be fully part of only > row4. > F should be part of row2 and row3 but in our case it would be part of only row3. > I should be part of row4 and row5 but in our case it would be part of only row4. > Then display will come like below :- > x---x---x---x---x > | A | B | C | D | row1 > x---x---x---x---x row2 height would be 0. > | | | > | | F X row3 > | | | > | E x---x---x > | | | | > | | I x H x row4 > | | | | > x---x---x---x row5 height would be 0. > > But it should look like :- > > x---x---x---x---x > | A | B | C | D | row1 > x---x---x---x---x row2 height would be 0. > | | | | > | | F | | row3 > | | | | > | E x---x H x > | | | | > | | I | | row4 > | | | | > x---x---x---x row5 height would be 0. Hi Julien, I was on vacation. Please review this issue.
> Hi Julien, I was on vacation. > > Please review this issue. Hi Suchit, could you update the patch's comments along the lines I pointed out?
On 2013/12/18 14:15:52, Julien Chaffraix - CET wrote: > > Hi Julien, I was on vacation. > > > > Please review this issue. > > Hi Suchit, could you update the patch's comments along the lines I pointed out? Hi Julien, Patch is updated. Can you please review it. thanks
On 2014/01/10 06:04:29, a.suchit wrote:
> On 2013/12/18 14:15:52, Julien Chaffraix - CET wrote:
> > > Hi Julien, I was on vacation.
> > >
> > > Please review this issue.
> >
> > Hi Suchit, could you update the patch's comments along the lines I pointed
> out?
>
> Hi Julien, Patch is updated. Can you please review it. thanks
As per our fix for rowspan issue in table, we have applied below algo :
1. Collect all row spanning cells.
2. sort them in the order in which we need to distribute extra row spanning
height in rows
3. Apply one by one row spanning cell's extra height in rows
1. Getting spanning row's height and rowspan's cell height.
2. if only spanning cells are present in spanning rows then apply height based
on spanning cells.
else distribute extra height in percent rows, auto rows and fixed rows.
respectively.
Here we are missing some of the spanning cells those do not come under Only
spanning cells row and do not able to distribute their heights in spanning row's
because all spanning row's height is 0. So case :
2. if only spanning cells are present in spanning rows then apply height based
on spanning cells.
else distribute extra height in percent rows, auto rows and fixed rows.
respectively.
does not work for these spanning cells and these spanning cells remains same
with 0 height.
So in this patch, I have changed case :
2. if only spanning cells are present in spanning rows then apply height based
on spanning cells.
else distribute extra height in percent rows, auto rows and fixed rows.
respectively.
TO
2. if only spanning cells are present in spanning rows then apply height based
on spanning cells.
else if total height of spanning rows are 0 then apply rowspanning cells
height here.
else distribute extra height in percent rows, auto rows and fixed rows.
respectively.
@Julien, Please check it once and help me that how should I go future? Give your
opinion and suggestions to improve it. Thanks
Thanks for the long explanation and sorry for the delay, it seems OK to do what you said but I really would like us to put good quality comments as this code is getting complex. https://codereview.chromium.org/47923009/diff/710001/Source/core/rendering/Re... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/47923009/diff/710001/Source/core/rendering/Re... Source/core/rendering/RenderTableSection.cpp:543: // Here we are handling only rows those have at least one visible cell without rowspan value. This comment really confuses me: it implies that populateSpanningRowsHeightFromCell is doing some work besides just populating SpanningRowsHeight. https://codereview.chromium.org/47923009/diff/710001/Source/core/rendering/Re... Source/core/rendering/RenderTableSection.cpp:547: if (spanningRowsHeight.rowWithOnlySpanningCells) I was double-checking how we compute rowWithOnlySpanningCells and it seems wrong: spanningRowsHeight.rowWithOnlySpanningCells |= rowHasOnlySpanningCells(actualRow); (I think it should be &= else the variable is misnamed) https://codereview.chromium.org/47923009/diff/710001/Source/core/rendering/Re... Source/core/rendering/RenderTableSection.cpp:550: // Here we are handling only rows those have rowspanning cell(s) and at least one empty cell. s/those/who/ or s/those/that/ How do you determine that there is one empty cell? (I thought you negated the previous condition but not really) I think putting the explanation you put in comment #12 would be helpful here.
https://codereview.chromium.org/47923009/diff/710001/Source/core/rendering/Re... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/47923009/diff/710001/Source/core/rendering/Re... Source/core/rendering/RenderTableSection.cpp:543: // Here we are handling only rows those have at least one visible cell without rowspan value. On 2014/01/23 23:54:56, Julien Chaffraix - PST wrote: > This comment really confuses me: it implies that > populateSpanningRowsHeightFromCell is doing some work besides just populating > SpanningRowsHeight. Sorry It is wrong comment here. https://codereview.chromium.org/47923009/diff/710001/Source/core/rendering/Re... Source/core/rendering/RenderTableSection.cpp:547: if (spanningRowsHeight.rowWithOnlySpanningCells) On 2014/01/23 23:54:56, Julien Chaffraix - PST wrote: > I was double-checking how we compute rowWithOnlySpanningCells and it seems > wrong: > > spanningRowsHeight.rowWithOnlySpanningCells |= > rowHasOnlySpanningCells(actualRow); > > (I think it should be &= else the variable is misnamed) Here, we are checking for any one row have only spanning cells. So better we can change it to if (!spanningRowsHeight.rowWithOnlySpanningCells && !spanningRowsHeight.rowHeight[row]) spanningRowsHeight.rowWithOnlySpanningCells = rowHasOnlySpanningCells(actualRow); https://codereview.chromium.org/47923009/diff/710001/Source/core/rendering/Re... Source/core/rendering/RenderTableSection.cpp:550: // Here we are handling only rows those have rowspanning cell(s) and at least one empty cell. On 2014/01/23 23:54:56, Julien Chaffraix - PST wrote: > s/those/who/ or s/those/that/ > > How do you determine that there is one empty cell? (I thought you negated the > previous condition but not really) > > I think putting the explanation you put in comment #12 would be helpful here. spanningRowsHeight.totalRowsHeight is spanning cell's height. Here 'spanning cell's height is 0' would be only possible when row have 'Only spanning cell(s)' OR 'spanning cell(s) + empty cell(s)'. But 'Only spanning cell(s)' case is not possible because this case is already handled above (by updateRowsHeightHavingOnlySpanningCells). So there is only possibility "row have 'spanning cell(s) + empty cell(s)'"
https://codereview.chromium.org/47923009/diff/840001/Source/core/rendering/Re... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/47923009/diff/840001/Source/core/rendering/Re... Source/core/rendering/RenderTableSection.cpp:291: if (!spanningRowsHeight.rowWithOnlySpanningCells && !spanningRowsHeight.rowHeight[row]) You misunderstood what I said: this code is equivalent to the previous one and I was fine with using a logical OR. I don't see the value in this part of the change. If you name this variable rowWithOnlySpanningCells, it means that any row that *doesn't* have spanning cells should turn it to false but here it's the opposite. That makes wonder if it's not wrongly named. https://codereview.chromium.org/47923009/diff/840001/Source/core/rendering/Re... Source/core/rendering/RenderTableSection.cpp:551: // Here we are handling only row(s) who have rowspanning cell(s) and at least one empty cell. I tried reading those comments several times but as I keep pointing out, they are just mirroring the code and thus don't add value. What we need is 'why' comments (why is it correct to do that?) or comments adding values to our code ('what' comments that go beyond just the code by giving easy to understand examples or helping the reader follow the high level algorithm). https://codereview.chromium.org/47923009/diff/840001/Source/core/rendering/Re... Source/core/rendering/RenderTableSection.cpp:554: m_rowPos[rowIndex + rowSpan] += spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing + borderSpacingForRow(rowIndex + rowSpan - 1); This should be spanningCellEndIndex for consistency. https://codereview.chromium.org/47923009/diff/840001/Source/core/rendering/Re... Source/core/rendering/RenderTableSection.cpp:557: continue; My biggest issue with this code is that it will wrongly handle the case where you only have spanning cells maybe with empty cells and some set row's height. Also it is allocating the extra space to the last row in a rowspan (m_rowPos[rowIndex + rowSpan]). https://codereview.chromium.org/47923009/diff/840001/Source/core/rendering/Re... Source/core/rendering/RenderTableSection.cpp:561: extraHeightToPropagate = m_rowPos[rowIndex + rowSpan] - originalBeforePosition; How does this ensures that we allocate all of spanningCellHeightIgnoringBorderSpacing?
https://codereview.chromium.org/47923009/diff/840001/Source/core/rendering/Re... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/47923009/diff/840001/Source/core/rendering/Re... Source/core/rendering/RenderTableSection.cpp:291: if (!spanningRowsHeight.rowWithOnlySpanningCells && !spanningRowsHeight.rowHeight[row]) On 2014/02/06 20:30:45, Julien Chaffraix - PST wrote: > You misunderstood what I said: this code is equivalent to the previous one and I > was fine with using a logical OR. I don't see the value in this part of the > change. > > If you name this variable rowWithOnlySpanningCells, it means that any row that > *doesn't* have spanning cells should turn it to false but here it's the > opposite. That makes wonder if it's not wrongly named. We are checking all the rows of rowspanning cell. If any row is having only spanning cells and empty cells are considered as non rowspanning cell then |rowWithOnlySpanningCells| becomes true. At least one spanning row is having only spanning cells than |rowWithOnlySpanningCells| becomes true https://codereview.chromium.org/47923009/diff/840001/Source/core/rendering/Re... Source/core/rendering/RenderTableSection.cpp:551: // Here we are handling only row(s) who have rowspanning cell(s) and at least one empty cell. On 2014/02/06 20:30:45, Julien Chaffraix - PST wrote: > I tried reading those comments several times but as I keep pointing out, they > are just mirroring the code and thus don't add value. What we need is 'why' > comments (why is it correct to do that?) or comments adding values to our code > ('what' comments that go beyond just the code by giving easy to understand > examples or helping the reader follow the high level algorithm). Done. https://codereview.chromium.org/47923009/diff/840001/Source/core/rendering/Re... Source/core/rendering/RenderTableSection.cpp:554: m_rowPos[rowIndex + rowSpan] += spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing + borderSpacingForRow(rowIndex + rowSpan - 1); On 2014/02/06 20:30:45, Julien Chaffraix - PST wrote: > This should be spanningCellEndIndex for consistency. Done. https://codereview.chromium.org/47923009/diff/840001/Source/core/rendering/Re... Source/core/rendering/RenderTableSection.cpp:561: extraHeightToPropagate = m_rowPos[rowIndex + rowSpan] - originalBeforePosition; On 2014/02/06 20:30:45, Julien Chaffraix - PST wrote: > How does this ensures that we allocate all of > spanningCellHeightIgnoringBorderSpacing? Spanning rows total height is already more then the spanning cell height with ignoring border space. So If spanning cell already have enough height which is equal or more than specified by user for this spanning cell then we are leaving spanning cell with same height.
On 2014/02/18 13:07:10, a.suchit wrote: > https://codereview.chromium.org/47923009/diff/840001/Source/core/rendering/Re... > File Source/core/rendering/RenderTableSection.cpp (right): > > https://codereview.chromium.org/47923009/diff/840001/Source/core/rendering/Re... > Source/core/rendering/RenderTableSection.cpp:291: if > (!spanningRowsHeight.rowWithOnlySpanningCells && > !spanningRowsHeight.rowHeight[row]) > On 2014/02/06 20:30:45, Julien Chaffraix - PST wrote: > > You misunderstood what I said: this code is equivalent to the previous one and > I > > was fine with using a logical OR. I don't see the value in this part of the > > change. > > > > If you name this variable rowWithOnlySpanningCells, it means that any row that > > *doesn't* have spanning cells should turn it to false but here it's the > > opposite. That makes wonder if it's not wrongly named. > > We are checking all the rows of rowspanning cell. If any row is having only > spanning cells and empty cells are considered as non rowspanning cell then > |rowWithOnlySpanningCells| becomes true. > > At least one spanning row is having only spanning cells than > |rowWithOnlySpanningCells| becomes true > > https://codereview.chromium.org/47923009/diff/840001/Source/core/rendering/Re... > Source/core/rendering/RenderTableSection.cpp:551: // Here we are handling only > row(s) who have rowspanning cell(s) and at least one empty cell. > On 2014/02/06 20:30:45, Julien Chaffraix - PST wrote: > > I tried reading those comments several times but as I keep pointing out, they > > are just mirroring the code and thus don't add value. What we need is 'why' > > comments (why is it correct to do that?) or comments adding values to our code > > ('what' comments that go beyond just the code by giving easy to understand > > examples or helping the reader follow the high level algorithm). > > Done. > > https://codereview.chromium.org/47923009/diff/840001/Source/core/rendering/Re... > Source/core/rendering/RenderTableSection.cpp:554: m_rowPos[rowIndex + rowSpan] > += spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing + > borderSpacingForRow(rowIndex + rowSpan - 1); > On 2014/02/06 20:30:45, Julien Chaffraix - PST wrote: > > This should be spanningCellEndIndex for consistency. > > Done. > > https://codereview.chromium.org/47923009/diff/840001/Source/core/rendering/Re... > Source/core/rendering/RenderTableSection.cpp:561: extraHeightToPropagate = > m_rowPos[rowIndex + rowSpan] - originalBeforePosition; > On 2014/02/06 20:30:45, Julien Chaffraix - PST wrote: > > How does this ensures that we allocate all of > > spanningCellHeightIgnoringBorderSpacing? > > Spanning rows total height is already more then the spanning cell height with > ignoring border space. > So If spanning cell already have enough height which is equal or more than > specified by user for this spanning cell then we are leaving spanning cell with > same height. @julien, Please review it and give your comments. Why comment is added for change and fixed review comments. Thanks
lgtm. If there is a need for another of these changes, I will request the flag to be added (we shouldn't have removed it in the first place :() and the algorithm to be formally described in some document before we proceed any further. https://codereview.chromium.org/47923009/diff/1060001/Source/core/rendering/R... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/47923009/diff/1060001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:292: spanningRowsHeight.rowWithOnlySpanningCells = rowHasOnlySpanningCells(actualRow); As mentioned previously, this change should be removed as it doesn't add anything. The fact that you *had* to explain what it is supposed to mean (and what you said just mirrored the code) underlines that this variable is badly named. Note that booleans should have a verb, which is not the case anyway so we could kill 2 birds with one stone. https://codereview.chromium.org/47923009/diff/1060001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:557: // So here, we are giving text'height of spanning cell to the last row of spanning cell. First and this is a recurring issue, I have a hard time understanding what you mean and that's only because I can't parse your English. My advice would be to stick to very simple sentences, splitting complex ones into more simple. Also be mindful of the conjugation and word used. // This code handle row(s) that have rowspanning cell(s) and at least one empty cell. // Such rows are not handled below and end up having a height of 0. That would mean // content overlapping if one of their cells has any content. To avoid the problem, we // add all the remaining spanning cells' height to the last spanned row. // This means that we could grow a row past its 'height' or break percentage spreading // however this is better than overlapping content. FIXME: Is there a better algorithm?
The CQ bit was checked by a.suchit@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a.suchit@samsung.com/47923009/1100001
https://codereview.chromium.org/47923009/diff/1060001/Source/core/rendering/R... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/47923009/diff/1060001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:292: spanningRowsHeight.rowWithOnlySpanningCells = rowHasOnlySpanningCells(actualRow); On 2014/02/26 22:29:31, Julien Chaffraix - PST wrote: > As mentioned previously, this change should be removed as it doesn't add > anything. > > The fact that you *had* to explain what it is supposed to mean (and what you > said just mirrored the code) underlines that this variable is badly named. > > Note that booleans should have a verb, which is not the case anyway so we could > kill 2 birds with one stone. change variable name |rowWithOnlySpanningCells| to |isAnyRowWithOnlySpanningCells| which explain every thing here. https://codereview.chromium.org/47923009/diff/1060001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:557: // So here, we are giving text'height of spanning cell to the last row of spanning cell. On 2014/02/26 22:29:31, Julien Chaffraix - PST wrote: > First and this is a recurring issue, I have a hard time understanding what you > mean and that's only because I can't parse your English. My advice would be to > stick to very simple sentences, splitting complex ones into more simple. Also be > mindful of the conjugation and word used. > > // This code handle row(s) that have rowspanning cell(s) and at least one empty > cell. > // Such rows are not handled below and end up having a height of 0. That would > mean > // content overlapping if one of their cells has any content. To avoid the > problem, we > // add all the remaining spanning cells' height to the last spanned row. > // This means that we could grow a row past its 'height' or break percentage > spreading > // however this is better than overlapping content. FIXME: Is there a better > algorithm? Done.
Message was sent while issue was closed.
Change committed as 168326 |
