Bug 230337 – Parsing support for font-palette-values

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Parsing support for font-palette-values

[Myles C. Maxfield]

Created attachment 438329 [details]
WIP

[Myles C. Maxfield]

Created attachment 438407 [details]
WIP

[Myles C. Maxfield]

I have to put it behind a guard, and make CSSFontPaletteValuesRule match https://drafts.csswg.org/css-fonts/#om-fontpalettevalues

[Myles C. Maxfield]

Created attachment 438445 [details]
Patch

[EWS Watchlist]

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

[Myles C. Maxfield]

Created attachment 438456 [details]
Patch

[Myles C. Maxfield]

Created attachment 438551 [details]
Patch

[Myles C. Maxfield]

Created attachment 438555 [details]
Patch

[Antti Koivisto]

Comment on attachment 438555 [details]
Patch

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

> Source/WebCore/css/CSSRule.h:61
> +        MARGIN_RULE = 9,
> +        NAMESPACE_RULE = 10,
> +        COUNTER_STYLE_RULE = 11,
> +        SUPPORTS_RULE = 12,
> +        DOCUMENT_RULE = 13,
> +        FONT_FEATURE_VALUES_RULE = 14,
> +        VIEWPORT_RULE = 15,
> +        REGION_STYLE_RULE = 16,
> +        CUSTOM_MEDIA_RULE = 17,
> +        PROPERTY_RULE = 18,
> +        FONT_PALETTE_VALUES_RULE = 19, // https://github.com/w3c/csswg-drafts/issues/6623
> +        LAYER_RULE = 20

Why are you you adding constants for things we don't support?

[Antti Koivisto]

Comment on attachment 438555 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Parsing support for font-palette-values
> +        https://bugs.webkit.org/show_bug.cgi?id=230337
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        There's nothing particularly interesting here - it's just support for another at-rule.
> +        I've implemented what's in the spec right now.

You could point to the spec.

> Source/WebCore/ChangeLog:18
> +        Also, some of our StyleRules had the wrong web-exposed Type constant, so I fixed that too, because
> +        this patch adds a new StyleRule and it needs to have the correct Type.

Let's not do any drive-by fixing here.

[Myles C. Maxfield]

Created attachment 438744 [details]
Patch

[Myles C. Maxfield]

Created attachment 438754 [details]
Patch

[Myles C. Maxfield]

Created attachment 438756 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 438756 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4055
> +        /* Unimplemented @font-palette-values properties */

Weird comment style, but I see it is not unusual for this file.

> Source/WebCore/css/CSSFontPaletteValuesRule.cpp:153
> +    builder.append("}");

"}" => '}'?

> Source/WebCore/css/CSSRule.cpp:4
> + * Copyright (C) 2002 - 2021 Apple Inc. All rights reserved.

I think there are normally no spaces around the hyphen.

> Source/WebCore/css/CSSValue.h:3
> + * Copyright (C) 2004 - 2021 Apple Inc. All rights reserved.

Same here.

> Source/WebCore/css/StyleRuleType.h:2
> + * Copyright (C) 2019 - 2021 Apple Inc. All rights reserved.

Same here (and a bunch of others).

[Antti Koivisto]

Comment on attachment 438756 [details]
Patch

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

> Source/WebCore/css/CSSFontPaletteValuesRule.cpp:75
> +
> +void CSSFontPaletteValuesRule::setFontFamily(String&& fontFamily)
> +{
> +    m_fontPaletteValuesRule->setFontFamily(fontFamily);
> +}

This needs CSSStyleSheet::RuleMutationScope. See CSSKeyframesRule::setName for example.

If it is important to have a mutable CSSOM API then it needs to be tested also in shared stylesheet case (in WPT too). For example, use the same exact stylesheet in two different shadow roots, mutate one and check the other one doesn't mutate too.

> Source/WebCore/css/CSSFontPaletteValuesRule.cpp:77
> +void CSSFontPaletteValuesRule::setBasePalette(String&& basePalette)

Same thing here.

> Source/WebCore/css/CSSFontPaletteValuesRule.cpp:99
> +void CSSFontPaletteValuesRule::setFromMapLike(unsigned key, String&& value)

And here.

> Source/WebCore/css/CSSFontPaletteValuesRule.idl:27
> +    maplike<unsigned long, CSSOMString>;
> +    attribute CSSOMString fontFamily;
> +    attribute CSSOMString basePalette;

Why do people keep adding mutable things to CSSOM? What is this stuff good for?

[Myles C. Maxfield]

Created attachment 438778 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 438756 [details]
Patch

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

>> Source/WebCore/css/CSSFontPaletteValuesRule.idl:27
>> +    attribute CSSOMString basePalette;
> 
> Why do people keep adding mutable things to CSSOM? What is this stuff good for?

I don't think it's important. https://github.com/w3c/csswg-drafts/issues/6645

[Myles C. Maxfield]

Created attachment 438784 [details]
Patch

[Myles C. Maxfield]

Created attachment 438785 [details]
Patch

[Myles C. Maxfield]

Created attachment 438786 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/83343696>

[Myles C. Maxfield]

Committed r282806 (241938@main): <https://commits.webkit.org/241938@main>