Summary: | [CSS Container Queries] Support all size features | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||||||||||||
Component: | CSS | Assignee: | Antti Koivisto <koivisto> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, macpherson, menard, ntim, sam, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 229659 | ||||||||||||||||
Attachments: |
|
Description
Antti Koivisto
2022-02-15 02:42:54 PST
Created attachment 452009 [details]
Patch
Created attachment 452010 [details]
Patch
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) { ? (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* Comment on attachment 452010 [details]
Patch
Please address my non-switch comments.
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? > > 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.
> 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. Created attachment 452034 [details]
Patch for landing
Created attachment 452036 [details]
Patch
Tools/Scripts/svn-apply failed to apply attachment 452036 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Created attachment 452048 [details]
Patch for landing
Created attachment 452049 [details]
Patch for landing
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]. |