Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1213153003:
Cope with intrinsic height styled on table cells (Closed)
|
Created:
5 years, 5 months ago by rhogan Modified:
5 years, 5 months ago Reviewers:
mstensho (USE GERRIT) CC:
blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionCope with intrinsic height styled on table cells
Treat it just like auto as intrinsic values don't really apply to cells.
BUG=478279
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198890
Patch Set 1 #
Total comments: 4
Patch Set 2 : Updated #Patch Set 3 : Updated #
Total comments: 3
Patch Set 4 : Updated #
Total comments: 3
Patch Set 5 : Updated #
Dependent Patchsets: Messages
Total messages: 17 (2 generated)
robhogan@gmail.com changed reviewers: + mstensho@opera.com
Referring to http://dev.w3.org/csswg/css-device-adapt in the description doesn't seem right. CSS device adaptation has nothing to do with this. https://codereview.chromium.org/1213153003/diff/1/LayoutTests/fast/table/cell... File LayoutTests/fast/table/cell-intrinsic-height.html (right): https://codereview.chromium.org/1213153003/diff/1/LayoutTests/fast/table/cell... LayoutTests/fast/table/cell-intrinsic-height.html:4: <td style='height: -webkit-fit-content;' data-expected-height=22><div style="height: 20px; width: 20px"></div></td> This doesn't seem to be whitespace sensitive, so why not move the DIV to a separate line? https://codereview.chromium.org/1213153003/diff/1/LayoutTests/fast/table/cell... LayoutTests/fast/table/cell-intrinsic-height.html:5: <table> </table> https://codereview.chromium.org/1213153003/diff/1/Source/core/layout/LayoutTa... File Source/core/layout/LayoutTableCell.h (right): https://codereview.chromium.org/1213153003/diff/1/Source/core/layout/LayoutTa... Source/core/layout/LayoutTableCell.h:112: int styleLogicalHeight = computeLogicalHeightFromCellStyle(style()->logicalHeight()); Any reason why we shouldn't just treat intrinsic values as auto? I know intrinsic and auto isn't the same, but I don't see any point in treating them differently inside tables. There's no table spec that defines how to treat intrinsic height values, as far as I know. Tables and fancy height property values are two technologies that never met, very much like 5.25" floppy drives and USB ports.
On 2015/07/07 at 07:11:40, mstensho wrote: > Referring to http://dev.w3.org/csswg/css-device-adapt in the description doesn't seem right. CSS device adaptation has nothing to do with this. That's true but it is the home of some of the newer and fancier height/width values that appear in valueForLength() and which we don't have to worry about asserting as they do not apply to tables/cells AFAICT.
https://codereview.chromium.org/1213153003/diff/1/Source/core/layout/LayoutTa... File Source/core/layout/LayoutTableCell.h (right): https://codereview.chromium.org/1213153003/diff/1/Source/core/layout/LayoutTa... Source/core/layout/LayoutTableCell.h:112: int styleLogicalHeight = computeLogicalHeightFromCellStyle(style()->logicalHeight()); On 2015/07/07 at 07:11:39, mstensho wrote: > Any reason why we shouldn't just treat intrinsic values as auto? I know intrinsic and auto isn't the same, but I don't see any point in treating them differently inside tables. There's no table spec that defines how to treat intrinsic height values, as far as I know. Tables and fancy height property values are two technologies that never met, very much like 5.25" floppy drives and USB ports. Sure, but we treat them this way when they're set on the table itself, see LayoutTable::convertStyleLogicalHeightToComputedHeight() and co. So at least we're being internally consistent this way. :)
On 2015/07/07 18:40:02, rhogan wrote: > https://codereview.chromium.org/1213153003/diff/1/Source/core/layout/LayoutTa... > File Source/core/layout/LayoutTableCell.h (right): > > https://codereview.chromium.org/1213153003/diff/1/Source/core/layout/LayoutTa... > Source/core/layout/LayoutTableCell.h:112: int styleLogicalHeight = > computeLogicalHeightFromCellStyle(style()->logicalHeight()); > On 2015/07/07 at 07:11:39, mstensho wrote: > > Any reason why we shouldn't just treat intrinsic values as auto? I know > intrinsic and auto isn't the same, but I don't see any point in treating them > differently inside tables. There's no table spec that defines how to treat > intrinsic height values, as far as I know. Tables and fancy height property > values are two technologies that never met, very much like 5.25" floppy drives > and USB ports. > > Sure, but we treat them this way when they're set on the table itself, see > LayoutTable::convertStyleLogicalHeightToComputedHeight() and co. So at least > we're being internally consistent this way. :) Tables honor the min-height and max-height properties, while cells don't, as far as I can tell. So for tables it's somewhat useful to support this. But do you have a test case where it does anything useful for table cells?
On 2015/07/07 at 19:15:42, mstensho wrote: > On 2015/07/07 18:40:02, rhogan wrote: > > https://codereview.chromium.org/1213153003/diff/1/Source/core/layout/LayoutTa... > > File Source/core/layout/LayoutTableCell.h (right): > > > > https://codereview.chromium.org/1213153003/diff/1/Source/core/layout/LayoutTa... > > Source/core/layout/LayoutTableCell.h:112: int styleLogicalHeight = > > computeLogicalHeightFromCellStyle(style()->logicalHeight()); > > On 2015/07/07 at 07:11:39, mstensho wrote: > > > Any reason why we shouldn't just treat intrinsic values as auto? I know > > intrinsic and auto isn't the same, but I don't see any point in treating them > > differently inside tables. There's no table spec that defines how to treat > > intrinsic height values, as far as I know. Tables and fancy height property > > values are two technologies that never met, very much like 5.25" floppy drives > > and USB ports. > > > > Sure, but we treat them this way when they're set on the table itself, see > > LayoutTable::convertStyleLogicalHeightToComputedHeight() and co. So at least > > we're being internally consistent this way. :) > > Tables honor the min-height and max-height properties, while cells don't, as far as I can tell. So for tables it's somewhat useful to support this. But do you have a test case where it does anything useful for table cells? I do not. :p I'll treat them as auto so!
https://codereview.chromium.org/1213153003/diff/40001/Source/core/layout/Layo... File Source/core/layout/LayoutTableCell.h (left): https://codereview.chromium.org/1213153003/diff/40001/Source/core/layout/Layo... Source/core/layout/LayoutTableCell.h:105: int styleLogicalHeight = valueForLength(style()->logicalHeight(), 0); Note that "LayoutUnit()" is cheaper than "0", when assigning it to a LayoutUnit. Something to keep in mind if you make the changes I requested above. :) https://codereview.chromium.org/1213153003/diff/40001/Source/core/layout/Layo... File Source/core/layout/LayoutTableCell.h (right): https://codereview.chromium.org/1213153003/diff/40001/Source/core/layout/Layo... Source/core/layout/LayoutTableCell.h:103: int computeLogicalHeightFromCellStyle(const Length& height) const I don't like the name. It essentially means the same thing as the name of the caller (logicalHeightFromStyle()). Then again, I don't really see the need for a separate method here, so why not stuff it back into logicalHeightFomStyle()? https://codereview.chromium.org/1213153003/diff/40001/Source/core/layout/Layo... Source/core/layout/LayoutTableCell.h:105: int maximumHeight = 0; This one is always 0, so I don't see any need for it.
On 2015/07/09 at 08:22:16, mstensho wrote: > https://codereview.chromium.org/1213153003/diff/40001/Source/core/layout/Layo... > File Source/core/layout/LayoutTableCell.h (left): > > https://codereview.chromium.org/1213153003/diff/40001/Source/core/layout/Layo... > Source/core/layout/LayoutTableCell.h:105: int styleLogicalHeight = valueForLength(style()->logicalHeight(), 0); > Note that "LayoutUnit()" is cheaper than "0", when assigning it to a LayoutUnit. Something to keep in mind if you make the changes I requested above. :) > > https://codereview.chromium.org/1213153003/diff/40001/Source/core/layout/Layo... > File Source/core/layout/LayoutTableCell.h (right): > > https://codereview.chromium.org/1213153003/diff/40001/Source/core/layout/Layo... > Source/core/layout/LayoutTableCell.h:103: int computeLogicalHeightFromCellStyle(const Length& height) const > I don't like the name. It essentially means the same thing as the name of the caller (logicalHeightFromStyle()). Then again, I don't really see the need for a separate method here, so why not stuff it back into logicalHeightFomStyle()? > > https://codereview.chromium.org/1213153003/diff/40001/Source/core/layout/Layo... > Source/core/layout/LayoutTableCell.h:105: int maximumHeight = 0; > This one is always 0, so I don't see any need for it. Updated CL for your review.. :)
https://codereview.chromium.org/1213153003/diff/60001/Source/core/layout/Layo... File Source/core/layout/LayoutTableCell.h (right): https://codereview.chromium.org/1213153003/diff/60001/Source/core/layout/Layo... Source/core/layout/LayoutTableCell.h:103: LayoutUnit logicalHeightFromStyle() const Why change the return type here? The table code seems to be in love with ints, so I see no reason not to be consistent. Did you do this because of my remark about using "LayoutUnit()" instead of "0"? Or was it to avoid an explicit "toInt()" on the result from valueForLength(), to give the last two ternary operands the same type?
https://codereview.chromium.org/1213153003/diff/60001/Source/core/layout/Layo... File Source/core/layout/LayoutTableCell.h (right): https://codereview.chromium.org/1213153003/diff/60001/Source/core/layout/Layo... Source/core/layout/LayoutTableCell.h:103: LayoutUnit logicalHeightFromStyle() const On 2015/07/12 at 19:38:55, mstensho wrote: > Why change the return type here? The table code seems to be in love with ints, so I see no reason not to be consistent. Did you do this because of my remark about using "LayoutUnit()" instead of "0"? Or was it to avoid an explicit "toInt()" on the result from valueForLength(), to give the last two ternary operands the same type? One of the two callers, LayoutTableCell::cellBaselinePosition, uses it as a LayoutUnit so my intention is to save the unnecessary cast in that case, and defer it in the other. I'm assuming that max(logicalHeightFromStyle() - computedCSSPaddingAfter() - borderAfter(), borderBefore() + paddingBefore()) knows the two callers are LayoutUnits but maybe they need to be max<LayoutUnit>()?
https://codereview.chromium.org/1213153003/diff/60001/Source/core/layout/Layo... File Source/core/layout/LayoutTableCell.h (right): https://codereview.chromium.org/1213153003/diff/60001/Source/core/layout/Layo... Source/core/layout/LayoutTableCell.h:103: LayoutUnit logicalHeightFromStyle() const On 2015/07/13 18:28:21, rhogan wrote: > On 2015/07/12 at 19:38:55, mstensho wrote: > > Why change the return type here? The table code seems to be in love with ints, > so I see no reason not to be consistent. Did you do this because of my remark > about using "LayoutUnit()" instead of "0"? Or was it to avoid an explicit > "toInt()" on the result from valueForLength(), to give the last two ternary > operands the same type? > > One of the two callers, LayoutTableCell::cellBaselinePosition, uses it as a > LayoutUnit so my intention is to save the unnecessary cast in that case, and > defer it in the other. > > I'm assuming that max(logicalHeightFromStyle() - computedCSSPaddingAfter() - > borderAfter(), borderBefore() + paddingBefore()) knows the two callers are > LayoutUnits but maybe they need to be max<LayoutUnit>()? I don't know how that type soup is resolved, but how about changing it back to int for now, since changing to LayoutUnit isn't relevant for your fix, and since most other parts of the table code use ints?
On 2015/07/14 at 08:45:29, mstensho wrote: > https://codereview.chromium.org/1213153003/diff/60001/Source/core/layout/Layo... > File Source/core/layout/LayoutTableCell.h (right): > > https://codereview.chromium.org/1213153003/diff/60001/Source/core/layout/Layo... > Source/core/layout/LayoutTableCell.h:103: LayoutUnit logicalHeightFromStyle() const > On 2015/07/13 18:28:21, rhogan wrote: > > On 2015/07/12 at 19:38:55, mstensho wrote: > > > Why change the return type here? The table code seems to be in love with ints, > > so I see no reason not to be consistent. Did you do this because of my remark > > about using "LayoutUnit()" instead of "0"? Or was it to avoid an explicit > > "toInt()" on the result from valueForLength(), to give the last two ternary > > operands the same type? > > > > One of the two callers, LayoutTableCell::cellBaselinePosition, uses it as a > > LayoutUnit so my intention is to save the unnecessary cast in that case, and > > defer it in the other. > > > > I'm assuming that max(logicalHeightFromStyle() - computedCSSPaddingAfter() - > > borderAfter(), borderBefore() + paddingBefore()) knows the two callers are > > LayoutUnits but maybe they need to be max<LayoutUnit>()? > > I don't know how that type soup is resolved, but how about changing it back to int for now, since changing to LayoutUnit isn't relevant for your fix, and since most other parts of the table code use ints? OK, done.
The CQ bit was checked by mstensho@opera.com
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1213153003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=198890 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
