Bug 230337 - Parsing support for font-palette-values
Summary: Parsing support for font-palette-values
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks: 230446 230447 230586
  Show dependency treegraph
 
Reported: 2021-09-16 02:11 PDT by Myles C. Maxfield
Modified: 2021-09-21 17:10 PDT (History)
21 users (show)

See Also:


Attachments
WIP (51.36 KB, patch)
2021-09-16 02:11 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (53.51 KB, patch)
2021-09-16 15:34 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (80.51 KB, patch)
2021-09-16 23:11 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (87.33 KB, patch)
2021-09-17 02:17 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (87.31 KB, patch)
2021-09-18 00:28 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (87.79 KB, patch)
2021-09-18 02:04 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (83.38 KB, patch)
2021-09-20 15:58 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (92.95 KB, patch)
2021-09-20 17:02 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (92.52 KB, patch)
2021-09-20 17:06 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (93.93 KB, patch)
2021-09-20 22:40 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (91.54 KB, patch)
2021-09-20 23:32 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (90.20 KB, patch)
2021-09-20 23:33 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (89.81 KB, patch)
2021-09-20 23:34 PDT, Myles C. Maxfield
koivisto: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2021-09-16 02:11:04 PDT
Parsing support for font-palette-values
Comment 1 Myles C. Maxfield 2021-09-16 02:11:59 PDT
Created attachment 438329 [details]
WIP
Comment 2 Myles C. Maxfield 2021-09-16 15:34:28 PDT
Created attachment 438407 [details]
WIP
Comment 3 Myles C. Maxfield 2021-09-16 16:05:26 PDT
I have to put it behind a guard, and make CSSFontPaletteValuesRule match https://drafts.csswg.org/css-fonts/#om-fontpalettevalues
Comment 4 Myles C. Maxfield 2021-09-16 23:11:09 PDT
Created attachment 438445 [details]
Patch
Comment 5 EWS Watchlist 2021-09-16 23:12:19 PDT
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
Comment 6 Myles C. Maxfield 2021-09-17 02:17:13 PDT
Created attachment 438456 [details]
Patch
Comment 7 Myles C. Maxfield 2021-09-18 00:28:07 PDT
Created attachment 438551 [details]
Patch
Comment 8 Myles C. Maxfield 2021-09-18 02:04:49 PDT
Created attachment 438555 [details]
Patch
Comment 9 Antti Koivisto 2021-09-19 05:40:40 PDT
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?
Comment 10 Antti Koivisto 2021-09-19 05:45:02 PDT
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.
Comment 11 Myles C. Maxfield 2021-09-20 15:58:07 PDT
Created attachment 438744 [details]
Patch
Comment 12 Myles C. Maxfield 2021-09-20 17:02:25 PDT
Created attachment 438754 [details]
Patch
Comment 13 Myles C. Maxfield 2021-09-20 17:06:10 PDT
Created attachment 438756 [details]
Patch
Comment 14 Joseph Pecoraro 2021-09-20 19:04:08 PDT
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).
Comment 15 Antti Koivisto 2021-09-20 21:44:51 PDT
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?
Comment 16 Myles C. Maxfield 2021-09-20 22:40:44 PDT
Created attachment 438778 [details]
Patch
Comment 17 Myles C. Maxfield 2021-09-20 23:16:01 PDT
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
Comment 18 Myles C. Maxfield 2021-09-20 23:32:15 PDT
Created attachment 438784 [details]
Patch
Comment 19 Myles C. Maxfield 2021-09-20 23:33:26 PDT
Created attachment 438785 [details]
Patch
Comment 20 Myles C. Maxfield 2021-09-20 23:34:23 PDT
Created attachment 438786 [details]
Patch
Comment 21 Radar WebKit Bug Importer 2021-09-21 01:19:44 PDT
<rdar://problem/83343696>
Comment 22 Myles C. Maxfield 2021-09-21 01:24:46 PDT
Committed r282806 (241938@main): <https://commits.webkit.org/241938@main>