Bug 116107

Summary:

REGRESSION: Basic flexbox layout with auto heights is broken.

Product:

WebKit

Reporter:

Dave Hyatt <hyatt>

Component:

Layout and Rendering

Assignee:

Dave Hyatt <hyatt>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bdakin, cbiesinger, commit-queue, esprehn+autocc, glenn, mitz, ojan, rniwa, simon.fraser

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Simple reduction that illustrates the problem.	none
Reduction	none
Patch	none
Patch	none
Patch (fixed the intrinsic auto snafus)	none
Patch with layout test.	none
Patch	none
Patch	none
Patch	simon.fraser: review+

Dave Hyatt

Reported 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.

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 attachment proposed patch, testcase, etc.

Dave Hyatt

Comment 1 2013-05-14 08:51:20 PDT

Created attachment 201718 [details] Simple reduction that illustrates the problem.

Dave Hyatt

Comment 2 2013-05-14 08:51:52 PDT

Created attachment 201719 [details] Reduction

Dave Hyatt

Comment 3 2013-05-14 08:57:20 PDT

Created attachment 201720 [details] Patch

Dave Hyatt

Comment 4 2013-05-14 09:02:59 PDT

Created attachment 201722 [details] Patch

Dave Hyatt

Comment 5 2013-05-14 09:05:02 PDT

Created attachment 201723 [details] Patch (fixed the intrinsic auto snafus)

Simon Fraser (smfr)

Comment 6 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?

Dave Hyatt

Comment 7 2013-05-14 09:28:46 PDT

Created attachment 201726 [details] Patch with layout test.

Dave Hyatt

Comment 8 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.

Christian Biesinger

Comment 9 2013-05-14 12:02:11 PDT

*** Bug 115329 has been marked as a duplicate of this bug. ***

Dave Hyatt

Comment 10 2013-05-14 12:04:19 PDT

Created attachment 201738 [details] Patch

Dave Hyatt

Comment 11 2013-05-14 12:26:58 PDT

Created attachment 201741 [details] Patch

WebKit Commit Bot

Comment 12 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.

Dave Hyatt

Comment 13 2013-05-14 12:43:24 PDT

Created attachment 201744 [details] Patch

Dave Hyatt

Comment 14 2013-05-14 13:36:46 PDT

Landed in r150087.

Ryosuke Niwa

Comment 15 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

Note You need to log in before you can comment on or make changes to this bug.