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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 290823003: Chrome incorrectly honors "align-content" in "flex-wrap: wrap" flex containers that only have a sin… (Closed)

Created:
6 years, 7 months ago by harpreet.sk
Modified:
6 years, 7 months ago
CC:
blink-reviews, blink-reviews-rendering, zoltan1, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, pdr., rune+blink
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Chrome incorrectly honors "align-content" in "flex-wrap: wrap" flex containers that only have a single line Chrome incorrectly honors "align-content" property in "flex-wrap: wrap" flex containers that only have a single line. According to the specification the "align-content" property should have no effect when the flex container has only a single line. Links to specific spec text: http://dev.w3.org/csswg/css-flexbox/#propdef-align-content This patch fixes this bug by adding check of numberOfLines to be greater than 1 in initialAlignContentOffset() BUG=324178 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174615

Patch Set 1 #

Total comments: 4

Patch Set 2 : Addresses the changes asked in patch set 1 #

Total comments: 2

Patch Set 3 : Merge all LayoutTests into one #

Patch Set 4 : nit: correction in indentation of layout test #

Patch Set 5 : Modified LayoutTests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+95 lines, -2 lines) Patch
A LayoutTests/css3/flexbox/alignContent-applies-with-flexWrap-wrap-with-single-line.html View 1 2 3 4 1 chunk +65 lines, -0 lines 0 comments Download
A LayoutTests/css3/flexbox/alignContent-applies-with-flexWrap-wrap-with-single-line-expected.txt View 1 2 3 4 1 chunk +26 lines, -0 lines 0 comments Download
M LayoutTests/css3/flexbox/multiline-align-content.html View 1 2 3 4 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/rendering/RenderFlexibleBox.cpp View 1 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 12 (0 generated)
harpreet.sk
PTAL
6 years, 7 months ago (2014-05-19 13:57:43 UTC) #1
tony
https://codereview.chromium.org/290823003/diff/1/LayoutTests/fast/flexbox/alignContent-flexWrap-with-single-line.html File LayoutTests/fast/flexbox/alignContent-flexWrap-with-single-line.html (right): https://codereview.chromium.org/290823003/diff/1/LayoutTests/fast/flexbox/alignContent-flexWrap-with-single-line.html#newcode6 LayoutTests/fast/flexbox/alignContent-flexWrap-with-single-line.html:6: <div style="display: flex; flex-wrap: wrap; align-content: flex-end; height: 100px"> ...
6 years, 7 months ago (2014-05-19 16:43:45 UTC) #2
harpreet.sk
PTAL https://codereview.chromium.org/290823003/diff/1/LayoutTests/fast/flexbox/alignContent-flexWrap-with-single-line.html File LayoutTests/fast/flexbox/alignContent-flexWrap-with-single-line.html (right): https://codereview.chromium.org/290823003/diff/1/LayoutTests/fast/flexbox/alignContent-flexWrap-with-single-line.html#newcode6 LayoutTests/fast/flexbox/alignContent-flexWrap-with-single-line.html:6: <div style="display: flex; flex-wrap: wrap; align-content: flex-end; height: ...
6 years, 7 months ago (2014-05-20 07:39:42 UTC) #3
tony
LGTM, but please merge the tests before committing. https://codereview.chromium.org/290823003/diff/10001/LayoutTests/fast/flexbox/alignContent-center-applies-with-flexWrap-wrap-with-single-line.html File LayoutTests/fast/flexbox/alignContent-center-applies-with-flexWrap-wrap-with-single-line.html (right): https://codereview.chromium.org/290823003/diff/10001/LayoutTests/fast/flexbox/alignContent-center-applies-with-flexWrap-wrap-with-single-line.html#newcode1 LayoutTests/fast/flexbox/alignContent-center-applies-with-flexWrap-wrap-with-single-line.html:1: <!doctype ...
6 years, 7 months ago (2014-05-20 16:21:10 UTC) #4
harpreet.sk
Please have a look at new LayoutTest before i commit the changes. https://codereview.chromium.org/290823003/diff/10001/LayoutTests/fast/flexbox/alignContent-center-applies-with-flexWrap-wrap-with-single-line.html File LayoutTests/fast/flexbox/alignContent-center-applies-with-flexWrap-wrap-with-single-line.html ...
6 years, 7 months ago (2014-05-21 08:26:04 UTC) #5
tony
Oh, also the test case is in the wrong directory. It should be in css3/flexbox ...
6 years, 7 months ago (2014-05-21 19:31:02 UTC) #6
harpreet.sk
On 2014/05/21 19:31:02, tony wrote: > Oh, also the test case is in the wrong ...
6 years, 7 months ago (2014-05-22 08:21:22 UTC) #7
harpreet.sk
I had made the changes as per your comment and also made change in one ...
6 years, 7 months ago (2014-05-22 08:24:25 UTC) #8
tony
Great, LGTM!
6 years, 7 months ago (2014-05-22 16:44:01 UTC) #9
harpreet.sk
The CQ bit was checked by harpreet.sk@samsung.com
6 years, 7 months ago (2014-05-22 17:32:04 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/harpreet.sk@samsung.com/290823003/70001
6 years, 7 months ago (2014-05-22 17:34:15 UTC) #11
commit-bot: I haz the power
6 years, 7 months ago (2014-05-23 00:35:30 UTC) #12
Message was sent while issue was closed. 
Change committed as 174615

Powered by Google App Engine
This is Rietveld 408576698