Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(375)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 13674002: Support intrinsic values for height, min-height and max-height (Closed)

Created:
7 years, 8 months ago by cbiesinger
Modified:
7 years, 8 months ago
Reviewers:
ojan
CC:
blink-reviews, Julien - ping for review
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Support intrinsic values for height, min-height and max-height As specified in http://dev.w3.org/csswg/css-sizing/ The changes here fall in three categories: - Change RenderBox to pass the content height through to computeLogicalHeight and related functions, which needs it to resolve max-content, et. al. - Make the callers pass the content height to this function. Some callers pass logicalHeight() (adjusted for border/padding) - this works because if logicalHeight is not the content height, then it is the height we ended up using, so the constrain* functions will just constrain to that value again - Changes to CSS parsing to accept the values for heights, not just widths. BUG=226251 R=ojan@chromium.org Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=148314

Patch Set 1 #

Patch Set 2 : better fixme comment #

Total comments: 29

Patch Set 3 : Made all changes except for the table test, will do that tomorrow #

Patch Set 4 : Another test, another bug found #

Patch Set 5 : CSS table tests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1382 lines, -111 lines) Patch
A LayoutTests/fast/css-intrinsic-dimensions/height.html View 1 2 1 chunk +92 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-intrinsic-dimensions/height-css-tables.html View 1 2 3 4 1 chunk +40 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-intrinsic-dimensions/height-css-tables-collapsed.html View 1 2 3 4 1 chunk +40 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-intrinsic-dimensions/height-css-tables-collapsed-expected.html View 1 2 3 4 1 chunk +34 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-intrinsic-dimensions/height-css-tables-expected.html View 1 2 3 4 1 chunk +36 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-intrinsic-dimensions/height-expected.html View 1 2 1 chunk +80 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-intrinsic-dimensions/height-flexbox.html View 1 2 1 chunk +37 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-intrinsic-dimensions/height-flexbox-expected.html View 1 2 1 chunk +27 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-intrinsic-dimensions/height-positioned.html View 1 2 3 1 chunk +129 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-intrinsic-dimensions/height-positioned-expected.html View 1 2 3 1 chunk +120 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-intrinsic-dimensions/height-positioned-replaced.html View 1 2 3 1 chunk +119 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-intrinsic-dimensions/height-positioned-replaced-expected.html View 1 2 3 1 chunk +111 lines, -0 lines 0 comments Download
M LayoutTests/fast/css-intrinsic-dimensions/height-property-value.html View 2 chunks +13 lines, -13 lines 0 comments Download
M LayoutTests/fast/css-intrinsic-dimensions/height-property-value-expected.txt View 1 chunk +13 lines, -28 lines 0 comments Download
A LayoutTests/fast/css-intrinsic-dimensions/height-replaced.html View 1 2 1 chunk +84 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-intrinsic-dimensions/height-replaced-expected.html View 1 2 1 chunk +85 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-intrinsic-dimensions/height-tables.html View 1 2 3 1 chunk +40 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-intrinsic-dimensions/height-tables-collapsed.html View 1 2 3 4 1 chunk +40 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-intrinsic-dimensions/height-tables-collapsed-expected.html View 1 2 3 4 1 chunk +36 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-intrinsic-dimensions/height-tables-expected.html View 1 2 3 1 chunk +36 lines, -0 lines 0 comments Download
A LayoutTests/fast/css-intrinsic-dimensions/resources/height-keyword-classes.css View 1 2 1 chunk +77 lines, -0 lines 0 comments Download
M Source/WebCore/css/CSSParser.h View 1 2 3 1 chunk +1 line, -2 lines 0 comments Download
M Source/WebCore/css/CSSParser.cpp View 1 2 3 3 chunks +8 lines, -26 lines 0 comments Download
M Source/WebCore/css/StyleBuilder.cpp View 1 2 2 chunks +3 lines, -3 lines 0 comments Download
M Source/WebCore/platform/Length.h View 1 chunk +2 lines, -0 lines 0 comments Download
M Source/WebCore/rendering/RenderBox.h View 1 2 3 3 chunks +6 lines, -5 lines 0 comments Download
M Source/WebCore/rendering/RenderBox.cpp View 1 2 3 13 chunks +49 lines, -21 lines 0 comments Download
M Source/WebCore/rendering/RenderFlexibleBox.cpp View 2 chunks +10 lines, -6 lines 0 comments Download
M Source/WebCore/rendering/RenderGrid.cpp View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M Source/WebCore/rendering/RenderReplaced.cpp View 1 2 1 chunk +3 lines, -0 lines 0 comments Download
M Source/WebCore/rendering/RenderTable.cpp View 1 2 3 2 chunks +9 lines, -6 lines 0 comments Download

Messages

Total messages: 16 (0 generated)
cbiesinger
I think this is ready for review. Ojan, can you submit this to the try ...
7 years, 8 months ago (2013-04-04 23:32:52 UTC) #1
cbiesinger
Ah, two more things: - I ran all the tests locally and they pass - ...
7 years, 8 months ago (2013-04-04 23:50:39 UTC) #2
cbiesinger
cc'ing jchaffraix in case he has thoughts on this RenderGrid change
7 years, 8 months ago (2013-04-09 18:20:00 UTC) #3
ojan
The code mostly looks good. Can you add a table test as well? See css-intrinsic-dimensions/tables.html ...
7 years, 8 months ago (2013-04-11 03:21:33 UTC) #4
ojan
https://codereview.chromium.org/13674002/diff/3001/LayoutTests/fast/css-intrinsic-dimensions/height-expected.html File LayoutTests/fast/css-intrinsic-dimensions/height-expected.html (right): https://codereview.chromium.org/13674002/diff/3001/LayoutTests/fast/css-intrinsic-dimensions/height-expected.html#newcode1 LayoutTests/fast/css-intrinsic-dimensions/height-expected.html:1: <!doctype HTML> Nit: We typically do <!DOCTYPE html> since ...
7 years, 8 months ago (2013-04-11 03:21:46 UTC) #5
cbiesinger
Made all changes except for the table test, which will have to wait until tomorrow. ...
7 years, 8 months ago (2013-04-12 01:17:52 UTC) #6
ojan
lgtm Feel free to add the table tests to this patch or to a followup ...
7 years, 8 months ago (2013-04-12 04:03:10 UTC) #7
cbiesinger
Added a table test, and correspondingly, I also added a fix to RenderTable.cpp. https://codereview.chromium.org/13674002/diff/3001/Source/WebCore/css/CSSParser.h File ...
7 years, 8 months ago (2013-04-12 20:30:52 UTC) #8
cbiesinger
(I'll wait with submitting until you had a change to look at the RenderTable diff)
7 years, 8 months ago (2013-04-12 20:31:31 UTC) #9
ojan
lgtm For completeness sake, can you also add test cases with CSS tables and tables ...
7 years, 8 months ago (2013-04-12 21:21:57 UTC) #10
cbiesinger
On 2013/04/12 21:21:57, ojan wrote: > lgtm > > For completeness sake, can you also ...
7 years, 8 months ago (2013-04-12 21:29:05 UTC) #11
ojan
On 2013/04/12 21:29:05, cbiesinger wrote: > On 2013/04/12 21:21:57, ojan wrote: > > lgtm > ...
7 years, 8 months ago (2013-04-12 21:41:51 UTC) #12
cbiesinger
On 2013/04/12 21:41:51, ojan wrote: > On 2013/04/12 21:29:05, cbiesinger wrote: > > On 2013/04/12 ...
7 years, 8 months ago (2013-04-12 22:31:52 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cbiesinger@chromium.org/13674002/38001
7 years, 8 months ago (2013-04-12 22:45:50 UTC) #14
commit-bot: I haz the power
Presubmit check for 13674002-38001 failed and returned exit status 1. INFO:root:Found 31 file(s). Running presubmit ...
7 years, 8 months ago (2013-04-12 22:46:08 UTC) #15
cbiesinger
7 years, 8 months ago (2013-04-12 23:09:04 UTC) #16
Message was sent while issue was closed. 
Committed patchset #5 manually as r148314 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698