Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1023133002:
Negative row height when cell has percentage height. (Closed)
|
Created:
5 years, 9 months ago by a.suchit Modified:
5 years, 4 months ago Reviewers:
a.suchit2, leviw_travelin_and_unemployed, esprehn, eseidel, mstensho (USE GERRIT), suchit.agrawal, Julien - ping for review CC:
blink-reviews, blink-reviews-rendering, Dominik Röttsches, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionNegative row height when cell has percentage height.
While distributing the extra height in percent rows, row's height
is reducing from the height required for the content to fit in that
row.
Minimum row's height should based on the content in that row,
irrespective of user defined height.
R=jchaffraix@chromium.org
BUG=446043
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200053
Patch Set 1 #Patch Set 2 : Added Layout test #
Total comments: 9
Patch Set 3 : Review comments addressed #Patch Set 4 : Updated test expectation #Patch Set 5 : Test expected changed #Patch Set 6 : Test expectation updated #Patch Set 7 : Test updated #Patch Set 8 : Test expected updated. #Patch Set 9 : Test expected file updated #
Total comments: 8
Patch Set 10 : Review comments addressed #Patch Set 11 : Test expectation updated #Patch Set 12 : Layout test expectation updated #Patch Set 13 : Layout test expectation updated #
Total comments: 1
Patch Set 14 : Rebase #Patch Set 15 : Rebase #Patch Set 16 : Rebase #
Messages
Total messages: 39 (13 generated)
suchit.agrawal@gmail.com changed reviewers: + suchit.agrawal@gmail.com
On 2015/03/20 10:40:18, suchit.agrawal wrote: Hi Julien, Please review it. Thanks
a.suchit@samsung.com changed reviewers: + dsinclair@chromium.org, eseidel@chromium.org, esprehn@chromium.org
https://codereview.chromium.org/1023133002/diff/20001/Source/core/layout/Layo... File Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1023133002/diff/20001/Source/core/layout/Layo... Source/core/layout/LayoutTableSection.cpp:316: toAdd = std::max(toAdd, 0); How does toAdd become negative here?
a.suchit@chromium.org changed reviewers: + a.suchit@chromium.org
https://codereview.chromium.org/1023133002/diff/20001/Source/core/layout/Layo... File Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1023133002/diff/20001/Source/core/layout/Layo... Source/core/layout/LayoutTableSection.cpp:316: toAdd = std::max(toAdd, 0); On 2015/03/26 08:23:11, esprehn wrote: > How does toAdd become negative here? If percent height is less than the rowHeight based on the content in the cells of the row then percent height should be ignored. just consider : rowsHeight[row - rowIndex] -> 10 px => this is the row height based on content on the cells of this row. Total table height is 20 px then 1% (percent height for the row) of 20 px is 2. toAdd become 2 - 20 = -18
https://codereview.chromium.org/1023133002/diff/20001/LayoutTests/fast/table/... File LayoutTests/fast/table/table-rowspan-row-height-less-than-content-height.html (right): https://codereview.chromium.org/1023133002/diff/20001/LayoutTests/fast/table/... LayoutTests/fast/table/table-rowspan-row-height-less-than-content-height.html:8: <h3>Test for chromium bug : <a href="https://code.google.com/p/chromium/issues/detail?id=406043">406043</a>. Text Rendering is not correct on page.</h3> The name of the bug as well as this CL could be improved. It's very very very vague as written. Better naming: Negative row height when cell has percentage height https://codereview.chromium.org/1023133002/diff/20001/LayoutTests/fast/table/... LayoutTests/fast/table/table-rowspan-row-height-less-than-content-height.html:14: <tr data-offset-y="424"> Can we use meaningful numbers here? It's difficult to know if the test is passing or failing with that kind of numbers. Some ideas: * set font to use an easy to remember font-size and line-height. * break your content to have a fixed number of lines (ie it's easy to see how tall the rows should be). https://codereview.chromium.org/1023133002/diff/20001/Source/core/layout/Layo... File Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1023133002/diff/20001/Source/core/layout/Layo... Source/core/layout/LayoutTableSection.cpp:316: toAdd = std::max(toAdd, 0); On 2015/03/26 at 09:38:35, a.suchit2 wrote: > On 2015/03/26 08:23:11, esprehn wrote: > > How does toAdd become negative here? > > If percent height is less than the rowHeight based on the content in the cells of the row then percent height should be ignored. > > just consider : > rowsHeight[row - rowIndex] -> 10 px => this is the row height based on content on the cells of this row. > > Total table height is 20 px > > then 1% (percent height for the row) of 20 px is 2. > > toAdd become 2 - 20 = -18 It's not the only place where we do something similar and it's reasonable to do so here IMO.
leviw@chromium.org changed reviewers: + leviw@chromium.org
Please give a better description of the issue.
On 2015/03/26 at 17:50:49, leviw wrote: > Please give a better description of the issue. Specifically, the title "Text Rendering is not correct on page"
Any progress here? Looks like the test just needs updating. It'd be awesome to fix this compat bug. https://codereview.chromium.org/1023133002/diff/20001/LayoutTests/fast/table/... File LayoutTests/fast/table/table-rowspan-row-height-less-than-content-height.html (right): https://codereview.chromium.org/1023133002/diff/20001/LayoutTests/fast/table/... LayoutTests/fast/table/table-rowspan-row-height-less-than-content-height.html:4: #perc-height { height : 1% } #percent-height, don't abbreviate.
https://codereview.chromium.org/1023133002/diff/20001/LayoutTests/fast/table/... File LayoutTests/fast/table/table-rowspan-row-height-less-than-content-height.html (right): https://codereview.chromium.org/1023133002/diff/20001/LayoutTests/fast/table/... LayoutTests/fast/table/table-rowspan-row-height-less-than-content-height.html:4: #perc-height { height : 1% } On 2015/06/12 06:21:33, esprehn wrote: > #percent-height, don't abbreviate. Done. https://codereview.chromium.org/1023133002/diff/20001/LayoutTests/fast/table/... LayoutTests/fast/table/table-rowspan-row-height-less-than-content-height.html:8: <h3>Test for chromium bug : <a href="https://code.google.com/p/chromium/issues/detail?id=406043">406043</a>. Text Rendering is not correct on page.</h3> On 2015/03/26 15:22:10, Julien Chaffraix - PST wrote: > The name of the bug as well as this CL could be improved. It's very very very > vague as written. > > Better naming: Negative row height when cell has percentage height Done. https://codereview.chromium.org/1023133002/diff/20001/LayoutTests/fast/table/... LayoutTests/fast/table/table-rowspan-row-height-less-than-content-height.html:14: <tr data-offset-y="424"> On 2015/03/26 15:22:10, Julien Chaffraix - PST wrote: > Can we use meaningful numbers here? It's difficult to know if the test is > passing or failing with that kind of numbers. > > Some ideas: > * set font to use an easy to remember font-size and line-height. > * break your content to have a fixed number of lines (ie it's easy to see how > tall the rows should be). Done.
On 2015/06/12 12:40:20, a.suchit wrote: > https://codereview.chromium.org/1023133002/diff/20001/LayoutTests/fast/table/... > File > LayoutTests/fast/table/table-rowspan-row-height-less-than-content-height.html > (right): > > https://codereview.chromium.org/1023133002/diff/20001/LayoutTests/fast/table/... > LayoutTests/fast/table/table-rowspan-row-height-less-than-content-height.html:4: > #perc-height { height : 1% } > On 2015/06/12 06:21:33, esprehn wrote: > > #percent-height, don't abbreviate. > > Done. > > https://codereview.chromium.org/1023133002/diff/20001/LayoutTests/fast/table/... > LayoutTests/fast/table/table-rowspan-row-height-less-than-content-height.html:8: > <h3>Test for chromium bug : <a > href="https://code.google.com/p/chromium/issues/detail?id=406043">406043</a>. > Text Rendering is not correct on page.</h3> > On 2015/03/26 15:22:10, Julien Chaffraix - PST wrote: > > The name of the bug as well as this CL could be improved. It's very very very > > vague as written. > > > > Better naming: Negative row height when cell has percentage height > > Done. > > https://codereview.chromium.org/1023133002/diff/20001/LayoutTests/fast/table/... > LayoutTests/fast/table/table-rowspan-row-height-less-than-content-height.html:14: > <tr data-offset-y="424"> > On 2015/03/26 15:22:10, Julien Chaffraix - PST wrote: > > Can we use meaningful numbers here? It's difficult to know if the test is > > passing or failing with that kind of numbers. > > > > Some ideas: > > * set font to use an easy to remember font-size and line-height. > > * break your content to have a fixed number of lines (ie it's easy to see how > > tall the rows should be). > > Done. I have some system issues so expected file created manually. It may fail in Trybots.
Please review it. Thanks
https://codereview.chromium.org/1023133002/diff/160001/LayoutTests/TestExpect... File LayoutTests/TestExpectations (left): https://codereview.chromium.org/1023133002/diff/160001/LayoutTests/TestExpect... LayoutTests/TestExpectations:1636: Unneeded line removal. https://codereview.chromium.org/1023133002/diff/160001/LayoutTests/fast/table... File LayoutTests/fast/table/table-rowspan-row-height-less-than-content-height.html (right): https://codereview.chromium.org/1023133002/diff/160001/LayoutTests/fast/table... LayoutTests/fast/table/table-rowspan-row-height-less-than-content-height.html:10: <table border="1"> Do we need the table border, the cell padding and cell spacing? (the reason is that it makes the numbers very difficult to read) https://codereview.chromium.org/1023133002/diff/160001/LayoutTests/fast/table... LayoutTests/fast/table/table-rowspan-row-height-less-than-content-height.html:12: <td rowspan="4"> This is rowspan cell. It would spanned for 3 rows in table. This is rowspan cell. It would spanned for 3 rows in table.</td> I think this is better English: "It would span 3 rows in the table" Also why is this repeated twice? https://codereview.chromium.org/1023133002/diff/160001/Source/core/layout/Lay... File Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1023133002/diff/160001/Source/core/layout/Lay... Source/core/layout/LayoutTableSection.cpp:326: toAdd = std::min(toAdd, extraRowSpanningHeight); I think a single line would be better as it would show the relationship we want: toAdd = std::max(std::min(toAdd, extraRowSpanningHeight), 0); Also I would do it *after* applying the min as it would ensure that a potentially negative extraRowSpanHeight won't make toAdd negative.
https://codereview.chromium.org/1023133002/diff/160001/LayoutTests/TestExpect... File LayoutTests/TestExpectations (left): https://codereview.chromium.org/1023133002/diff/160001/LayoutTests/TestExpect... LayoutTests/TestExpectations:1636: On 2015/07/14 17:02:41, Julien Chaffraix - PST wrote: > Unneeded line removal. Done. https://codereview.chromium.org/1023133002/diff/160001/LayoutTests/fast/table... File LayoutTests/fast/table/table-rowspan-row-height-less-than-content-height.html (right): https://codereview.chromium.org/1023133002/diff/160001/LayoutTests/fast/table... LayoutTests/fast/table/table-rowspan-row-height-less-than-content-height.html:10: <table border="1"> On 2015/07/14 17:02:41, Julien Chaffraix - PST wrote: > Do we need the table border, the cell padding and cell spacing? (the reason is > that it makes the numbers very difficult to read) We do not need it. Removed. https://codereview.chromium.org/1023133002/diff/160001/LayoutTests/fast/table... LayoutTests/fast/table/table-rowspan-row-height-less-than-content-height.html:12: <td rowspan="4"> This is rowspan cell. It would spanned for 3 rows in table. This is rowspan cell. It would spanned for 3 rows in table.</td> On 2015/07/14 17:02:41, Julien Chaffraix - PST wrote: > I think this is better English: "It would span 3 rows in the table" > > Also why is this repeated twice? done. rowspan cell height should be more so it would go for distribution and 1% cells height would be generated negative row height. Now changed and given explicit height for rowspan cell. https://codereview.chromium.org/1023133002/diff/160001/Source/core/layout/Lay... File Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1023133002/diff/160001/Source/core/layout/Lay... Source/core/layout/LayoutTableSection.cpp:326: toAdd = std::min(toAdd, extraRowSpanningHeight); On 2015/07/14 17:02:41, Julien Chaffraix - PST wrote: > I think a single line would be better as it would show the relationship we want: > > toAdd = std::max(std::min(toAdd, extraRowSpanningHeight), 0); > > Also I would do it *after* applying the min as it would ensure that a > potentially negative extraRowSpanHeight won't make toAdd negative. Done.
Please review. Thanks https://codereview.chromium.org/1023133002/diff/230001/LayoutTests/fast/table... File LayoutTests/fast/table/table-rowspan-row-height-less-than-content-height.html (right): https://codereview.chromium.org/1023133002/diff/230001/LayoutTests/fast/table... LayoutTests/fast/table/table-rowspan-row-height-less-than-content-height.html:21: <tr data-offset-y="119"> I have a confusion here that how is it 119 because table height should be 100px (+- ~2-5px). It renders correct in content shell but while generating the expected files is different. +-------+-----------------------+ | | | | | | | +-----------------------+ | | 21 px | | 100px +-----------------------+ | | 21 px | | +-----------------------+ | | 21 px | +-------+-----------------------+ Please let me know, If I am doing any mistake. Thanks
On 2015/07/20 12:42:39, a.suchit wrote: > Please review. Thanks > > https://codereview.chromium.org/1023133002/diff/230001/LayoutTests/fast/table... > File > LayoutTests/fast/table/table-rowspan-row-height-less-than-content-height.html > (right): > > https://codereview.chromium.org/1023133002/diff/230001/LayoutTests/fast/table... > LayoutTests/fast/table/table-rowspan-row-height-less-than-content-height.html:21: > <tr data-offset-y="119"> > I have a confusion here that how is it 119 because table height should be 100px > (+- ~2-5px). It renders correct in content shell but while generating the > expected files is different. > > +-------+-----------------------+ > | | | > | | | > | +-----------------------+ > | | 21 px | > | 100px +-----------------------+ > | | 21 px | > | +-----------------------+ > | | 21 px | > +-------+-----------------------+ > > Please let me know, If I am doing any mistake. Thanks Please review it. Thanks
The CQ bit was checked by a.suchit@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1023133002/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1023133002/250001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by a.suchit@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1023133002/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1023133002/250001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blink_presubmit on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...) linux_blink_compile_dbg on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_compile_dbg/...) linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7...) linux_chromium_gn_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
The CQ bit was checked by a.suchit@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1023133002/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1023133002/270001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/07/29 14:15:27, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. @Julien, Please review it. All the review comments addressed.
jchaffraix@chromium.org changed reviewers: + mstensho@opera.com
lgtm, sorry for the long delay. In the future, feel free to ping me on IM or ask mstensho to review the change.
The CQ bit was checked by jchaffraix@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1023133002/290001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1023133002/290001
dsinclair@chromium.org changed reviewers: - dsinclair@chromium.org
Message was sent while issue was closed.
Committed patchset #16 (id:290001) as https://src.chromium.org/viewvc/blink?view=rev&revision=200053 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
