Bug 218964 – Web Inspector: Show current properties for font in new Elements sidebar Font panel

• Component: Web Inspector
• Status: RESOLVED FIXED

[Patrick Angle]

Support viewing (inspecting) properties of text in a new sidebar panel of the Elements tab.

<rdar://69794678>

[Patrick Angle]

Created attachment 415326 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Patrick Angle]

Created attachment 415328 [details]
Screenshot of Patch 415326

[Blaze Burg]

Comment on attachment 415326 [details]
Patch

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

> Source/WebCore/ChangeLog:21
> +        - Null implementation for non-Cocoa platforms.

Nit: empty implementation

> Source/JavaScriptCore/inspector/protocol/CSS.json:236
> +            "id": "Font",

Does the protocol Font object correspond to a WebCore::Font? The naming here is tricky, because as I recall, the WebCore concept doesn't map cleanly to something in a @font-face rule. Let's be precise so its easy to distinguish a font face vs font family vs font. If those concepts are visible to the frontend, then it would be a good idea to have a brief explanation of how these concepts are related. This would be useful in the protocol description and/or as part of the ChangeLog.

> Source/JavaScriptCore/inspector/protocol/CSS.json:240
> +                { "name": "name", "type": "string", "description": "The user-facing name of the font." },

nit: prefer fontName to make this easier to disambiguate.

If there is more than one name-like field associated with the font (i.e., the file name, author, etc) then these would be added here.

> Source/JavaScriptCore/inspector/protocol/CSS.json:241
> +                { "name": "availableVariationAxes", "type": "array", "items": { "$ref": "FontVariationAxis" }, "description": "-" }

Nit: I prefer just 'variationAxes'

> Source/JavaScriptCore/inspector/protocol/CSS.json:247
> +            "properties": [

Nit: add description like: "Represents a single font variation axis. Variation axes are only available for foobar". (I don't know if this is actually true, but this is what would go in the description.)

> Source/JavaScriptCore/inspector/protocol/CSS.json:248
> +                { "name": "name", "type": "string", "description": "The name of the variation axis provided by the font." },

Nit: "The name of a single variation axis associated with a Font."

Sidebar: do we have a convention/syntax for cross-referencing protocol types? Should this be 'Font' or 'CSS.Font'?

> Source/JavaScriptCore/inspector/protocol/CSS.json:301
> +            "name": "getComputedPrimaryFontForNode",

I think this should be more generalized so we can add more font-related data for a node in the future. Such as... the ranges for each font (once we figure how to instrument that the right way :))

Maybe: CSS.getFontDataForNode(nodeid) ?

It's straightforward to add more parameters to the result object as more things get exposed.

> Source/JavaScriptCore/inspector/protocol/CSS.json:307
> +                { "name": "primaryFont", "$ref": "Font", "description": "Computed primary font for the specified DOM node." }

I was wondering about this name, but it corresponds to the property of FontCascade, so its good :)

> Source/WebCore/platform/graphics/FontPlatformData.cpp:94
> +    // FIXME: Not implemented yet.

If you haven't already, I recommend you make a generic "[GTK|Win] Web Inspector: add support for Fonts panel" bugs for visibility. Beyond that, it's up to port maintainers to decide when and whether to implement it.

> Source/WebCore/platform/graphics/coretext/FontPlatformDataCoreText.cpp:204
> +        identifier = __builtin_bswap32(identifier);

Uh, does this fix the endianness or something? might be worth a comment.

[Devin Rousso]

Comment on attachment 415326 [details]
Patch

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

This is really great work!  Mostly structural and style (both code and visual) comments :)

>> Source/JavaScriptCore/inspector/protocol/CSS.json:241
>> +                { "name": "availableVariationAxes", "type": "array", "items": { "$ref": "FontVariationAxis" }, "description": "-" }
> 
> Nit: I prefer just 'variationAxes'

either include a real description or drop the property

> Source/JavaScriptCore/inspector/protocol/CSS.json:252
> +                { "name": "minimumValue", "type": "number", "description": "-" },
> +                { "name": "maximumValue", "type": "number", "description": "-" },
> +                { "name": "defaultValue", "type": "number", "description": "-" }

ditto (:241)

>> Source/WebCore/platform/graphics/FontPlatformData.cpp:94
>> +    // FIXME: Not implemented yet.
> 
> If you haven't already, I recommend you make a generic "[GTK|Win] Web Inspector: add support for Fonts panel" bugs for visibility. Beyond that, it's up to port maintainers to decide when and whether to implement it.

Is this a followup?  If so, please create a bug and include the URL here.  If not, please change the comment to something more appropriate.

> Source/WebCore/platform/graphics/FontPlatformData.h:83
> +        FontVariationAxis(String& name, String& tag, int defaultValue, int minimumValue, int maximumValue)

`const String&`

> Source/WebCore/platform/graphics/FontPlatformData.h:93
> +        const String name() const { return m_name; }
> +        const String tag() const { return m_tag; }

`const String&`

> Source/WebCore/platform/graphics/coretext/FontPlatformDataCoreText.cpp:185
> +    

Style: unnecessary space

> Source/WebCore/platform/graphics/coretext/FontPlatformDataCoreText.cpp:193
> +

ditto (:185)

> Source/WebCore/platform/graphics/coretext/FontPlatformDataCoreText.cpp:203
> +        CFNumberGetValue(static_cast<CFNumberRef>(CFDictionaryGetValue(axisVal, kCTFontVariationAxisIdentifierKey)), kCFNumberIntType, &identifier);

Should we care about whether or not these `CFNumberGetValue` actually succeed?  I'd imagine that if it doesn't we'd want to skip the entire item.

>> Source/WebCore/platform/graphics/coretext/FontPlatformDataCoreText.cpp:204
>> +        identifier = __builtin_bswap32(identifier);
> 
> Uh, does this fix the endianness or something? might be worth a comment.

Perhaps instead of doing this you could create a helper function somewhere and share the logic everywhere `kCTFontVariationAxisIdentifierKey` is used (e.g. `defaultVariationValues` in `Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp`)?

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:256
> +localizedStrings["Capitals"] = "Capitals";

please add a key (and/or comment) to all of these new UI strings that gives the context of this being related to fonts and not countries :)

> Source/WebInspectorUI/UserInterface/Base/Setting.js:226
> +    experimentalEnableFontStylesPanel: new WI.Setting("experimental-font-styles-panel", false),

NIT: I generally try to add "Details" or "Navigation" to these names so it's immediately clear where it's expected to be used

> Source/WebInspectorUI/UserInterface/Main.html:694
> +    <script src="Views/FontDetailsPanel.js"></script>
> +    <script src="Views/FontDetailsSidebarPanel.js"></script>

Aside: It's really unfortunate that we have to have this indirection.  It just makes things more complex for no reason AFAIK.  I feel like we don't need it anymore and should remove it.

> Source/WebInspectorUI/UserInterface/Models/Font.js:38
> +        let availableVariationAxes = payload.availableVariationAxes.map((axisPayload) => WI.FontVariationAxis.fromPayload(axisPayload) );

Style: unnecessary space

> Source/WebInspectorUI/UserInterface/Views/ElementsTabContentView.js:37
> +        if (WI.settings.experimentalEnableFontStylesPanel.value)
> +            detailsSidebarPanelConstructors.push(WI.FontDetailsSidebarPanel);

NIT: I feel like this should go after the Computed panel

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.css:28
> +.multi-sidebar.showing-multiple > .sidebar > .panel.details.style-font > .content > .pseudo-classes {
> +    display: none;
> +}

Aside: perhaps we should move this to a more generic CSS file and have it just be `.panel.details:not(.style-rules)` so we don't have to do this for every CSS related panel we may want to add in the future

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:30
> +        super(delegate, "font", "font", WI.UIString("Font"));

Style: please pull out constant literals into separate variables so we know what they are intended to be used for.
```
    const className = "font";
    const identifier = "font";
```

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:34
> +        this._fontProperties = new Map;
> +        this._fontVariationSettings = new Map;
> +        this._fontFeatureSettings = new Map;

Style: I'd add the word `Map` to the end of each of these properties as right now they read like arrays.

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:46
> +        this._fontPreview.value = this.nodeStyles?.computedPrimaryFont?.name || "";

Can we not assume that `nodeStyles` exists since `nodeStylesRefreshed` is only called if there is a valid `WI.DOMNodeStyles`?  I'd also expect that we'd only show this sidebar panel if there is a `computedPrimaryFont` (or at least show a completely different UI, but there should always be a font right?).

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:51
> +        this._fontStretch.value = this._formatSimpleSingleValue(this.fontProperties["font-stretch"], "wdth", "%d%%");

Should we localize "%"?  I dunno if all languages/locales use that symbol.

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:60
> +        // We don't really support these yet.

As in WebKit doesn't support it or Web Inspector doesn't support it?  Is there a bug you can link to for this being added?

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:65
> +        this.fontFeatureSettings.forEach((value, key) => {
> +            featureRows.push(new WI.DetailsSectionSimpleRow(key, value));
> +        });
> +        this._feautreSettingsGroup.rows = featureRows;

We prefer using `for (let [key, value] of this._fontFeatureSettings)` instead of `*.prototype.forEach`.

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:72
> +        this.fontVariationSettings.forEach((value, key) => {
> +            let name = `${value.name} (${key})`;
> +            variationRows.push(new WI.DetailsSectionSimpleRow(name, this._formatVariationValue(value)));
> +        });
> +        this._variationSettingsGroup.rows = variationRows;

ditto (:62)

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:77
> +    get fontProperties()

This only appears to be used by this class, so you should remove it and just use `this._fontProperties` directly instead.

Style: simple `get` can be compacted into a single line at the top of the `Public` section

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:82
> +    get fontFeatureSettings()

ditto (:77)

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:87
> +    get fontVariationSettings()

ditto (:77)

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:94
> +    initialLayout()

please call `super.iniitalLayout()` or include a comment explaining why you are not doing that

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:102
> +        this._fontSize = new WI.DetailsSectionSimpleRow(WI.UIString("Size"));
> +        this._fontStyle = new WI.DetailsSectionSimpleRow(WI.UIString("Style"));
> +        this._fontWeight = new WI.DetailsSectionSimpleRow(WI.UIString("Weight"));
> +        this._fontStretch = new WI.DetailsSectionSimpleRow(WI.UIString("Stretch"));

Style: these should all be suffixed with "Row" so it's clear that they're UI elements and not model objects/values.

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:112
> +        this._fontVariantLigatures = new WI.DetailsSectionSimpleRow(WI.UIString("Ligatures"));
> +        this._fontVariantPosition = new WI.DetailsSectionSimpleRow(WI.UIString("Position"));
> +        this._fontVariantCaps = new WI.DetailsSectionSimpleRow(WI.UIString("Capitals"));
> +        this._fontVariantNumeric = new WI.DetailsSectionSimpleRow(WI.UIString("Numeric"));
> +        this._fontVariantAlternates = new WI.DetailsSectionSimpleRow(WI.UIString("Historical Figures"));
> +        this._fontVariantEastAsian = new WI.DetailsSectionSimpleRow(WI.UIString("East Asian"));

ditto (:99)

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:116
> +        this._feautreSettingsGroup = new WI.DetailsSectionGroup();

`feautre` -> `feature`

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:117
> +        this._feautreSettingsGroup = new WI.DetailsSectionGroup();
> +        this._variationSettingsGroup = new WI.DetailsSectionGroup();

Looking at the screenshot, it wasn't obvious to me that these groups were for feature settings and variation settings respectively.  Rather than have them be groups as part of the "Properties" section, I'd make them into separate "Feature Settings"  and "Variation Settings" groups so that it's clear what they are (and are independently collapsable).

Also, what do we show if the font doesn't have any feature settings or variation settings?  If you keep these as groups, I'd expect the entire group to be hidden.  If you make them into separate sections, I'd expect something like "No Feature Settings"/"No Variation Settings" placeholder text to be shown.

Style: no `()` needed for constructors that don't pass any arguments

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:129
> +        // Data processing some day...
> +        this._updateFontProperties();

Does this need to happen before the `super` call or can it be moved after?  We usually only put things before the `super` call for specific reasons (if so please include a comment why) or when "destroying"/removing (to kinda match how C++ destructors work).

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:153
> +                ["oblique", "oblique 14deg"]

Style: missing comma

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:154
> +            ])

ditto (:153)

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:164
> +            ])

ditto (:153)

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:180
> +            ])

ditto (:153)

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:193
> +    _populateProperty({fontProperties, name, variations, features, keywordComputedReplacements, keywordReplacements}) {

I feel like a lot of the logic in this function and those below could instead be on `WI.Font` (which means that we could test it), as none of it really deals with any UI/DOM.

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:202
> +        variations = variations || [];
> +        features = features || [];
> +        keywordComputedReplacements = keywordComputedReplacements || [];
> +        keywordReplacements = keywordReplacements || new Map;

`||=`

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:220
> +                if (!propertyInformation.variationValues)
> +                    propertyInformation.variationValues = new Map;

`||=`

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:233
> +                if (!propertyInformation.featureValues)
> +                    propertyInformation.featureValues = new Map;

`||=`

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:247
> +        return this.nodeStyles?.effectivePropertyForName(name)?.value || "";

ditto (:46)

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:252
> +        return this.nodeStyles?.computedStyle?.properties.filter((property) => name === property.name )[0]?.value || "";

ditto (:46)

Also, I think you can do
```
    const dontCreateIfMissing = true;
    return this.nodeStyles.computedStyle.propertyForName(name, dontCreateIfMissing)?.value || "";
```
to make this slightly cleaner.

Style: unnecessary space

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:261
> +            cssVariationSettingsRawValue.split(",").forEach((value) => {

ditto (:62)

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:290
> +            this._fontFeatureSettings = featureSettings;

NIT: Rather than have this early-return, you could just invert this to `!==` and move the `for` below inside to make a more uniform control flow (and avoid repeated code).

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:317
> +            return "Normal";

Should this be localized?

Alternatively, should we represent this in terms of a CSS property (i.e. `normal`)?

ditto for similar cases below

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:352
> +        let discretionary;
> +        let historical;
> +        let contextual;

Style: please give these a default value like `null` as they're not always defined in the logic below

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:368
> +        return [common, discretionary, historical, contextual].filter((part) => !!part ).join(", ");

NIT: rather than have variables, construct the array, and then filter it, why not create the array at the beginning and `push` values inside the `if`?
```
    let result = [];

    if (!this._featureIsEnabled(value, "clig", !valueParts.includes("no-common-ligatures")))
        result.push("Disabled Common");
    else
        result.push("Common");

    if (this._featureIsEnabled(value, "dlig", valueParts.includes("discretionary-ligatures")))
        result.push("Discretionary");

    if (this._featureIsEnabled(value, "hlig", valueParts.includes("historical-ligatures")))
        result.push("Historical");

    if (this._featureIsEnabled(value, "calt", valueParts.includes("contextual")))
        result.push("Contextual Alternates");

    return result.join(", ");
```

ditto for similar cases below

Style: unnecessary space

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:413
> +        let figures;
> +        let spacing;
> +        let fractions;
> +        let ordinal;
> +        let slashedZero;

ditto (:350)

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:418
> +        if (this._featureIsEnabled(value, "onum", valueParts.includes("oldstyle-nums")))

Can this (and below) be an `else if` or is this an order of importance (if so then can we reverse the order and use an `else if`)?

ditto for similar cases below

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:439
> +        let result = [figures, spacing, fractions, ordinal, slashedZero].filter((part) => !!part ).join(", ");

Style: unnecessary space

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:461
> +        let forms;
> +        let width;
> +        let ruby;

ditto (:350)

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:490
> +        let result = [forms, width, ruby].filter((part) => !!part ).join(", ");

Style: unnecessary space

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:500
> +        if (value.featureValues?.has(featureTag))

Rather than have to check for the existence of `featureValues`, could we instead just always create `featureValues: new Map` inside `propertyInformation`?

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:511
> +    _hasVariationValue(value, variationTag, mustHaveValue = true)

ditto (:GeneralStyleDetailsSidebarPanel.js:28)

I think `{optional} = {}` would be the clearest as it's default-false and makes it clear what a `true` would mean at the callsite instead of having to wonder "what's this third parameter for?".

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:513
> +        if (value.variationValues?.has(variationTag))

ditto (:500)

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:520
> +        return this.nodeStyles?.computedPrimaryFont?.availableVariationAxes || [];

ditto (:46)

> Source/WebInspectorUI/UserInterface/Views/FontNameDetailsSectionRow.css:28
> +    text-align: center;

This is not our normal style for details sections.  Rather than have the section be called "Font" (which is redundant given that the name of the panel is also "Font"), perhaps we could call this "Identity" and have one row with "Name: ...".  This way you could also add additional info like "URL: ..." (which would show the url of the font file) or "Initiator: ..." (which would be a source code location to where that font file was included) and it would fit the section name.

> Source/WebInspectorUI/UserInterface/Views/FontNameDetailsSectionRow.js:52
> +        this.element.classList.toggle("empty", !this._value && this._value !== 0);

Why the direct comparison to `0`?  Also, `this._value` can never be `0` because `0 || ""` will always be `""`.

> Source/WebInspectorUI/UserInterface/Views/FontNameDetailsSectionRow.js:57
> +    get tooltip()

this appears unused

> Source/WebInspectorUI/UserInterface/Views/GeneralStyleDetailsSidebarPanel.js:28
> +    constructor(identifier, displayName, panelConstructor, allowFiltering = true)

I strongly recommend you use an optional `options = {}` object instead of individually named flags as that is MUCH easier to extend in future patches.
```
    constructor(identifier, displayName, panelConstructor, {hideNewRule, hideFilter, hideClassList} = {})
```
Moreover, we already do something like this with a `get` and CSS (e.g. `get supportsNewRule` and `.supports-new-rule`), both in the other CSS related details sidebar panels and in lots of other view class trees.  I feel like that would be simpler than a constructor parameter.

Also, we normally avoid default-true parameters and instead have flags that disable the default behavior (e.g. `hideFilter` or `disallowFiltering`).

> Source/WebInspectorUI/UserInterface/Views/GeneralStyleDetailsSidebarPanel.js:168
> +            if (InspectorBackend.hasCommand("DOM.resolveNode")) {

We shouldn't include the class list toggle under `_allowFiltering`.  Please add another flag.

> Source/WebInspectorUI/UserInterface/Views/GeneralStyleDetailsSidebarPanel.js:185
> +            let newRuleButton = optionsContainer.createChild("img", "new-rule");

We shouldn't include the new CSS rule button under `_allowFiltering`.  Please add another flag.

Also, as mentioned in my comment on :28 we already have `get supportsNewRule` and `.supports-new-rule` that does this.

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:404
> +        elementsGroup.addSetting(WI.settings.experimentalEnableFontStylesPanel, WI.UIString("Show Font Styles sidebar panel"));

The Font panel doesn't have anything to do with the Styles panel, so this should really just be "Show Font sidebar panel".

> LayoutTests/inspector/css/getComputedPrimaryFontForNode.html:37
> +	        InspectorTest.evaluateInPage(`

Please make this into its own function that lives in the test page (i.e. put it outside of `function test() { ... }`) so that it's properly syntax highlighted and indexable.
```
async function loadFontFace(fontDeclaration, text, eventName) {
    try {
        await document.fonts.load(fontDeclaration, text);
        TestPage.log("PASS: Font should load.");
    } catch {
        TestPage.log("FAIL: Font should load.");
    }
    TestPage.dispatchEventToFrontend(eventName);
}

function test() {
    ...
    InspectorTest.evaluateInPage(`loadFontFace("${fontDeclaration}", "${text}", "${eventName}")`);
}
```

[Patrick Angle]

Comment on attachment 415326 [details]
Patch

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

>>> Source/WebCore/platform/graphics/FontPlatformData.cpp:94
>>> +    // FIXME: Not implemented yet.
>> 
>> If you haven't already, I recommend you make a generic "[GTK|Win] Web Inspector: add support for Fonts panel" bugs for visibility. Beyond that, it's up to port maintainers to decide when and whether to implement it.
> 
> Is this a followup?  If so, please create a bug and include the URL here.  If not, please change the comment to something more appropriate.

This would be a followup in the same way the above empty implantation of `FontPlatformData::familyName()` still is, yes. I'll open the appropriate bug for this and reference it in the comment.

>>> Source/WebCore/platform/graphics/coretext/FontPlatformDataCoreText.cpp:204
>>> +        identifier = __builtin_bswap32(identifier);
>> 
>> Uh, does this fix the endianness or something? might be worth a comment.
> 
> Perhaps instead of doing this you could create a helper function somewhere and share the logic everywhere `kCTFontVariationAxisIdentifierKey` is used (e.g. `defaultVariationValues` in `Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp`)?

Yeah. Both this and `FontCacheCoreText.cpp` should use the same implementation. I'm inclined to use the existing implementation in `FontCacheCoreText.cpp` since it obviously is already battle-tested on some level despite not quite matching what the Cocoa/CoreText folks recommended.

>> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:46
>> +        this._fontPreview.value = this.nodeStyles?.computedPrimaryFont?.name || "";
> 
> Can we not assume that `nodeStyles` exists since `nodeStylesRefreshed` is only called if there is a valid `WI.DOMNodeStyles`?  I'd also expect that we'd only show this sidebar panel if there is a `computedPrimaryFont` (or at least show a completely different UI, but there should always be a font right?).

This is a good point; I need to handle the inevitable remote inspection of a target without the new font support a bit more gracefully.

>> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:51
>> +        this._fontStretch.value = this._formatSimpleSingleValue(this.fontProperties["font-stretch"], "wdth", "%d%%");
> 
> Should we localize "%"?  I dunno if all languages/locales use that symbol.

The answer to this is dependent on the answer to line :317. If we indent to match CSS syntax, we should not localize this, if we intend to "clean up" the representation of the style here, this should absolutely be localized.

>> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:60
>> +        // We don't really support these yet.
> 
> As in WebKit doesn't support it or Web Inspector doesn't support it?  Is there a bug you can link to for this being added?

Neither. This was a development comment for when I hadn't implemented building `fontFeatureSettings` yet. Why I didn't have my usual `TODO: [PCA]` on this comment is beyond me (or why I still didn't see this when reviewing before submitting).

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:128
> +        // Data processing some day...

Making a note to remove this comment as well so I don't forget.

>> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:193
>> +    _populateProperty({fontProperties, name, variations, features, keywordComputedReplacements, keywordReplacements}) {
> 
> I feel like a lot of the logic in this function and those below could instead be on `WI.Font` (which means that we could test it), as none of it really deals with any UI/DOM.

That's an interesting idea; I like it. Should I be concerned that the logic requires not only the `WI.Font`but also to be handed a node's `WI.DOMNodeStyles` when moving it to Font? I do like the idea of moving it out of here in general to create better Model/View separation.

>> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:317
>> +            return "Normal";
> 
> Should this be localized?
> 
> Alternatively, should we represent this in terms of a CSS property (i.e. `normal`)?
> 
> ditto for similar cases below

I'm not sold in either direction... But this function is an example of the wrong answer, where I've accidentally mixed the two.

[Devin Rousso]

Comment on attachment 415326 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/coretext/FontPlatformDataCoreText.cpp:204
>>>> +        identifier = __builtin_bswap32(identifier);
>>> 
>>> Uh, does this fix the endianness or something? might be worth a comment.
>> 
>> Perhaps instead of doing this you could create a helper function somewhere and share the logic everywhere `kCTFontVariationAxisIdentifierKey` is used (e.g. `defaultVariationValues` in `Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp`)?
> 
> Yeah. Both this and `FontCacheCoreText.cpp` should use the same implementation. I'm inclined to use the existing implementation in `FontCacheCoreText.cpp` since it obviously is already battle-tested on some level despite not quite matching what the Cocoa/CoreText folks recommended.

yeah I'd use the existing implementation :)

>>> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:46
>>> +        this._fontPreview.value = this.nodeStyles?.computedPrimaryFont?.name || "";
>> 
>> Can we not assume that `nodeStyles` exists since `nodeStylesRefreshed` is only called if there is a valid `WI.DOMNodeStyles`?  I'd also expect that we'd only show this sidebar panel if there is a `computedPrimaryFont` (or at least show a completely different UI, but there should always be a font right?).
> 
> This is a good point; I need to handle the inevitable remote inspection of a target without the new font support a bit more gracefully.

I think actually you've already done that in `ElementsTabContentView.js` as the sidebar panel won't get added unless the backend supports it :)

>>> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:51
>>> +        this._fontStretch.value = this._formatSimpleSingleValue(this.fontProperties["font-stretch"], "wdth", "%d%%");
>> 
>> Should we localize "%"?  I dunno if all languages/locales use that symbol.
> 
> The answer to this is dependent on the answer to line :317. If we indent to match CSS syntax, we should not localize this, if we intend to "clean up" the representation of the style here, this should absolutely be localized.

My usual approach is to use `WI.unlocalizedUIString` for things that are code (e.g. CSS property names, keyword values, etc.).  I normally localize everything else.

I've been on the fence as to whether to localize concepts (e.g. "WebSocket" or "Canvas"), but I've been told that it's a good idea to localize even those too because some languages that do not use the English alphabet (like Japanese) have a different character set for things like this.

>>> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:193
>>> +    _populateProperty({fontProperties, name, variations, features, keywordComputedReplacements, keywordReplacements}) {
>> 
>> I feel like a lot of the logic in this function and those below could instead be on `WI.Font` (which means that we could test it), as none of it really deals with any UI/DOM.
> 
> That's an interesting idea; I like it. Should I be concerned that the logic requires not only the `WI.Font`but also to be handed a node's `WI.DOMNodeStyles` when moving it to Font? I do like the idea of moving it out of here in general to create better Model/View separation.

Ah yeah grabbing CSS properties.

`WI.DOMNodeStyles` is a model object too, so that's fine :)

FWIW I think it should be a parameter to any necessary functions, not a constructor argument.

[Patrick Angle]

Created attachment 415578 [details]
Patch

[Patrick Angle]

Created attachment 415579 [details]
Screenshot of v2.0-v2.65

[Patrick Angle]

Created attachment 415642 [details]
Patch v2.1 - Win|WinCairo fixes

[Patrick Angle]

Comment on attachment 415642 [details]
Patch v2.1 - Win|WinCairo fixes

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

> Source/WebCore/platform/graphics/coretext/FontPlatformDataCoreText.cpp:200
> +#if PLATFORM(COCOA)
> +
> +Vector<FontPlatformData::FontVariationAxis> FontPlatformData::variationAxes() const
> +{
> +    auto platformFont = ctFont();
> +    if (!platformFont)
> +        return { };
> +    
> +    auto axes = defaultVariationValues(platformFont);
> +    Vector<FontVariationAxis> results;
> +    for (auto& [tag, values] : axes)
> +        results.append(FontPlatformData::FontVariationAxis(values.axisName, String(tag.data(), tag.size()), values.defaultValue, values.minimumValue, values.maximumValue));
> +    
> +    return results;
> +}
> +
> +#endif
> +

I've realized this morning this should be defined in FontPlatformDataCocoa.mm since it is only for Cocoa ports, not all ports using CoreText. This will eliminate the need for the `#if PLATFORM(COCOA)` check around this implementation.

[Patrick Angle]

Created attachment 415647 [details]
Patch v2.2 - Move Cocoa-specific impl to FontPlatformDataCocoa.mm

[Devin Rousso]

Comment on attachment 415647 [details]
Patch v2.2 - Move Cocoa-specific impl to FontPlatformDataCocoa.mm

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

Nice iteration!  This is looking really clean.  Some programatic questions (and Style/NIT).  Would love to see some tests for the `WI.Font` functions added in this patch :)

> Source/JavaScriptCore/inspector/protocol/CSS.json:240
> +                { "name": "fontName", "type": "string", "description": "The user-facing name of the font." },

NIT: Maybe we should call this `displayName`?  The description you wrote makes me think that there's another name that's non-user-facing (e.g. something odd like "Helvetica" vs "hlvtc").

On a more general/unrelated note, what's the difference between `fontName` and something like `fontFamily`?  `font-family` is what's used in CSS, but maybe that's not quite the same thing here?

> Source/JavaScriptCore/inspector/protocol/CSS.json:249
> +                { "name": "name", "type": "string", "description": "The name, generally human-readable, of the variation axis. Some axes may repeat the their tag here instead of providing a human-readable name." },

Should we make this `optional` then so that we don't have duplicated data if the `name` is the same as the `tag`?

> Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:71
> +    auto axes = defaultVariationValues(platformFont);

NIT: I'd inline this.

> Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:74
> +        results.append(FontPlatformData::FontVariationAxis(values.axisName, String(tag.data(), tag.size()), values.defaultValue, values.minimumValue, values.maximumValue));

Aside: I wonder if we should add `std::array` variants for constructing `String` so that this would just work :)

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:46
> +        this._computedPrimaryFont = null;

NIT: `_fontData` to match the command name?

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:342
> +        this._pendingRefreshTask = Promise.all([fetchedMatchedStylesPromise.promise, fetchedInlineStylesPromise.promise, fetchedComputedStylesPromise.promise, fetchedFontDataPromise.promise])
> +        .then(([fetchMatchedStylesResult, fetchInlineStylesResult, fetchComputedStylesResult, fetchedFontDataResult]) => {

NIT: you could reorder this if you wanted to avoid having all these extra unused variables
```
    this._pendingRefreshTask = Promise.all([fetchedComputedStylesPromise.promise, fetchedMatchedStylesPromise.promise, fetchedInlineStylesPromise.promise, fetchedFontDataPromise.promise])
    .then([fetchedComputedStylesResult]) => {
        ...
    });
```

> Source/WebInspectorUI/UserInterface/Models/Font.js:52
> +    calculateFontProperties(domNodeStyle)

Can we add tests for this?

> Source/WebInspectorUI/UserInterface/Models/Font.js:57
> +

Style: unnecessary newline

> Source/WebInspectorUI/UserInterface/Models/Font.js:152
> +                let [tag, value] = axis.trim().replaceAll("\"", "").split(" ");

Is it possible for the tag or value to contain a "?  Rather than replace all ", should we only strip the first and last one?

> Source/WebInspectorUI/UserInterface/Models/Font.js:154
> +                    value = "1";

Why the string `"1"` instead of `true` or even just keeping the `"on"`?

Also, in the case that no value is provided should we wrap the value with parenthesis (e.g. `(on)`) so that there's a distinction between "specified without a value, which implicitly means enabled" and "specified with an explicit enabled value"?

> Source/WebInspectorUI/UserInterface/Models/Font.js:156
> +                    value = "0";

ditto (:154)

> Source/WebInspectorUI/UserInterface/Models/Font.js:164
> +    _populateProperty({resultProperties, style, name, variations, features, keywordComputedReplacements, keywordReplacements})

NIT: I'd move all the required properties out to be positional parameters and leave all the optional properties as part of the last optional-with-default parameter.
```
    _populateProperty(name, style, resultProperties, {variations, features, keywordComputedReplacements, keywordReplacements} = {})
```
This should make hopefully make some of the callsites a bit cleaner and not have to have as many lines.

> Source/WebInspectorUI/UserInterface/Models/Font.js:169
> +    _computeProperty({style, name, variations, features, keywordComputedReplacements, keywordReplacements})

ditto (:164)

> Source/WebInspectorUI/UserInterface/Models/Font.js:171
> +        // TODO: [PCA] Clean up naming of variables throughout.

?

> Source/WebInspectorUI/UserInterface/Views/ElementsTabContentView.js:39
> +            detailsSidebarPanelConstructors.splice(detailsSidebarPanelConstructors.indexOf(WI.ComputedStyleDetailsSidebarPanel) + 1, 0, WI.FontDetailsSidebarPanel);

NIT: Rather than use `indexOf` and `splice`, you could just wait to add the other panels until after this `if`.
```
    let detailsSidebarPanelConstructors = [
        WI.RulesStyleDetailsSidebarPanel,
        WI.ComputedStyleDetailsSidebarPanel,
    ];
    // COMPATIBILITY (iOS 14.0): `CSS.getFontDataForNode` did not exist yet.
    if (WI.settings.experimentalEnableFontDetailsPanel.value && InspectorBackend.hasCommand("CSS.getFontDataForNode"))
        detailsSidebarPanelConstructors.splice(detailsSidebarPanelConstructors.indexOf(WI.ComputedStyleDetailsSidebarPanel) + 1, 0, WI.FontDetailsSidebarPanel);
    detailsSidebarPanelConstructors.push(WI.ChangesDetailsSidebarPanel, WI.DOMNodeDetailsSidebarPanel);
    ...
```

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:44
> +        return 200;

Where did this come from?  Is it even possible for the details sidebar to be this narrow since the `WI.NavigationBar` that lets you switch panels is likely going to be wider than this.

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:56
> +        this._fontSizeRow.value = this._formatSizeValue(this._fontPropertiesMap["font-size"]);

Do these actually work?  AFAIU `_fontPropertiesMap` is a `Map`, so it should use `.get("font-size")` not brackets.

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:72
> +        this._fontAdditionalFeaturesGroup.hidden = !featureRows;

`!featureRows.length`

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:80
> +            let emptyRow = new WI.DetailsSectionRow(WI.UIString("No additional variation axes.", "No additional variation axes. @ Font Details Sidebar", "Message shown when there are no additional variation axes to show."));

Should we have an empty message for `_fontAdditionalFeaturesGroup` if `featureRows` is empty?  Is there a reason that this isn't hidden if empty like `_fontAdditionalFeaturesGroup`?

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:86
> +        super.refresh();

This should go first unless there's a good reason for it to go last (which should also be explained in a comment here).

Also, please pass `significantChange` too.

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:95
> +        // TODO: [PCA] Localization descriptions AND every other review note for this group.

?

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:118
> +        this._fontVariantAlternatesRow = new WI.DetailsSectionSimpleRow(WI.UIString("Historical Figures", "Historical Figures @ Font Details Sidebar Property", "Property title for `font-variant-alternates`."));

Should this be `"Alternates"`?

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:136
> +        // Update the maps of font information before super refreshes the interface.

Why can't we do this at the beginning of `refresh`?  `nodeStylesRefreshed` also appears to be called even if the panel is not attached/visible, so this may do unnecessary work :(

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:137
> +        let fontMap = this.nodeStyles.computedPrimaryFont?.calculateFontProperties(this.nodeStyles);

NIT: should this be `fontProperties` to match the name?

Also, you could have the fallback here to avoid doing a check three times below.
```
    let fontProperties = this.nodeStyles.computedPrimaryFont?.calculateFontProperties(this.nodeStyles) ?? {};
    this._fontPropertiesMap = fontMap.propertiesMap ?? new Map;
    ...
```

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:168
> +

Style: unnecessary newline

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:184
> +        let value = variation.value ? variation.value : variation.defaultValue;

`variation.value || variation.defaultValue`

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:190
> +        let valueParts = property.value.split(" ");

Is it possible for `property.value` to contain a space inside quotes?

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:193
> +        // Only one result should be pushed per group.
> +        let resultGroups = [];

Why not use a `Set`?

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:196
> +            if ((!feature.group || resultGroups.includes(feature.group)) && this._featureIsEnabled(property, feature.tag, valueParts.includes(feature.name))) {

How does `resultGroups` get populated if we don't take this path unless `resultGroups` already includes `feature.group` (or `feature.group` is not defined, meaning that `undefined` would get added to `resultGroups`?  Should this be `!feature.group || !resultGroups.has(feature.group)` instead (i.e. preventing more than one item from the same group)?

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:221
> +            { tag: "dlig", name: "discretionary-ligatures", result: WI.UIString("Discretionary", "Discretionary @ Font Details Sidebar Property Value", "Property value for `font-variant-ligatures: discretionary-ligatures`.") },

Style: We don't add leading or trailing whitespace for object/array literals in JavaScript (we do in JSON files though).

Aside: I wonder if we should be using `WI.unlocalizedString` for `tag` and `name` since they're known values (as opposed to strings only used internally by Web Inspector, such as CSS classes or enums).

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:265
> +    // WI.UIString("XXXXX", "XXXXX @ Font Details Sidebar Property Value", "Property value for `font-variant-alternates: ZZZZZ`.")

?

> Source/WebInspectorUI/UserInterface/Views/GeneralStyleDetailsSidebarPanel.css:137
> +.sidebar > .panel.details.css-style > .content:not(.supports-options) ~ .options-container,

NIT: I wonder if instead of `.supports-options` we should have a `.supports-toggle-class` and then have a combined selector that hides the entire `.options-container` if all three are not supported.
```
    .sidebar > .panel.details.css-style > .content:not(.supports-new-rule) ~ .options-container > .new-rule,
    .sidebar > .panel.details.css-style > .content:not(.supports-toggle-class) ~.options-container > .toggle-class-toggle,
    .sidebar > .panel.details.css-style > .content:not(.has-filter-bar) ~ .options-container > .filter-bar,
    .sidebar > .panel.details.css-style > .content:not(.supports-new-rule):not(.supports-toggle-class):not(.has-filter-bar) ~ .options-container {
        display: none;
    }
```
This way, we don't have one setting/`get` that is effectively an override of the others.

[Patrick Angle]

Comment on attachment 415647 [details]
Patch v2.2 - Move Cocoa-specific impl to FontPlatformDataCocoa.mm

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

Thanks for the feedback; I'll get some tests written up for the new code and take care of the NIT/Style notes as well.

>> Source/JavaScriptCore/inspector/protocol/CSS.json:240
>> +                { "name": "fontName", "type": "string", "description": "The user-facing name of the font." },
> 
> NIT: Maybe we should call this `displayName`?  The description you wrote makes me think that there's another name that's non-user-facing (e.g. something odd like "Helvetica" vs "hlvtc").
> 
> On a more general/unrelated note, what's the difference between `fontName` and something like `fontFamily`?  `font-family` is what's used in CSS, but maybe that's not quite the same thing here?

I'm not opposed to `displayName`.
To the second question, this is technically different from `font-family`. For example when `font-family: -apple-system`, the font's name is actually `.AppleSystemUIFont`. The tests also show cases with WebFonts where the family name does not match the actual font's name. I'll tweak this description to be less ambiguous.

>> Source/JavaScriptCore/inspector/protocol/CSS.json:249
>> +                { "name": "name", "type": "string", "description": "The name, generally human-readable, of the variation axis. Some axes may repeat the their tag here instead of providing a human-readable name." },
> 
> Should we make this `optional` then so that we don't have duplicated data if the `name` is the same as the `tag`?

That would make sense, and then branch to show the tag only in the sidebar if there is no name.

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:46
>> +        this._computedPrimaryFont = null;
> 
> NIT: `_fontData` to match the command name?

I left this as `_computedPrimaryFont` to distinguish it from other properties we will eventually return in the `fontData`, namely the `_usedFontRanges`.

>> Source/WebInspectorUI/UserInterface/Models/Font.js:152
>> +                let [tag, value] = axis.trim().replaceAll("\"", "").split(" ");
> 
> Is it possible for the tag or value to contain a "?  Rather than replace all ", should we only strip the first and last one?

From the OpenType spec:
```
Axis tags must begin with a letter (0x41 to 0x5A, 0x61 to 0x7A) and must use only letters, digits (0x30 to 0x39) or space (0x20).
```
Which does mean the `.split(" ")` could suffer from the issue you call out below with splitting on escaped spaces, but they can't contain a `"`.

>> Source/WebInspectorUI/UserInterface/Models/Font.js:154
>> +                    value = "1";
> 
> Why the string `"1"` instead of `true` or even just keeping the `"on"`?
> 
> Also, in the case that no value is provided should we wrap the value with parenthesis (e.g. `(on)`) so that there's a distinction between "specified without a value, which implicitly means enabled" and "specified with an explicit enabled value"?

From the CSS Fonts Module spec:
```
For boolean features, a value of 1 enables the feature. For non-boolean features, a value of 1 or greater enables the feature and indicates the feature selection index. A value of on is synonymous with 1 and off is synonymous with 0. If the value is omitted, a value of 1 is assumed.
```
My goal here is to normalize the input so that an eventual editing interface doesn't need to. I can't think of a reason we would need to distinguish between explicitly enabled and implicitly enabled here.

>> Source/WebInspectorUI/UserInterface/Models/Font.js:171
>> +        // TODO: [PCA] Clean up naming of variables throughout.
> 
> ?

Oh dear...

>> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:80
>> +            let emptyRow = new WI.DetailsSectionRow(WI.UIString("No additional variation axes.", "No additional variation axes. @ Font Details Sidebar", "Message shown when there are no additional variation axes to show."));
> 
> Should we have an empty message for `_fontAdditionalFeaturesGroup` if `featureRows` is empty?  Is there a reason that this isn't hidden if empty like `_fontAdditionalFeaturesGroup`?

The features are shown as part of a group of standard features, so there will still be items in that section, whereas here if there are no additional variation axes, this entire group will be empty.

>> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:95
>> +        // TODO: [PCA] Localization descriptions AND every other review note for this group.
> 
> ?

Oh dear... again.

>> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:118
>> +        this._fontVariantAlternatesRow = new WI.DetailsSectionSimpleRow(WI.UIString("Historical Figures", "Historical Figures @ Font Details Sidebar Property", "Property title for `font-variant-alternates`."));
> 
> Should this be `"Alternates"`?

Arguably yes, but Historical Figures is the only alternates right now.

>> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:190
>> +        let valueParts = property.value.split(" ");
> 
> Is it possible for `property.value` to contain a space inside quotes?

Not with any of the the current properties, but that feels like a good thing to proactively handle anyways.

>> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:196
>> +            if ((!feature.group || resultGroups.includes(feature.group)) && this._featureIsEnabled(property, feature.tag, valueParts.includes(feature.name))) {
> 
> How does `resultGroups` get populated if we don't take this path unless `resultGroups` already includes `feature.group` (or `feature.group` is not defined, meaning that `undefined` would get added to `resultGroups`?  Should this be `!feature.group || !resultGroups.has(feature.group)` instead (i.e. preventing more than one item from the same group)?

Yes. The intention here is to prevent more than one item from the same group.

>> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:265
>> +    // WI.UIString("XXXXX", "XXXXX @ Font Details Sidebar Property Value", "Property value for `font-variant-alternates: ZZZZZ`.")
> 
> ?

Now I just feel sloppy...

>> Source/WebInspectorUI/UserInterface/Views/GeneralStyleDetailsSidebarPanel.css:137
>> +.sidebar > .panel.details.css-style > .content:not(.supports-options) ~ .options-container,
> 
> NIT: I wonder if instead of `.supports-options` we should have a `.supports-toggle-class` and then have a combined selector that hides the entire `.options-container` if all three are not supported.
> ```
>     .sidebar > .panel.details.css-style > .content:not(.supports-new-rule) ~ .options-container > .new-rule,
>     .sidebar > .panel.details.css-style > .content:not(.supports-toggle-class) ~.options-container > .toggle-class-toggle,
>     .sidebar > .panel.details.css-style > .content:not(.has-filter-bar) ~ .options-container > .filter-bar,
>     .sidebar > .panel.details.css-style > .content:not(.supports-new-rule):not(.supports-toggle-class):not(.has-filter-bar) ~ .options-container {
>         display: none;
>     }
> ```
> This way, we don't have one setting/`get` that is effectively an override of the others.

+1, Good idea.

[Patrick Angle]

Created attachment 415797 [details]
Patch v2.5 - Added tests for WI.Font._calculateProperties

[Devin Rousso]

Comment on attachment 415797 [details]
Patch v2.5 - Added tests for WI.Font._calculateProperties

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

r=mews with a few more fixes/comments.  Also, thanks for adding the model object test!  Really great work :)

> Source/WebInspectorUI/ChangeLog:91
> +        * Localizations/en.lproj/localizedStrings.js:

i think this a repeat of the above

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:577
> +        resultVariationAxes->addItem(axis);

`WTFMove(axis)`

> Source/WebInspectorUI/UserInterface/Models/Font.js:140
> +                let [tag, value] = axis.match(/[^\s"']+|["']([^"']*)["']/g);

This regex is repeated multiple times.  Should we save it to a static like `WI.Font.SettingPattern = /[^\s"']+|["']([^"']*)["']/g`?

> Source/WebInspectorUI/UserInterface/Models/Font.js:190
> +            // Only truly null for undefined values should be ignored. `0` is a valid value.

Did you mean s/for/or?

> Source/WebInspectorUI/UserInterface/Models/Font.js:191
> +            if (featureSettingValue != null) {

If you want to include `0` I'd suggest one of these instead of using abstract equality:
 - if valid `featureSettingValue` are always a number, you could use `!isNaN(featureSettingValue)`
 - if valid `featureSettingValue` can be anything other than `null`/`undefined`, then I'd `featureSettingValue || featureSettingValue === 0`
We should avoid abstract equality wherever possible.

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:42
> +    get supportsToggleClass()

NIT: I realize I suggested the name (so, my apologies), but I'd probably put "CSS" in there somewhere (e.g. `supportsToggleCSSClass`) so it's clear it's not talking about JavaScript `class` 😅

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:171
> +        let value = variation.value ?? variation.defaultValue;

Using `??` over `""` means that if `variation.value` is an empty string (`""`) it will be used instead of the `variation.defaultValue`.  Is that what you want?  (btw along those lines using `??` prevents `variation.defaultValue` from being used if `variation.value` is `0`, so if the answer to my question is "no" then maybe this needs some special logic instead of a logical operator)

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:208
> +            {tag: WI.unlocalizedString("dlig"), name: WI.unlocalizedString("discretionary-ligatures"), result: WI.UIString("Discretionary", "Discretionary @ Font Details Sidebar Property Value", "Property value for `font-variant-ligatures: discretionary-ligatures`.")},

Style: These lines are getting a bit long, so maybe we should split the properties onto multiple lines for readability?
```
    let otherResults = this._formatSimpleFeatureValues(property, [
        {
            tag: WI.unlocalizedString("dlig"),
            name: WI.unlocalizedString("discretionary-ligatures"),
            result: WI.UIString("Discretionary", "Discretionary @ Font Details Sidebar Property Value", "Property value for `font-variant-ligatures: discretionary-ligatures`."),
        },
        {
            tag: WI.unlocalizedString("hlig"),
            name: WI.unlocalizedString("historical-ligatures"),
            result: WI.UIString("Historical", "Historical @ Font Details Sidebar Property Value", "Property value for `font-variant-ligatures: historical-ligatures`."),
        },
        {
            tag: WI.unlocalizedString("calt"),
            name: WI.unlocalizedString("contextual"),
            result: WI.UIString("Contextual Alternates", "Contextual Alternates @ Font Details Sidebar Property Value", "Property value for `font-variant-ligatures: contextual`."),
        },
    ]);
```

ditto below

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:277
> +        if (property.features?.has(featureTag))
> +            return !!property.features.get(featureTag);

NIT: Out of curiosity, is the value of `property.features.get(featureTag)` ever possible to be `undefined`?  If not, you could just use that as a replacement for `property.features?.has(featureTag)` to avoid the double-lookup.  That may be more prone to accidental breakage tho (although we try to avoid using `undefined` wherever possible), so what you have is probably fine.

> Source/WebInspectorUI/UserInterface/Views/FontDetailsSidebarPanel.js:30
> +        const allowFiltering = false;

This doesn't exist anymore in `WI.GeneralStyleDetailsSidebarPanel`.

> Source/WebInspectorUI/UserInterface/Views/StyleDetailsPanel.js:-109
> -    filterDidChange(filterBar)

Why is this being removed?

[Patrick Angle]

Comment on attachment 415797 [details]
Patch v2.5 - Added tests for WI.Font._calculateProperties

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

>> Source/WebInspectorUI/UserInterface/Models/Font.js:190
>> +            // Only truly null for undefined values should be ignored. `0` is a valid value.
> 
> Did you mean s/for/or?

I meant `or`, but given your note below on avoiding abstract equality, I don't think this comment will be necessary any more.

>> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:171
>> +        let value = variation.value ?? variation.defaultValue;
> 
> Using `??` over `""` means that if `variation.value` is an empty string (`""`) it will be used instead of the `variation.defaultValue`.  Is that what you want?  (btw along those lines using `??` prevents `variation.defaultValue` from being used if `variation.value` is `0`, so if the answer to my question is "no" then maybe this needs some special logic instead of a logical operator)

For the first occurrence of `""` in your comment did you mean `||`? If so, the intent is if `variation.value` is not present to use the default value, otherwise use the value. Zero is a valid value for `variation.value`, so I don't want to use `variation.defaultValue` in its place leading me to use `??`.

>> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:277
>> +            return !!property.features.get(featureTag);
> 
> NIT: Out of curiosity, is the value of `property.features.get(featureTag)` ever possible to be `undefined`?  If not, you could just use that as a replacement for `property.features?.has(featureTag)` to avoid the double-lookup.  That may be more prone to accidental breakage tho (although we try to avoid using `undefined` wherever possible), so what you have is probably fine.

I don't define a `features` property if there are no explicit features, which is why this check is here. This is different from the case of variation axes where even without a value there may still be information like default value and min/max.

>> Source/WebInspectorUI/UserInterface/Views/StyleDetailsPanel.js:-109
>> -    filterDidChange(filterBar)
> 
> Why is this being removed?

GeneralStyleDetailsSidebarPanel.js:235 checks for the existence of this function in order to determine the filter bar's visibility. An empty implementation here meant that all `WI.StyleDetailsPanel` subclasses would have filter bars, even if there was no actual implementation. An alternative to removing this would be to add another public function to enable showing the filter bar.

[Devin Rousso]

Comment on attachment 415797 [details]
Patch v2.5 - Added tests for WI.Font._calculateProperties

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

>>> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:171
>>> +        let value = variation.value ?? variation.defaultValue;
>> 
>> Using `??` over `""` means that if `variation.value` is an empty string (`""`) it will be used instead of the `variation.defaultValue`.  Is that what you want?  (btw along those lines using `??` prevents `variation.defaultValue` from being used if `variation.value` is `0`, so if the answer to my question is "no" then maybe this needs some special logic instead of a logical operator)
> 
> For the first occurrence of `""` in your comment did you mean `||`? If so, the intent is if `variation.value` is not present to use the default value, otherwise use the value. Zero is a valid value for `variation.value`, so I don't want to use `variation.defaultValue` in its place leading me to use `??`.

Yep totally meant `||` my bad 😅

Ultimately, I think the best approach is to use the same logic you end up using on Font.js:191 so that it's consistent.

>>> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:277
>>> +            return !!property.features.get(featureTag);
>> 
>> NIT: Out of curiosity, is the value of `property.features.get(featureTag)` ever possible to be `undefined`?  If not, you could just use that as a replacement for `property.features?.has(featureTag)` to avoid the double-lookup.  That may be more prone to accidental breakage tho (although we try to avoid using `undefined` wherever possible), so what you have is probably fine.
> 
> I don't define a `features` property if there are no explicit features, which is why this check is here. This is different from the case of variation axes where even without a value there may still be information like default value and min/max.

Err, I meant "is it possible for `property.features` to have an entry where the value is `undefined` (e.g. `property.features.set(featureTag, undefined)` but obviously not using the literal `undefined` it'd be inside a variable)?".

>>> Source/WebInspectorUI/UserInterface/Views/StyleDetailsPanel.js:-109
>>> -    filterDidChange(filterBar)
>> 
>> Why is this being removed?
> 
> GeneralStyleDetailsSidebarPanel.js:235 checks for the existence of this function in order to determine the filter bar's visibility. An empty implementation here meant that all `WI.StyleDetailsPanel` subclasses would have filter bars, even if there was no actual implementation. An alternative to removing this would be to add another public function to enable showing the filter bar.

Oh interesting.  Looking at `GeneralStyleDetailsSidebarPanel.js` I do see what you're describing, but I also see that it's used unconditionally inside `initialLayout`.  If you want to have the existence of `filterDidChange` on the subclass be the indicator as to whether that subclass supports filtering, you might want to also wrap its usage inside `initialLayout` (which would likely end up as the `WI.FilterBar` not even being created/added).

[Patrick Angle]

Comment on attachment 415797 [details]
Patch v2.5 - Added tests for WI.Font._calculateProperties

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

>>>> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:277
>>>> +            return !!property.features.get(featureTag);
>>> 
>>> NIT: Out of curiosity, is the value of `property.features.get(featureTag)` ever possible to be `undefined`?  If not, you could just use that as a replacement for `property.features?.has(featureTag)` to avoid the double-lookup.  That may be more prone to accidental breakage tho (although we try to avoid using `undefined` wherever possible), so what you have is probably fine.
>> 
>> I don't define a `features` property if there are no explicit features, which is why this check is here. This is different from the case of variation axes where even without a value there may still be information like default value and min/max.
> 
> Err, I meant "is it possible for `property.features` to have an entry where the value is `undefined` (e.g. `property.features.set(featureTag, undefined)` but obviously not using the literal `undefined` it'd be inside a variable)?".

Oh. I think I understand the question now. The value should never be undefined but can be 0 (this is the check from Font.js:191 to only add the value if it exists and the check from Font.js:142 to make sure implicitly enabled features have an assigned value of 1). In this situation if there is no entry for the tag we fall back to the `tagNotPresentCondition`. If there is a value for the tag we return the boolean equivalent of the value (false for 0, true for all others). Given that, this can become
```
let featureValue = property.features?.get(featureTag);
if (featureValue || featureValue === 0)
    return !!featureValue;
return tagNotPresentCondition;
```
which provides a nice symmetry with the other checks against the feature value.

[Patrick Angle]

Created attachment 415880 [details]
Patch v2.6 - Rebase, review notes

[EWS]

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

[Patrick Angle]

Created attachment 415894 [details]
Patch v2.65 - Added Review By to changelogs

[EWS]

Committed r270637: <https://trac.webkit.org/changeset/270637>

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