Bug 62050 – add css parsing of flex()

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

[Tony Chang]

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.

[Tony Chang]

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.

[Tony Chang]

Created attachment 97026 [details]
Patch

[WebKit Review Bot]

Comment on attachment 97026 [details]
Patch

Attachment 97026 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8837287

[Tony Chang]

Created attachment 97029 [details]
Patch

[Ojan Vafai]

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.

[Ojan Vafai]

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.

[Tony Chang]

Created attachment 97041 [details]
Patch

[Tony Chang]

Also, Tab, if you have some other cases we should test, let me know.

[Tab Atkins]

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

[Tony Chang]

Created attachment 97504 [details]
Patch

[Tony Chang]

The only change from the previous patch is to use StringBuilder in CSSFlexValue.cpp.

Can someone comfortable with CSSParser changes review this?

[Tony Chang]

Maybe Simon can review?

[Eric Seidel (no email)]

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;

[Tony Chang]

Created attachment 98064 [details]
Patch

[Tony Chang]

Created attachment 98068 [details]
Patch

[Tony Chang]

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

[Eric Seidel (no email)]

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.

[Tony Chang]

Created attachment 98077 [details]
Patch

[Eric Seidel (no email)]

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. :)

[Tony Chang]

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.

[Tony Chang]

Created attachment 98082 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 98082 [details]
Patch for landing

Clearing flags on attachment: 98082

Committed r89396: <http://trac.webkit.org/changeset/89396>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.