https://github.com/w3c/csswg-drafts/blob/main/css-sizing-4/Overview.bs
Created attachment 455318 [details] Patch
Created attachment 455367 [details] Patch
Comment on attachment 455367 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455367&action=review > Source/WebCore/ChangeLog:11 > + value for Length and AutoAndLenght type. AutoAndLength. > Source/WebCore/css/parser/CSSPropertyParser.cpp:4101 > + list->append(CSSValuePool::singleton().createIdentifierValue(CSSValueAuto)); Better here to use autoValue I think.
Created attachment 455758 [details] Patch
Patch LGTM to me :) I will leave it for an Apple person to do the final review though.
Comment on attachment 455758 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455758&action=review > Source/WebCore/css/parser/CSSParserContext.cpp:219 > + uint64_t bits2 = context.containIntrinsicSizeEnabled << 0 > + | (unsigned long long)context.mode << 1; // This is multiple bits, so keep it last. Why the need for bits2? bits is 64 bit, so we should be able to go up to 1 << 63.
Comment on attachment 455758 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455758&action=review >> Source/WebCore/css/parser/CSSParserContext.cpp:219 >> + | (unsigned long long)context.mode << 1; // This is multiple bits, so keep it last. > > Why the need for bits2? bits is 64 bit, so we should be able to go up to 1 << 63. Yes, sizeof(uint64_t) is 8. But I got this compiling error, not sure why... ``` ./css/parser/CSSParserContext.cpp:217:61: error: shift count >= width of type [-Werror,-Wshift-count-overflow] | context.containIntrinsicSizeEnabled << 32 ^ ~~ 1 error generated ```
Comment on attachment 455758 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455758&action=review >>> Source/WebCore/css/parser/CSSParserContext.cpp:219 >>> + | (unsigned long long)context.mode << 1; // This is multiple bits, so keep it last. >> >> Why the need for bits2? bits is 64 bit, so we should be able to go up to 1 << 63. > > Yes, sizeof(uint64_t) is 8. But I got this compiling error, not sure why... > > ``` > ./css/parser/CSSParserContext.cpp:217:61: error: shift count >= width of type [-Werror,-Wshift-count-overflow] > | context.containIntrinsicSizeEnabled << 32 > ^ ~~ > 1 error generated > ``` Done! Thanks, Rob, for pointing out context.containIntrinsicSizeEnabled is only 32 bit, I just need to add a cast here.
Created attachment 455911 [details] Patch
<rdar://problem/90919423>
Comment on attachment 455911 [details] Patch Hi, I think the patch is ready for review, please take a look. Thanks!
Created attachment 456820 [details] Patch
Created attachment 456837 [details] Patch
Can you fix the assert: https://ews-build.s3-us-west-2.amazonaws.com/macOS-AppleSilicon-Big-Sur-Debug-WK2-Tests-EWS/456837-28300-rerun/accessibility/mac/media-role-descriptions-stderr.txt ? Seems like this needs animation support.
Created attachment 456899 [details] Patch
(In reply to Tim Nguyen (:ntim) from comment #14) > Can you fix the assert: > > https://ews-build.s3-us-west-2.amazonaws.com/macOS-AppleSilicon-Big-Sur- > Debug-WK2-Tests-EWS/456837-28300-rerun/accessibility/mac/media-role- > descriptions-stderr.txt > > ? Seems like this needs animation support. Thanks for the heads up, the latest patch should fix it.
Created attachment 457027 [details] Patch
Created attachment 457541 [details] Patch
This is now ready for review :)
Comment on attachment 457541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457541&action=review > Source/WebCore/css/parser/CSSParserContext.cpp:217 > + | (unsigned long long)context.containIntrinsicSizeEnabled << 32 Why does this need to be (unsigned long long)? I assume this is bad copy paste? > Source/WebCore/css/parser/CSSPropertyParser.cpp:4097 > + break; Why not `return nullptr;`? > Source/WebCore/rendering/style/StyleRareNonInheritedData.h:240 > + unsigned containIntrinsicWidthType : 3; > + unsigned containIntrinsicHeightType : 3; Does this not fit in 2? The enum only has 3 values and 2**2 == 4
Created attachment 457708 [details] Patch
Comment on attachment 457541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457541&action=review >> Source/WebCore/css/parser/CSSParserContext.cpp:217 >> + | (unsigned long long)context.containIntrinsicSizeEnabled << 32 > > Why does this need to be (unsigned long long)? I assume this is bad copy paste? Yes that was a bit lazy, fixed. >> Source/WebCore/css/parser/CSSPropertyParser.cpp:4097 >> + break; > > Why not `return nullptr;`? Why not indeed. Fixed. >> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:240 >> + unsigned containIntrinsicHeightType : 3; > > Does this not fit in 2? The enum only has 3 values and 2**2 == 4 It should fit indeed. Fixed.
Comment on attachment 457708 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457708&action=review Don't forget to fix RenderStyle::diff() in this or a future patch. > Source/WebCore/rendering/style/StyleRareNonInheritedData.h:240 > + unsigned containIntrinsicWidthType : 2; > + unsigned containIntrinsicHeightType : 2; Add a `// ContainIntrinsicSizeType` comment on both lines > Source/WebCore/style/StyleBuilderCustom.h:2167 > + CSSPropertyID newId = CSSProperty::resolveDirectionAwareProperty(CSSPropertyContainIntrinsicBlockSize, style.direction(), style.writingMode()); Maybe `auto resolvedID = `
Comment on attachment 457708 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457708&action=review > Source/WebCore/css/parser/CSSParserContext.cpp:217 > + | (uint64_t)context.containIntrinsicSizeEnabled << 32 Does: | context.containIntrinsicSizeEnabled << 32 just work here? (just for consistency for the other *Enabled flags)
Comment on attachment 457708 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457708&action=review >> Source/WebCore/css/parser/CSSParserContext.cpp:217 >> + | (uint64_t)context.containIntrinsicSizeEnabled << 32 > > Does: > | context.containIntrinsicSizeEnabled << 32 > > just work here? (just for consistency for the other *Enabled flags) Looks like not, we will get the following error: ``` ./css/parser/CSSParserContext.cpp:217:61: error: shift count >= width of type [-Werror,-Wshift-count-overflow] | context.containIntrinsicSizeEnabled << 32 ^ ~~ 1 error generated ``` For context.containIntrinsicSizeEnabled is 32 bit, we need to extend it to 64 bit, then it could be moved 32.
Comment on attachment 457708 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457708&action=review >> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:240 >> + unsigned containIntrinsicHeightType : 2; > > Add a `// ContainIntrinsicSizeType` comment on both lines Done >> Source/WebCore/style/StyleBuilderCustom.h:2167 >> + CSSPropertyID newId = CSSProperty::resolveDirectionAwareProperty(CSSPropertyContainIntrinsicBlockSize, style.direction(), style.writingMode()); > > Maybe `auto resolvedID = ` Done
(In reply to Simon Fraser (smfr) from comment #23) > Comment on attachment 457708 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=457708&action=review > > Don't forget to fix RenderStyle::diff() in this or a future patch. > Thanks! We will handle this in the implementing patch:)
Created attachment 457970 [details] Patch
Created attachment 457971 [details] Patch
Committed r293090 (249801@main): <https://commits.webkit.org/249801@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 457971 [details].
Comment on attachment 457971 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457971&action=review > Source/WebCore/css/CSSProperties.json:5327 > + "logical-property-group": { Logical properties are resolved into physical before being applied. So you can use skip-builder:true and then remove the logical BuilderCustom methods. That's what other logical properties do.
*** Bug 204316 has been marked as a duplicate of this bug. ***