Bug 70796 – flexbox flexing implementation should match the spec

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Ojan Vafai]

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.

[Tony Chang]

The spec hasn't been updated, but the current thinking is here:
http://lists.w3.org/Archives/Public/www-style/2011Oct/0533.html

With a JS implementation here:
http://www.xanthir.com/etc/flexplay/test.html

[Tony Chang]

Created attachment 133106 [details]
Patch

[Tony Chang]

Created attachment 133309 [details]
Patch

[Tony Chang]

(In reply to comment #3)
> Created an attachment (id=133309) [details]
> Patch

Rebase to ToT and drop m_ from struct member variables.

[Ojan Vafai]

Comment on attachment 133309 [details]
Patch

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)
    freezeViolations(...);
return !totalViolation

Not sure that's better though. It's fewer lines of code.

[Tony Chang]

Created attachment 133333 [details]
Patch

[Tony Chang]

Comment on attachment 133333 [details]
Patch

I made all the changes (it looks much better).  I'm going to let the bots chew through this before landing.

[WebKit Review Bot]

Comment on attachment 133333 [details]
Patch

Clearing flags on attachment: 133333

Committed r111767: <http://trac.webkit.org/changeset/111767>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.