Bug 70796

Summary: flexbox flexing implementation should match the spec
Product: WebKit Reporter: Ojan Vafai <ojan>
Component: Layout and RenderingAssignee: Tony Chang <tony>
Severity: Normal CC: hyatt, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 62048    
Description Flags
Patch none

Description Ojan Vafai 2011-10-24 19:35:21 PDT
It should be updated shortly with normative text: http://dev.w3.org/csswg/css3-flexbox/#layout-algorithm. At the minimum, we'll certainly need to change how we handle min/max size constraints.
Comment 1 Tony Chang 2011-11-18 15:13:42 PST
The spec hasn't been updated, but the current thinking is here:

With a JS implementation here:
Comment 2 Tony Chang 2012-03-21 13:48:10 PDT
Created attachment 133106 [details]
Comment 3 Tony Chang 2012-03-22 11:55:08 PDT
Created attachment 133309 [details]
Comment 4 Tony Chang 2012-03-22 11:56:20 PDT
(In reply to comment #3)
> Created an attachment (id=133309) [details]
> Patch

Rebase to ToT and drop m_ from struct member variables.
Comment 5 Ojan Vafai 2012-03-22 12:10:03 PDT
Comment on attachment 133309 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=133309&action=review

> Source/WebCore/rendering/RenderFlexibleBox.cpp:787
> +            Length childMaxWidth = isHorizontalFlow() ? child->style()->maxWidth() : child->style()->maxHeight();
> +            Length childMinWidth = isHorizontalFlow() ? child->style()->minWidth() : child->style()->minHeight();

If you move these into adjustFlexSizeForMinAndMax, then you could use adjustFlexSizeForMinAndMax to replace lines 716-722 of computeNextFlexLine as well.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:798
> +        childSizes.append(childSize);

Nit: You could move this to replace line 778 and 790. Then you would only need the childSize variable in the else clause. Seems a bit cleaner to me.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:805
> +    if (!totalViolation)
> +        return true;
> +
> +    freezeViolations(totalViolation < 0 ? maxViolations : minViolations, availableFreeSpace, totalPositiveFlexibility, totalNegativeFlexibility, inflexibleItems);
> +    return false;

You could write this as:
if (totalViolation)
return !totalViolation

Not sure that's better though. It's fewer lines of code.
Comment 6 Tony Chang 2012-03-22 13:32:50 PDT
Created attachment 133333 [details]
Comment 7 Tony Chang 2012-03-22 13:33:24 PDT
Comment on attachment 133333 [details]

I made all the changes (it looks much better).  I'm going to let the bots chew through this before landing.
Comment 8 WebKit Review Bot 2012-03-22 14:46:15 PDT
Comment on attachment 133333 [details]

Clearing flags on attachment: 133333

Committed r111767: <http://trac.webkit.org/changeset/111767>
Comment 9 WebKit Review Bot 2012-03-22 14:46:20 PDT
All reviewed patches have been landed.  Closing bug.