Bug 236640 – [CSS Container Queries] Support all size features

• Component: CSS
• Status: RESOLVED FIXED

[Antti Koivisto]

Support inline-size, block-size, aspect-ratio and orientation in addition to the currently supported width and height.

[Antti Koivisto]

Created attachment 452009 [details]
Patch

[Antti Koivisto]

Created attachment 452010 [details]
Patch

[Tim Nguyen (:ntim)]

Comment on attachment 452010 [details]
Patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:9393
> +		494EBD0627AA9C5300F4844F /* PDFDocument.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = PDFDocument.h; sourceTree = "<group>"; };
> +		494EBD0827AA9C5300F4844F /* PDFDocument.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = PDFDocument.cpp; sourceTree = "<group>"; };

You just did Dan's good-first-bug :(

https://bugs.webkit.org/show_bug.cgi?id=236529

Ditto here and below.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.h:250
> +

nit: unnecessary new line

> Source/WebCore/style/ContainerQueryEvaluator.cpp:150
> +    if (!is<CSSPrimitiveValue>(value))
> +        return { };
> +    auto& primitiveValue = downcast<CSSPrimitiveValue>(*value);

nit: Use dynamicDowncast<CSSPrimitiveValue> and you can combine with one of the conditions below.

> Source/WebCore/style/ContainerQueryEvaluator.cpp:210
> +    if (sizeFeature.name == CQ::FeatureNames::width())
> +        return evaluateSize(context.renderer.contentWidth());
> +
> +    if (sizeFeature.name == CQ::FeatureNames::height())
> +        return evaluateSize(context.renderer.contentHeight());
> +
> +    if (sizeFeature.name == CQ::FeatureNames::inlineSize())
> +        return evaluateSize(context.renderer.contentLogicalWidth());
> +
> +    if (sizeFeature.name == CQ::FeatureNames::blockSize())
> +        return evaluateSize(context.renderer.contentLogicalHeight());
> +
> +    if (sizeFeature.name == CQ::FeatureNames::aspectRatio()) {

nit: switch (sizeFeature.name) { ?

[Tim Nguyen (:ntim)]

(In reply to Tim Nguyen (:ntim) from comment #3)
> Comment on attachment 452010 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452010&action=review
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:9393
> > +		494EBD0627AA9C5300F4844F /* PDFDocument.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = PDFDocument.h; sourceTree = "<group>"; };
> > +		494EBD0827AA9C5300F4844F /* PDFDocument.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = PDFDocument.cpp; sourceTree = "<group>"; };
> 
> You just did Dan's good-first-bug :(

I meant undid*

[Tim Nguyen (:ntim)]

Comment on attachment 452010 [details]
Patch

Please address my non-switch comments.

[Sam Weinig]

Comment on attachment 452010 [details]
Patch

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

> LayoutTests/imported/w3c/web-platform-tests/css/css-contain/container-queries/unsupported-axis-expected.txt:13
> +FAIL size(block-size > 0px) assert_equals: expected "" but got "true"
> +FAIL size(block-size > 0px), with writing-mode:vertical-rl assert_equals: expected "" but got "true"

Expected that these go from PASS to FAIL?

[Antti Koivisto]

> > LayoutTests/imported/w3c/web-platform-tests/css/css-contain/container-queries/unsupported-axis-expected.txt:13
> > +FAIL size(block-size > 0px) assert_equals: expected "" but got "true"
> > +FAIL size(block-size > 0px), with writing-mode:vertical-rl assert_equals: expected "" but got "true"
> 
> Expected that these go from PASS to FAIL?

Yeah, it now fails with the axis stuff this test is actually going after.

[Antti Koivisto]

> nit: switch (sizeFeature.name) { ?

Strings don't work like that.

(In reply to Tim Nguyen (:ntim) from comment #3)
> Comment on attachment 452010 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452010&action=review
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:9393
> > +		494EBD0627AA9C5300F4844F /* PDFDocument.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = PDFDocument.h; sourceTree = "<group>"; };
> > +		494EBD0827AA9C5300F4844F /* PDFDocument.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = PDFDocument.cpp; sourceTree = "<group>"; };
> 
> You just did Dan's good-first-bug :(

Doesn't look like it, the files still appear in the right place in Xcode. I think the project file was just misordered for some reason.

> nit: switch (sizeFeature.name) { ?

Sadly doesn't work.

[Antti Koivisto]

Created attachment 452034 [details]
Patch for landing

[Antti Koivisto]

Created attachment 452036 [details]
Patch

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 452036 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Antti Koivisto]

Created attachment 452048 [details]
Patch for landing

[Antti Koivisto]

Created attachment 452049 [details]
Patch for landing

[EWS]

Committed r289838 (247286@main): <https://commits.webkit.org/247286@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/88977009>