Bug 224574 – [css-display-3] Implement CSS Display Two Value Notation

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Jen Simmons]

CSS Display Module Level 3 introduces the two-value syntax for the display property. 

This provides Authors with an alternative way to write the display property — one that makes a lot of sense & improves Author experience. 

Examples of the new notation: 
```
display: block grid;
display: block flex;
display: inline flex;
display: block flow;
display: block flow-root;
```

Without implementation in all browsers, Authors cannot start using this new notation.

Simply aliasing the already currently supported display properties so that this new notation works is a fantastic MVP. We can ship just that. 

The spec is in CR (has been since Aug 2018): https://www.w3.org/TR/css-display-3/

[Sam Sneddon [:gsnedders]]

https://wpt.fyi/results/css/css-display/parsing/display-valid.html?label=master&label=experimental&aligned has a variety of tests for this, at least for the parsing side of it. Alas, it seems that /css/css-display/display-flow-root-list-item-001.html is the only WPT test for the functional side of it.

Currently, it appears that Gecko is only engine to implement this.

[Tim Nguyen (:ntim)]

I'll give this a stab.

[Tim Nguyen (:ntim)]

> Simply aliasing the already currently supported display properties so that this new notation works is a fantastic MVP. We can ship just that. 

This needs proper parser support. The values can be swapped (e.g. `grid inline` is also valid, instead of `inline grid` or `inline-grid`). Not to mention excessive whitespace between keywords which the parser needs to support too.

[Tim Nguyen (:ntim)]

Created attachment 426424 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 426425 [details]
Patch

[Antti Koivisto]

Comment on attachment 426425 [details]
Patch

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

> LayoutTests/imported/w3c/ChangeLog:9
> +        Also cleans up handling of -webkit-flex & -webkit-inline-flex on the way.

Does this need to be done in this patch? Sounds like something better done separately first.

[Antti Koivisto]

Comment on attachment 426425 [details]
Patch

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

Looks pretty good to me

> Source/WebCore/css/parser/CSSPropertyParser.cpp:417
> +    >(range);
> +    if (singleKeyword)
> +        return singleKeyword;

Please add an empty line before if for readability.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:450
> +    // Set defaults when one of the two values are unspecified
> +    displayOutside = displayOutside.valueOr(CSSValueBlock);
> +    displayInside = displayInside.valueOr(CSSValueFlow);

What if both of them were unspecified?

> Source/WebCore/css/parser/CSSPropertyParser.cpp:452
> +    auto cssValuePool = CSSValuePool::singleton();

You are making a copy of the CSSValuePool. You'll want

auto& cssValuePool = CSSValuePool::singleton();

Surprised that CSSValuePool is copyable, we should take care to delete the copy-constructor!

[Tim Nguyen (:ntim)]

Created attachment 426432 [details]
Patch

[Antti Koivisto]

Comment on attachment 426432 [details]
Patch

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

> Source/WebCore/css/parser/CSSPropertyParser.cpp:421
> +    Optional<CSSValueID> displayOutside, displayInside;

WebKit coding style wants variable declaration on separate lines as separate statements.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:445
> +        switch (range.peek().id()) {
> +        // <display-outside>
> +        case CSSValueBlock:
> +        case CSSValueInline:
> +            if (displayOutside)
> +                return nullptr;
> +            displayOutside = range.peek().id();
> +            consumeIdent(range);
> +            break;
> +        // <display-inside>
> +        case CSSValueFlex:
> +        case CSSValueFlow:
> +        case CSSValueFlowRoot:
> +        case CSSValueGrid:
> +        case CSSValueTable:
> +            if (displayInside)
> +                return nullptr;
> +            displayInside = range.peek().id();
> +            consumeIdent(range);
> +            break;
> +        default:
> +            return nullptr;
> +        }

you could put range.peek().id() into a variable to avoid repetition

consumeIdent(range) could be after the switch.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:475
> +    displayOutside = displayOutside.valueOr(CSSValueBlock);
> +    displayInside = displayInside.valueOr(CSSValueFlow);
> +
> +    auto& cssValuePool = CSSValuePool::singleton();
> +    if (*displayOutside == CSSValueBlock) {
> +        // Alias display: flow to display: block
> +        if (*displayInside == CSSValueFlow)
> +            return cssValuePool.createValue(CSSValueBlock);
> +        return cssValuePool.createValue(*displayInside);
> +    }
> +
> +    // Convert `display: inline <display-inside>` to the equivalent short value
> +    switch (*displayInside) {
> +    case CSSValueFlex:
> +        return cssValuePool.createValue(CSSValueInlineFlex);
> +    case CSSValueFlow:
> +        return cssValuePool.createValue(CSSValueInline);
> +    case CSSValueFlowRoot:
> +        return cssValuePool.createValue(CSSValueInlineBlock);
> +    case CSSValueGrid:
> +        return cssValuePool.createValue(CSSValueInlineGrid);
> +    case CSSValueTable:
> +        return cssValuePool.createValue(CSSValueInlineTable);
> +    default:
> +        ASSERT_NOT_REACHED();
> +        return nullptr;
> +    }

You could factor this stuff into a function (I'd use lambda) that takes displayOutside and displayInside and returns short value.

The CSSValue construction could be done after, something like

return CSSValuePool::singleton().createValue(longValuesToShortValue(displayOutside, displayInside));

[Tim Nguyen (:ntim)]

Created attachment 426445 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Tim Nguyen (:ntim)]

https://github.com/web-platform-tests/wpt/pull/28577

[Tim Nguyen (:ntim)]

Created attachment 426449 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 426470 [details]
Patch

[Darin Adler]

Comment on attachment 426470 [details]
Patch

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

> Source/WebCore/css/parser/CSSPropertyParser.cpp:385
> +    // Parse single keyword values

I think this would read better as a separate function: consumeSingleKeywordDisplayValue perhaps.

Then the early return here could be scoped inside the if:

    if (auto keyword = consumeSingleKeywordDisplayValue(range))
        return keyword;

> Source/WebCore/css/parser/CSSPropertyParser.cpp:451
> +    displayOutside = displayOutside.valueOr(CSSValueBlock);
> +    displayInside = displayInside.valueOr(CSSValueFlow);

If these went into new locals instead of being modified in place then they would not be optional any more which could get rid of the need for the * below. Could name the ones inside the loop "parsedDisplayOutside" and then use new locals here for the "valueOr" results.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:453
> +    auto longValuesToShortValue = [](auto displayOutside, auto displayInside) {

Not sure about capturing vs. arguments for this lambda, I think it should be called just selectShortValue or shortValue, with no arguments. The only reason it exists is so we can use "return". Above we use local variables instead of return statements and a block. Could do it either way.

> LayoutTests/imported/w3c/web-platform-tests/html/rendering/widgets/button-layout/computed-style.html:31
> +      if (val == 'flow') {
> +        // Use the more backwards-compatible form, `block` is better than `flow`
> +        // https://drafts.csswg.org/cssom/#serializing-css-values
> +        expectedVal = 'block';
> +      }

Is this change also being done upstream?

[Antti Koivisto]

> Not sure about capturing vs. arguments for this lambda, I think it should be
> called just selectShortValue or shortValue, with no arguments. The only
> reason it exists is so we can use "return". Above we use local variables
> instead of return statements and a block. Could do it either way.

While ability to use return is nice that's not the only reason I like lambdas for this sort of things. Dividing code into logical units with names improves readability and provides implicit documentation.

[Tim Nguyen (:ntim)]

(In reply to Darin Adler from comment #15)
> Comment on attachment 426470 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426470&action=review
> 
> > Source/WebCore/css/parser/CSSPropertyParser.cpp:385
> > +    // Parse single keyword values
> 
> I think this would read better as a separate function:
> consumeSingleKeywordDisplayValue perhaps.
> 
> Then the early return here could be scoped inside the if:
> 
>     if (auto keyword = consumeSingleKeywordDisplayValue(range))
>         return keyword;

I think it's essential to the understanding of consumeDisplay to know what those "single keywords" are (e.g. <display-box>/<display-internal>/<display-legacy>/prefixed), that would be a bit long to reflect in the function name.
Also `display: inline` or `display: block` (also single keywords) would be covered by the block underneath (`// Parse [ <display-outside> || <display-inside> ]`), so I don't think abstracting this out into a `consumeSingleKeywordDisplayValue` function is a good idea.

> > Source/WebCore/css/parser/CSSPropertyParser.cpp:451
> > +    displayOutside = displayOutside.valueOr(CSSValueBlock);
> > +    displayInside = displayInside.valueOr(CSSValueFlow);
> 
> If these went into new locals instead of being modified in place then they
> would not be optional any more which could get rid of the need for the *
> below. Could name the ones inside the loop "parsedDisplayOutside" and then
> use new locals here for the "valueOr" results.

Sounds good, will change that.

> > Source/WebCore/css/parser/CSSPropertyParser.cpp:453
> > +    auto longValuesToShortValue = [](auto displayOutside, auto displayInside) {
> 
> Not sure about capturing vs. arguments for this lambda, I think it should be
> called just selectShortValue or shortValue, with no arguments. The only
> reason it exists is so we can use "return". Above we use local variables
> instead of return statements and a block. Could do it either way.

I don't have a strong preference on this tbh. I've switched to capturing so it feels slightly more linear to read.

> > LayoutTests/imported/w3c/web-platform-tests/html/rendering/widgets/button-layout/computed-style.html:31
> > +      if (val == 'flow') {
> > +        // Use the more backwards-compatible form, `block` is better than `flow`
> > +        // https://drafts.csswg.org/cssom/#serializing-css-values
> > +        expectedVal = 'block';
> > +      }
> 
> Is this change also being done upstream?

Yes, it's been merged too, see https://github.com/web-platform-tests/wpt/pull/28577.

[Tim Nguyen (:ntim)]

Created attachment 426524 [details]
Patch

[EWS]

Committed r276293 (236775@main): <https://commits.webkit.org/236775@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426524 [details].

[Ling Ho]

rdar://76888572