Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1314703006:
Flex grow wrongly calculated for value between 0 and 1 (Closed)
DescriptionFlex grow wrongly calculated for value between 0 and 1
When flex grow value for a flex item is set between 0 and 1,
the extra space to flex for the flex item is wrongly calculated.
If the value of totalFlexGrow is less than 1, then the available
space would be totalFlexGrow times the available space. And
then this space is distributed to all the flex items within the flex line.
The spec describing it is:
http://dev.w3.org/csswg/css-flexbox-1/#resolve-flexible-lengths
...specifically this line "If the sum of the unfrozen flex items’ flex factors
is less than one, multiply the initial free space by this sum."
BUG=480752
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : #
Total comments: 24
Patch Set 4 : #Patch Set 5 #
Messages
Total messages: 30 (10 generated)
ashlin.j@samsung.com changed reviewers: + harpreet.sk@samsung.com, sanjoy.pal@samsung.com
Qucik Review: Add more test cases in layout test checking for column flexbox and for vertical writing mode as well https://codereview.chromium.org/1314703006/diff/20001/LayoutTests/css3/flexbo... File LayoutTests/css3/flexbox/flex-grow-less-than-one.html (right): https://codereview.chromium.org/1314703006/diff/20001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:2: <html> We generally skip html, head and body element. https://codereview.chromium.org/1314703006/diff/20001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:30: Remove Extra lines and spaces. Make your test concise. https://codereview.chromium.org/1314703006/diff/20001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:52: </body> html not required.
On 2015/09/07 06:10:16, harpreet.sk wrote: > Qucik Review: Add more test cases in layout test checking for column flexbox and > for vertical writing mode as well > > https://codereview.chromium.org/1314703006/diff/20001/LayoutTests/css3/flexbo... > File LayoutTests/css3/flexbox/flex-grow-less-than-one.html (right): > > https://codereview.chromium.org/1314703006/diff/20001/LayoutTests/css3/flexbo... > LayoutTests/css3/flexbox/flex-grow-less-than-one.html:2: <html> > We generally skip html, head and body element. > > https://codereview.chromium.org/1314703006/diff/20001/LayoutTests/css3/flexbo... > LayoutTests/css3/flexbox/flex-grow-less-than-one.html:30: > Remove Extra lines and spaces. Make your test concise. > > https://codereview.chromium.org/1314703006/diff/20001/LayoutTests/css3/flexbo... > LayoutTests/css3/flexbox/flex-grow-less-than-one.html:52: </body> > html not required. All comments have been resolved. PTAL
https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... File LayoutTests/css3/flexbox/flex-grow-less-than-one.html (right): https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:4: .parent { Rename it to something more appropriate something like "container" or check other test for naming conventions. https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:10: .child-flex-grow-1 { Rename classes so that it clearly explain what it is doing. https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:15: .child-flex-grow-2 { Ditto. https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:23: child flex-grow-3 and 4 are not required. We need to check for flex-grow value between 0 and 1. https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:32: Remove line. https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:36: <div>Test Case-1</div> Not required. https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:41: <div>Test Case-2</div> Ditto. https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:47: <div>Test Case-3</div> Ditto. https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:53: <div>Test Case-4</div> Ditto. https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:59: <div>Test Case-5</div> Ditto. https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:65: <div>Test Case-6</div> Ditto. https://codereview.chromium.org/1314703006/diff/40001/Source/core/layout/Layo... File Source/core/layout/LayoutFlexibleBox.cpp (right): https://codereview.chromium.org/1314703006/diff/40001/Source/core/layout/Layo... Source/core/layout/LayoutFlexibleBox.cpp:988: if (totalFlexGrow > 0 && totalFlexGrow < 1) totalFlexGrow > 0 not required as it is already checked in above if condition.
On 2015/09/07 13:45:45, harpreet.sk wrote: > https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... > File LayoutTests/css3/flexbox/flex-grow-less-than-one.html (right): > > https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... > LayoutTests/css3/flexbox/flex-grow-less-than-one.html:4: .parent { > Rename it to something more appropriate something like "container" or check > other test for naming conventions. > > https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... > LayoutTests/css3/flexbox/flex-grow-less-than-one.html:10: .child-flex-grow-1 { > Rename classes so that it clearly explain what it is doing. > > https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... > LayoutTests/css3/flexbox/flex-grow-less-than-one.html:15: .child-flex-grow-2 { > Ditto. > > https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... > LayoutTests/css3/flexbox/flex-grow-less-than-one.html:23: > child flex-grow-3 and 4 are not required. We need to check for flex-grow value > between 0 and 1. > > https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... > LayoutTests/css3/flexbox/flex-grow-less-than-one.html:32: > Remove line. > > https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... > LayoutTests/css3/flexbox/flex-grow-less-than-one.html:36: <div>Test Case-1</div> > Not required. > > https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... > LayoutTests/css3/flexbox/flex-grow-less-than-one.html:41: <div>Test Case-2</div> > Ditto. > > https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... > LayoutTests/css3/flexbox/flex-grow-less-than-one.html:47: <div>Test Case-3</div> > Ditto. > > https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... > LayoutTests/css3/flexbox/flex-grow-less-than-one.html:53: <div>Test Case-4</div> > Ditto. > > https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... > LayoutTests/css3/flexbox/flex-grow-less-than-one.html:59: <div>Test Case-5</div> > Ditto. > > https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... > LayoutTests/css3/flexbox/flex-grow-less-than-one.html:65: <div>Test Case-6</div> > Ditto. > > https://codereview.chromium.org/1314703006/diff/40001/Source/core/layout/Layo... > File Source/core/layout/LayoutFlexibleBox.cpp (right): > > https://codereview.chromium.org/1314703006/diff/40001/Source/core/layout/Layo... > Source/core/layout/LayoutFlexibleBox.cpp:988: if (totalFlexGrow > 0 && > totalFlexGrow < 1) > totalFlexGrow > 0 not required as it is already checked in above if condition. All changes are made at Patch Set4. Please take a look
harpreet.sk@samsung.com changed reviewers: + cbiesinger@chromium.org, tony@chromium.org
ashlin.j@samsung.com changed reviewers: - cbiesinger@chromium.org, tony@chromium.org
https://codereview.chromium.org/1314703006/diff/20001/LayoutTests/css3/flexbo... File LayoutTests/css3/flexbox/flex-grow-less-than-one.html (right): https://codereview.chromium.org/1314703006/diff/20001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:2: <html> On 2015/09/07 06:10:16, harpreet.sk wrote: > We generally skip html, head and body element. Done. https://codereview.chromium.org/1314703006/diff/20001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:30: On 2015/09/07 06:10:16, harpreet.sk wrote: > Remove Extra lines and spaces. Make your test concise. Done. https://codereview.chromium.org/1314703006/diff/20001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:52: </body> On 2015/09/07 06:10:16, harpreet.sk wrote: > html not required. Done. https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... File LayoutTests/css3/flexbox/flex-grow-less-than-one.html (right): https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:4: .parent { On 2015/09/07 13:45:44, harpreet.sk wrote: > Rename it to something more appropriate something like "container" or check > other test for naming conventions. Done. https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:10: .child-flex-grow-1 { On 2015/09/07 13:45:44, harpreet.sk wrote: > Rename classes so that it clearly explain what it is doing. Done. https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:15: .child-flex-grow-2 { On 2015/09/07 13:45:45, harpreet.sk wrote: > Ditto. Done. https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:23: On 2015/09/07 13:45:44, harpreet.sk wrote: > child flex-grow-3 and 4 are not required. We need to check for flex-grow value > between 0 and 1. Done. https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:32: On 2015/09/07 13:45:44, harpreet.sk wrote: > Remove line. Done. https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:36: <div>Test Case-1</div> On 2015/09/07 13:45:44, harpreet.sk wrote: > Not required. Done. https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:41: <div>Test Case-2</div> On 2015/09/07 13:45:44, harpreet.sk wrote: > Ditto. Done. https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:47: <div>Test Case-3</div> On 2015/09/07 13:45:45, harpreet.sk wrote: > Ditto. Done. https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:53: <div>Test Case-4</div> On 2015/09/07 13:45:44, harpreet.sk wrote: > Ditto. Done. https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:59: <div>Test Case-5</div> On 2015/09/07 13:45:44, harpreet.sk wrote: > Ditto. Done. https://codereview.chromium.org/1314703006/diff/40001/LayoutTests/css3/flexbo... LayoutTests/css3/flexbox/flex-grow-less-than-one.html:65: <div>Test Case-6</div> On 2015/09/07 13:45:44, harpreet.sk wrote: > Ditto. Done. https://codereview.chromium.org/1314703006/diff/40001/Source/core/layout/Layo... File Source/core/layout/LayoutFlexibleBox.cpp (right): https://codereview.chromium.org/1314703006/diff/40001/Source/core/layout/Layo... Source/core/layout/LayoutFlexibleBox.cpp:988: if (totalFlexGrow > 0 && totalFlexGrow < 1) On 2015/09/07 13:45:45, harpreet.sk wrote: > totalFlexGrow > 0 not required as it is already checked in above if condition. Done.
harpreet.sk@samsung.com changed reviewers: + cbiesinger@chromium.org, tony@chromium.org
Please take a look.
lgtm
Note that this should also apply to shrinking per https://lists.w3.org/Archives/Public/www-style/2015Sep/0225.html, but that can be a separate change.
The CQ bit was checked by ashlin.j@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1314703006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1314703006/80001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to commit the patch. Adding LayoutTests/css3/flexbox/flex-grow-less-than-one-expected.txt Adding LayoutTests/css3/flexbox/flex-grow-less-than-one.html Sending Source/core/layout/LayoutFlexibleBox.cpp Transmitting file data ..svn: E165001: Commit failed (details follow): svn: E165001: Commit blocked by pre-commit hook (exit code 2) with output: Sorry, /trunk/ is locked in read-only mode.
The CQ bit was checked by harpreet.sk@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1314703006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1314703006/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ashlin.j@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1314703006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1314703006/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_android on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
You're going to have to recreate this codereview with a new checkout, now that blink has been merged into the main repository.
On 2015/09/28 16:26:47, cbiesinger wrote: > You're going to have to recreate this codereview with a new checkout, now that > blink has been merged into the main repository. Have uploaded the patch at https://codereview.chromium.org/1375163002.
On 2015/09/28 16:26:47, cbiesinger wrote: > You're going to have to recreate this codereview with a new checkout, now that > blink has been merged into the main repository. Have uploaded the patch at https://codereview.chromium.org/1375163002.
On 2015/09/28 16:26:47, cbiesinger wrote: > You're going to have to recreate this codereview with a new checkout, now that > blink has been merged into the main repository. Have uploaded the patch at https://codereview.chromium.org/1375163002.
On 2015/09/22 23:37:16, cbiesinger wrote: > Note that this should also apply to shrinking per > https://lists.w3.org/Archives/Public/www-style/2015Sep/0225.html, but that can > be a separate change. Will do that.
I'll close this change because it landed separately as https://codereview.chromium.org/1375163002 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
