Bug 116107 - REGRESSION: Basic flexbox layout with auto heights is broken.
Summary: REGRESSION: Basic flexbox layout with auto heights is broken.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords:
: 115329 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-05-14 08:50 PDT by Dave Hyatt
Modified: 2013-05-14 23:07 PDT (History)
9 users (show)

See Also:

Attachments
Simple reduction that illustrates the problem. (186 bytes, text/html)
2013-05-14 08:51 PDT, Dave Hyatt 		no flags Details
Reduction (173 bytes, text/html)
2013-05-14 08:51 PDT, Dave Hyatt 		no flags Details
Patch (3.71 KB, patch)
2013-05-14 08:57 PDT, Dave Hyatt 		no flags Details | Formatted Diff | Diff
Patch (3.97 KB, patch)
2013-05-14 09:02 PDT, Dave Hyatt 		no flags Details | Formatted Diff | Diff
Patch (fixed the intrinsic auto snafus) (3.97 KB, patch)
2013-05-14 09:05 PDT, Dave Hyatt 		no flags Details | Formatted Diff | Diff
Patch with layout test. (7.84 KB, patch)
2013-05-14 09:28 PDT, Dave Hyatt 		no flags Details | Formatted Diff | Diff
Patch (10.50 KB, patch)
2013-05-14 12:04 PDT, Dave Hyatt 		no flags Details | Formatted Diff | Diff
Patch (10.83 KB, patch)
2013-05-14 12:26 PDT, Dave Hyatt 		no flags Details | Formatted Diff | Diff
Patch (18.22 KB, patch)
2013-05-14 12:43 PDT, Dave Hyatt 		simon.fraser: review+
Details | Formatted Diff | Diff
Show Obsolete (8) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2013-05-14 08:50:46 PDT 
r147261 broke basic vertical flexbox layout, since flexing items no lay out with height of 0 when auto heights are specified. This is obviously wrong. They should be allowed to flex down to size 0, but when they are just getting an infinite amount of space to play with (which they should when computing an auto height), they should lay out at their preferred size.
Comment 1 Dave Hyatt 2013-05-14 08:51:20 PDT 
Created attachment 201718 [details]
Simple reduction that illustrates the problem.
Comment 2 Dave Hyatt 2013-05-14 08:51:52 PDT 
Created attachment 201719 [details]
Reduction
Comment 3 Dave Hyatt 2013-05-14 08:57:20 PDT 
Created attachment 201720 [details]
Patch
Comment 4 Dave Hyatt 2013-05-14 09:02:59 PDT 
Created attachment 201722 [details]
Patch
Comment 5 Dave Hyatt 2013-05-14 09:05:02 PDT 
Created attachment 201723 [details]
Patch (fixed the intrinsic auto snafus)
Comment 6 Simon Fraser (smfr) 2013-05-14 09:09:03 PDT 
Comment on attachment 201723 [details]
Patch (fixed the intrinsic auto snafus)

Do you plan to add a test case?
Comment 7 Dave Hyatt 2013-05-14 09:28:46 PDT 
Created attachment 201726 [details]
Patch with layout test.
Comment 8 Dave Hyatt 2013-05-14 11:17:00 PDT 
Cleared the review bit, since I misunderstood what flex-basis was. The current rendering is unfortunately correct according to the latest flexbox draft. I think the spec will end up changing though.

For now in WebKit only, I'm going to make flex-basis default to auto so that it behaves like the old flexbox. We can figure out what to do later when flex-basis stops churning.
Comment 9 Christian Biesinger 2013-05-14 12:02:11 PDT 
*** Bug 115329 has been marked as a duplicate of this bug. ***
Comment 10 Dave Hyatt 2013-05-14 12:04:19 PDT 
Created attachment 201738 [details]
Patch
Comment 11 Dave Hyatt 2013-05-14 12:26:58 PDT 
Created attachment 201741 [details]
Patch
Comment 12 WebKit Commit Bot 2013-05-14 12:28:56 PDT 
Attachment 201741 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/rendering/RenderFlexibleBox.cpp', u'Source/WebCore/rendering/RenderFlexibleBox.h']" exit_code: 1
Source/WebCore/rendering/RenderFlexibleBox.cpp:746:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Dave Hyatt 2013-05-14 12:43:24 PDT 
Created attachment 201744 [details]
Patch
Comment 14 Dave Hyatt 2013-05-14 13:36:46 PDT 
Landed in r150087.
Comment 15 Ryosuke Niwa 2013-05-14 23:07:16 PDT 
It seems like the test added by this patch has been failing on Mac:
e.g.
http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK1%20(Tests)/r150087%20(9969)/results.html