Bug 224574

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.

I'll give this a stab.

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

Created attachment 426424 [details] Patch

Created attachment 426425 [details] Patch

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.

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!

Created attachment 426432 [details] Patch

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

Created attachment 426445 [details] Patch

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

Created attachment 426449 [details] Patch

Created attachment 426470 [details] Patch

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?

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

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

Created attachment 426524 [details] Patch

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