Bug 234430 - [css-grid] Fix grid shorthand expansion of initial values
Summary: [css-grid] Fix grid shorthand expansion of initial values
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zsun
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-17 02:58 PST by zsun
Modified: 2022-01-25 01:58 PST (History)
13 users (show)

See Also:


Attachments
Patch (35.63 KB, patch)
2021-12-17 03:32 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (50.26 KB, patch)
2021-12-17 05:32 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (51.45 KB, patch)
2022-01-14 02:40 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (51.45 KB, patch)
2022-01-14 02:42 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (50.23 KB, patch)
2022-01-17 01:12 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (50.23 KB, patch)
2022-01-17 01:12 PST, zsun
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zsun 2021-12-17 02:58:12 PST
We need to  add InitialValue support for 6 grid properties when parsing the grid shorthand. At the moment, it just adds a CSSInitial.

Affected WPT test -

imported/w3c/web-platform-tests/css/css-grid/parsing/grid-shorthand.html
Comment 1 zsun 2021-12-17 03:32:00 PST
Created attachment 447446 [details]
Patch
Comment 2 zsun 2021-12-17 05:32:24 PST
Created attachment 447449 [details]
Patch
Comment 3 Radar WebKit Bug Importer 2021-12-24 02:59:17 PST
<rdar://problem/86880897>
Comment 4 zsun 2022-01-10 06:19:00 PST
Can I have a review on this patch please? Many thanks!
Comment 5 Sergio Villar Senin 2022-01-12 03:47:20 PST
Comment on attachment 447449 [details]
Patch

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

> Source/WebCore/style/StyleBuilderConverter.h:1166
> +        ASSERT(downcast<CSSPrimitiveValue>(value).isValueID());

A bit weird to have a if for just an ASSERT. I'd better do

ASSERT(!is<CSSPrimitiveValue>(value) || downcast<CSSPrimitiveValue>(value).isValueID());

> Source/WebCore/style/StyleBuilderConverter.h:1171
>          return RenderStyle::initialGridAutoFlow();

Seems like you have to indent this return

> Source/WebCore/style/StyleBuilderConverter.h:1175
> +    auto* second = downcast<CSSPrimitiveValue>(is<CSSValueList>(value) && downcast<CSSValueList>(value).length() == 2 ? downcast<CSSValueList>(value).item(1) : nullptr);

Looks like we could store is<CSSValueList>(value) in a boolean since it's also used in the if block above.
Comment 6 zsun 2022-01-14 02:40:19 PST
Created attachment 449154 [details]
Patch
Comment 7 zsun 2022-01-14 02:42:08 PST
Created attachment 449155 [details]
Patch
Comment 8 Sergio Villar Senin 2022-01-17 00:59:42 PST
Comment on attachment 449155 [details]
Patch

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

> Source/cmake/OptionsGTK.cmake:65
> +#WEBKIT_OPTION_DEFINE(USE_JPEGXL "Whether to enable support for JPEG-XL images." PUBLIC ${ENABLE_EXPERIMENTAL_FEATURES})

I think you mistakenly changed this
Comment 9 zsun 2022-01-17 01:12:00 PST
Created attachment 449316 [details]
Patch
Comment 10 zsun 2022-01-17 01:12:57 PST
Created attachment 449317 [details]
Patch
Comment 11 EWS 2022-01-25 01:58:52 PST
Committed r288544 (246376@main): <https://commits.webkit.org/246376@main>

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