Bug 70796 - flexbox flexing implementation should match the spec
Summary: flexbox flexing implementation should match the spec
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: Tony Chang
URL:
Keywords:
Depends on:
Blocks: 62048
  Show dependency treegraph
 
Reported: 2011-10-24 19:35 PDT by Ojan Vafai
Modified: 2012-03-22 14:46 PDT (History)
3 users (show)

See Also:


Attachments
Patch (12.45 KB, patch)
2012-03-21 13:48 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (12.58 KB, patch)
2012-03-22 11:55 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (15.13 KB, patch)
2012-03-22 13:32 PDT, Tony Chang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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:
http://lists.w3.org/Archives/Public/www-style/2011Oct/0533.html

With a JS implementation here:
http://www.xanthir.com/etc/flexplay/test.html
Comment 2 Tony Chang 2012-03-21 13:48:10 PDT
Created attachment 133106 [details]
Patch
Comment 3 Tony Chang 2012-03-22 11:55:08 PDT
Created attachment 133309 [details]
Patch
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]
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.
Comment 6 Tony Chang 2012-03-22 13:32:50 PDT
Created attachment 133333 [details]
Patch
Comment 7 Tony Chang 2012-03-22 13:33:24 PDT
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.
Comment 8 WebKit Review Bot 2012-03-22 14:46:15 PDT
Comment on attachment 133333 [details]
Patch

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.