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

Issue 1268753002: [css-flexbox] Implementing new CSS Box Alignment values (Closed)

Created:
5 years, 4 months ago by jfernandez
Modified:
3 years, 9 months ago
Reviewers:
cbiesinger
CC:
blink-reviews, cbiesinger, szager+layoutwatch_chromium.org, zoltan1, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, blink-reviews-rendering, jchaffraix+rendering
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[css-flexbox] Implementing new CSS Box Alignment values The CSS Box Alignment specification is intended to put in common the alignment logic, initially implemented by Flexbox, with any other layout model. As such, it adds new properties and values for the ones already defined, which should be implemented by Flexbox. BUG=507690

Patch Set 1 #

Total comments: 7

Patch Set 2 : Applied suggested changes. #

Total comments: 1

Patch Set 3 : Applied additional changes and added new test cases. #

Patch Set 4 : Patch rebased and applied suggested changes. #

Total comments: 5

Messages

Total messages: 9 (1 generated)
jfernandez
This is still a WIP patch, in a very early stage, but I'd like to ...
5 years, 4 months ago (2015-07-31 15:50:33 UTC) #2
ikilpatrick
Drive-by spelling review :) https://codereview.chromium.org/1268753002/diff/1/LayoutTests/css3/flexbox/flex-align-new-values.html File LayoutTests/css3/flexbox/flex-align-new-values.html (right): https://codereview.chromium.org/1268753002/diff/1/LayoutTests/css3/flexbox/flex-align-new-values.html#newcode382 LayoutTests/css3/flexbox/flex-align-new-values.html:382: <!-- This is equivalent to ...
5 years, 4 months ago (2015-08-02 23:15:37 UTC) #3
cbiesinger
https://codereview.chromium.org/1268753002/diff/1/Source/core/layout/LayoutFlexibleBox.cpp File Source/core/layout/LayoutFlexibleBox.cpp (right): https://codereview.chromium.org/1268753002/diff/1/Source/core/layout/LayoutFlexibleBox.cpp#newcode1351 Source/core/layout/LayoutFlexibleBox.cpp:1351: case ItemPositionLastBaseline: // TODO (lajava): This feature is at-risk ...
5 years, 4 months ago (2015-08-10 20:45:46 UTC) #4
jfernandez
Thanks for the review !!! I replied your questions; I'll upload a new patch with ...
5 years, 4 months ago (2015-08-11 13:52:59 UTC) #5
jfernandez
Added a new patch with the suggested changes.
5 years, 4 months ago (2015-08-11 14:27:52 UTC) #6
cbiesinger
https://codereview.chromium.org/1268753002/diff/20001/Source/core/layout/LayoutFlexibleBox.cpp File Source/core/layout/LayoutFlexibleBox.cpp (right): https://codereview.chromium.org/1268753002/diff/20001/Source/core/layout/LayoutFlexibleBox.cpp#newcode1373 Source/core/layout/LayoutFlexibleBox.cpp:1373: case ItemPositionStart: Ah, you're right about start vs flex-start, ...
5 years, 4 months ago (2015-08-11 23:04:58 UTC) #7
jfernandez
5 years ago (2015-11-26 11:32:08 UTC) #8
cbiesinger
5 years ago (2015-12-01 00:12:50 UTC) #9
https://codereview.chromium.org/1268753002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/css3/flexbox/flex-align-new-values.html
(right):

https://codereview.chromium.org/1268753002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/css3/flexbox/flex-align-new-values.html:19:
.horizontalTB { -webkit-writing-mode: horizontal-tb; }
no need for the -webkit- prefix

https://codereview.chromium.org/1268753002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/css3/flexbox/flex-align-new-values.html:20:
.horizontalBT { -webkit-writing-mode: horizontal-bt; }
hasn't this one been removed?

https://codereview.chromium.org/1268753002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/css3/flexbox/flex-align-new-values.html:35:
<script src="../../resources/check-layout.js"></script>
You can now use check-layout-th.js and then don't need the -expected.txt file
anymore. see other files in this directory (you need another <script>)

https://codereview.chromium.org/1268753002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right):

https://codereview.chromium.org/1268753002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1460: if
(isColumnFlow() && !style()->isLeftToRightDirection())
is that true? shouldn't it be if (!isHorizontalFlow() && ...) ?

same for right

https://codereview.chromium.org/1268753002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1468: case
ItemPositionLastBaseline: // TODO (lajava): This feature is at-risk of being
dropped during the CR period.
if we're still parsing it, it seems better not to have an assert_not_reached
here

Powered by Google App Engine
This is Rietveld 408576698