Bug 62050 - add css parsing of flex()
Summary: add css parsing of flex()
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: 62049
Blocks: 62048
  Show dependency treegraph
 
Reported: 2011-06-03 14:02 PDT by Tony Chang
Modified: 2011-06-21 17:26 PDT (History)
11 users (show)

See Also:


Attachments
Patch (40.22 KB, patch)
2011-06-13 16:04 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (40.29 KB, patch)
2011-06-13 16:17 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (40.79 KB, patch)
2011-06-13 17:22 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (40.99 KB, patch)
2011-06-16 14:42 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (38.89 KB, patch)
2011-06-21 15:33 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (39.67 KB, patch)
2011-06-21 15:38 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (39.72 KB, patch)
2011-06-21 16:34 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch for landing (40.48 KB, patch)
2011-06-21 17:11 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 Tony Chang 2011-06-03 14:02:15 PDT
Specifically, we need to handle the new display property, flex-direction, flex-order, flex-pack, flex-align, the fr unit and the flex() function.

At a minimum, we could have something to test with just the display property and the flex() function.
Comment 1 Tony Chang 2011-06-13 15:53:35 PDT
I'm going to start smaller with just parsing flex() for width and height.  I'll add parsing of flex() for margin/padding/border in a follow up patch.
Comment 2 Tony Chang 2011-06-13 16:04:16 PDT
Created attachment 97026 [details]
Patch
Comment 3 WebKit Review Bot 2011-06-13 16:13:57 PDT
Comment on attachment 97026 [details]
Patch

Attachment 97026 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8837287
Comment 4 Tony Chang 2011-06-13 16:17:21 PDT
Created attachment 97029 [details]
Patch
Comment 5 Ojan Vafai 2011-06-13 16:50:57 PDT
Comment on attachment 97026 [details]
Patch

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

Nice test case! Code looks good to me except for the readability nits.

> LayoutTests/css3/flexbox/flex-parsing-expected.txt:11
> +PASS testFlex("-webkit-flex(1)", "width") is "-webkit-flex(1 0 auto)"
> +PASS testFlex("-webkit-flex(0)", "width") is "-webkit-flex(0 0 auto)"
> +PASS testFlex("-webkit-flex(2.4)", "width") is "-webkit-flex(2.4 0 auto)"
> +PASS testFlex("-webkit-flex(auto)", "width") is "-webkit-flex(1 0 auto)"

Here and below it's not clear to me whether we should return the normalized value or the value you set. For other properties (e.g. color) we seem inconsistent (e.g. if you set #CCC we return the rgb value, if you set "Blue" we return the lowercased string value and FireFox returns the case-sensitive string value).

I don't think we need to block this patch on figuring that out, but we should get the spec to be explicit.

> LayoutTests/css3/flexbox/flex-parsing-expected.txt:40
> +PASS testFlex("-webkit-flex(1 3px 2)", "width") is "-webkit-flex(1 2 3px)"
> +PASS testFlex("-webkit-flex(3px 1 2)", "width") is "-webkit-flex(1 2 3px)"
> +PASS testFlex("-webkit-flex(auto 0 0)", "width") is "-webkit-flex(0 0 auto)"
> +PASS testFlex("-webkit-flex(0 0px 0)", "width") is "-webkit-flex(0 0 0px)"
> +PASS testFlex("-webkit-flex(1 2 3)", "width") is ""

In quirks mode we usually allow non-suffixed pixel values.  Allowing that would complicate the case one line 36 though. Again, it would be good for the spec to be explicit about this.

> Source/WebCore/css/CSSParser.cpp:4803
> +        else if (arg2IsNonNegativeNumber && (arg1->id == CSSValueAuto || validUnit(arg1, FLength | FPercent | FNonNeg, m_strict)))

Moving "arg1->id == CSSValueAuto || validUnit(arg1, FLength | FPercent | FNonNeg, m_strict))" into a helper function would make this more readable.

> Source/WebCore/css/CSSParser.cpp:4814
> +        else if (arg1IsNonNegativeNumber && arg2IsNonNegativeNumber && !validUnit(arg3, FNumber, m_strict) && (arg3->id == CSSValueAuto || validUnit(arg3, FLength | FPercent | FNonNeg, m_strict)))

Moving "!validUnit(arg3, FNumber, m_strict) && (arg3->id == CSSValueAuto || validUnit(arg3, FLength | FPercent | FNonNeg, m_strict))" into a helper function would make this a lot more readable.
Comment 6 Ojan Vafai 2011-06-13 16:53:40 PDT
Tab, there's a few spec questions in comment 5. I'm not sure they need to be addressed in the Flex spec since they're generic CSS spec questions, but there should be some spec that addresses this and, in the meantime, the Flex spec might want to call it out explicitly.
Comment 7 Tony Chang 2011-06-13 17:22:42 PDT
Created attachment 97041 [details]
Patch
Comment 8 Tony Chang 2011-06-13 17:24:05 PDT
Also, Tab, if you have some other cases we should test, let me know.
Comment 9 Tab Atkins 2011-06-13 17:27:16 PDT
(In reply to comment #5)
> > LayoutTests/css3/flexbox/flex-parsing-expected.txt:11
> > +PASS testFlex("-webkit-flex(1)", "width") is "-webkit-flex(1 0 auto)"
> > +PASS testFlex("-webkit-flex(0)", "width") is "-webkit-flex(0 0 auto)"
> > +PASS testFlex("-webkit-flex(2.4)", "width") is "-webkit-flex(2.4 0 auto)"
> > +PASS testFlex("-webkit-flex(auto)", "width") is "-webkit-flex(1 0 auto)"
> 
> Here and below it's not clear to me whether we should return the normalized value or the value you set. For other properties (e.g. color) we seem inconsistent (e.g. if you set #CCC we return the rgb value, if you set "Blue" we return the lowercased string value and FireFox returns the case-sensitive string value).

I'll be adding an explicit section to the spec in the very near future, similar to the section in Image Values.  Hint: It will specify that we should normalize the value.


> > LayoutTests/css3/flexbox/flex-parsing-expected.txt:40
> > +PASS testFlex("-webkit-flex(1 3px 2)", "width") is "-webkit-flex(1 2 3px)"
> > +PASS testFlex("-webkit-flex(3px 1 2)", "width") is "-webkit-flex(1 2 3px)"
> > +PASS testFlex("-webkit-flex(auto 0 0)", "width") is "-webkit-flex(0 0 auto)"
> > +PASS testFlex("-webkit-flex(0 0px 0)", "width") is "-webkit-flex(0 0 0px)"
> > +PASS testFlex("-webkit-flex(1 2 3)", "width") is ""
> 
> In quirks mode we usually allow non-suffixed pixel values.  Allowing that would complicate the case one line 36 though. Again, it would be good for the spec to be explicit about this.

CSS doesn't say anything about this, because it's not a CSS issue - it's a private quirk of HTML that is getting carried through to CSS.  There's no way to resolve the ambiguity that this would introduce; I recommend not trying to do anything with the quirk here, and in general not spreading the quirk to new CSS things.
Comment 10 Tony Chang 2011-06-16 14:42:20 PDT
Created attachment 97504 [details]
Patch
Comment 11 Tony Chang 2011-06-16 14:43:05 PDT
The only change from the previous patch is to use StringBuilder in CSSFlexValue.cpp.

Can someone comfortable with CSSParser changes review this?
Comment 12 Tony Chang 2011-06-17 11:58:02 PDT
Maybe Simon can review?
Comment 13 Eric Seidel (no email) 2011-06-21 12:34:20 PDT
Comment on attachment 97504 [details]
Patch

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

In general this looks fine.

> Source/WebCore/css/CSSParser.cpp:792
> +        return primitiveValueCache()->createValue(value->fValue, (CSSPrimitiveValue::UnitTypes) value->unit);

We don't generally c-style cast.  Can we use a helper here to avoid these long lines?

> Source/WebCore/css/CSSParser.cpp:4787
> +bool CSSParser::parseFlex(int propId, bool important)

I suspect we could write this shorter with a differnet algorithm.  If this is the exact algorithm from the spec, we should add a comment to note that.
http://dev.w3.org/csswg/css3-flexbox/#flex-function

Here is an alternate algorithm for your consideration:

CSSParserValue* value = m_valueList->current();

CSSParserValueList* args = value->function->args.get();
if (!equalIgnoringCase(value->function->name, "-webkit-flex(") || !args)
    return false;

RefPtr<CSSFlexValue> flex;

if (args->size() > 3)
   return false;

double positive = -1;
double negative = -1;
CSSPrimativeValue* size;

while (CSSParserValue* arg = args->current()) {
    // First check to see if we have a raw number (no units).
    if (validUnit(arg, FNumber | FNonNeg, m_strict)) {
        if (positive == -1)
            positive = arg->fValue;
        else if (negative == -1)
            negative = arg->fValue;
        else {
            // flex() contains at most 2 non-size values.
            return false;
        }
    } else if (arg->id == CSSValueAuto || validUnit(arg, FLength | FPercent | FNonNeg, m_strict))
        size = parseValidPrimitive(arg->id, arg);
    } else {
        // This was not a valid arg for flex() we just bail.
        return false;
    }
    arg->next();
}
RefPtr<CSSFlexValue> flex = CSSFlexValue::create(arg2->fValue, 0, parseValidPrimitive(arg1->id, arg1));
addProperty(propId, flex.release(), important);
m_valueList->next(); // Tell the parser we're done with flex()?
return true;
Comment 14 Tony Chang 2011-06-21 15:33:08 PDT
Created attachment 98064 [details]
Patch
Comment 15 Tony Chang 2011-06-21 15:38:05 PDT
Created attachment 98068 [details]
Patch
Comment 16 Tony Chang 2011-06-21 15:39:07 PDT
Updated to use the alternate algorithm proposed by Eric since it's a bit shorter and easier to read.  I also added some more test cases (0 args and 4 args) and added a helper method for the static cast (not sure if I understood that properly).
Comment 17 Eric Seidel (no email) 2011-06-21 16:12:24 PDT
Comment on attachment 98068 [details]
Patch

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

Asside from the question of 0 0 0 parsing, I think this is ready to go.  I feel like I sent you on a bit of a yak-shave, for which I apologize. :(

> Source/WebCore/css/CSSParser.cpp:786
> +inline CSSPrimitiveValue::UnitTypes parserUnitToCSSPrimitiveValueUnit(int unit)
> +{
> +    return static_cast<CSSPrimitiveValue::UnitTypes>(unit);
> +}

Nah, what I was going for here was that we should just have a createPrimativeValue() helper on CSSParser, which does the whole shebang:

return primitiveValueCache()->createValue(value->string, parserUnitToCSSPrimitiveValueUnit(value->unit));

Maybe takes an enum?
CSSParser::createPrimativeValue(value, String)
CSSParser::createPrimativeValue(value, Float)

Maybe just is overloaded and is fed the two value->string and value->unit values?
CSSParser::createPrimativeValue(value->string, value->unit)
CSSParser::createPrimativeValue(value->fValue, value->unit)

Not sure.

In either case, it's sorta out of scope for this patch, so you can go back to your non-inline version if you prefer.  Thanks for the attempt.

> Source/WebCore/css/CSSParser.cpp:4790
> +    double positiveFlex = -1;

Were I thinking about it when I wrote up the example, I might have defined a const double invalidValue = -1, or unsetValue, or whatever nice name, and then used that in place of -1 throught the rest of the function.

> Source/WebCore/css/CSSParser.cpp:4800
> +            else if (!arg->fValue) {

I know webkit shies away from == 0, but I wonder if that's apprpriate here?  (I can never remember if we have to worry about 0 vs. -0 for floats...

> Source/WebCore/css/CSSParser.cpp:4801
> +                // flex() only allows a size of 0 if the positive and negative flex values have already been set.

Does this really work then?  It seems that (0 0 0) will be valid with this logic?  Since the earlier checks don't check arg->fValue?

> Source/WebCore/css/CSSParser.cpp:4804
> +            } else
> +                return false;

We should probably document why this returns false here.  Specifically it seems that when we see a number and we've already seen two other numbers that's an invalid flex.  e.g. flex(1 1 1).

> Source/WebCore/css/CSSParser.cpp:4805
> +        } else if (!preferredSize.get() && (arg->id == CSSValueAuto || validUnit(arg, FLength | FPercent | FNonNeg, m_strict)))

No need for the .get(), RefPtr is smart enough to overload operator bool.

> Source/WebCore/css/CSSParser.cpp:4818
> +    if (!preferredSize.get())

No .get() is necessary.

> Source/WebCore/css/CSSParser.cpp:4823
> +    m_valueList->next();

We should probably add a comment here.
Comment 18 Tony Chang 2011-06-21 16:34:50 PDT
Created attachment 98077 [details]
Patch
Comment 19 Eric Seidel (no email) 2011-06-21 16:44:09 PDT
Comment on attachment 98077 [details]
Patch

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

LGTM!

> Source/WebCore/css/CSSParser.cpp:788
> +        return primitiveValueCache()->createValue(value->string, (CSSPrimitiveValue::UnitTypes) value->unit);

We don't generally use C-style casts.  But you can land this as-is, especially if you have any plans to clean up this primativeValueCache()->createValue repetative nonsense throughout this file. :)
Comment 20 Tony Chang 2011-06-21 16:53:45 PDT
You reviewed quickly :)  For those who care, the spec says:

"If the first two values are non-negative numbers and the third value is ‘0’, the positive flexibility is set to the first value, the negative flexibility is set to the second value, and the preferred size is set to ‘0px’."

So 0 without units is valid as the third value.
Comment 21 Tony Chang 2011-06-21 17:11:13 PDT
Created attachment 98082 [details]
Patch for landing
Comment 22 WebKit Review Bot 2011-06-21 17:26:16 PDT
Comment on attachment 98082 [details]
Patch for landing

Clearing flags on attachment: 98082

Committed r89396: <http://trac.webkit.org/changeset/89396>
Comment 23 WebKit Review Bot 2011-06-21 17:26:23 PDT
All reviewed patches have been landed.  Closing bug.