Bug 230065 – Web Inspector: Add a swatch for align-content

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Add a swatch for "align-content" CSS property. Clicking on it should display a small popover with icons for most common values, such as "start", "end", "center", "space-between" and etc.

[Radar WebKit Bug Importer]

<rdar://problem/82891361>

[Nikita Vasilyev]

Created attachment 442423 [details]
Patch

[Nikita Vasilyev]

Created attachment 442424 [details]
[Image] align-content popover

[Nikita Vasilyev]

Comment on attachment 442423 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:327
> -        popover.present(bounds.pad(2), [WI.RectEdge.MIN_X]);
> +        popover.present(bounds.pad(2), [WI.RectEdge.MIN_Y]);

I did some digging on why we try to display the popover to the left of the anchor. It's been like this at least since 2014, possibly since the 1st inline swatch was introduced: https://trac.webkit.org/browser/webkit/trunk/Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js?rev=164543#L710.

I believe the better default for all popovers is to be positioned above the anchor. It matches macOS more closely and it doesn't cover CSS property names in the style editor.

[Nikita Vasilyev]

Created attachment 442427 [details]
[HTML] Align-content flexbox

An HTML page with a few align-content boxes.

[Patrick Angle]

Comment on attachment 442423 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:10
> +        start, center, end, space-between, space-around, space-evenly, and stretch.

I really think we probably want to determine the writing direction and apply it's same effect to the visual representations for each of these values. E.g. `start` may be left-aligned in a horizontal layout in a LTR language, but would be right-aligned (mirror the icon on the X-axis) for an RTL element. Alignment also isn't always in the y-axis, where I would expect the icons to be rotated -90deg so that `start` is aligned to the top, for example. I don't think any of the icons need to be redrawn for these states, just mirrored/rotated.

> Source/WebInspectorUI/ChangeLog:21
> +        * UserInterface/Views/AlignContentEditor.css: Added.

Can we name this something like `AlignmentInlineSwatchEditor.css` to make clear it both related to inline swatches and makes it clearer that we will reuse these styles for other alignment properties like `justify-content`, `align-items`, `justify-items`? This kinda applies throughout in the naming of things.

> Source/WebInspectorUI/UserInterface/Images/AlignContentCenter.svg:1
> +<!-- Copyright © 2021 Apple Inc. All rights reserved. -->

Text encoding issue here. Same for all the other new SVGs as well.

> Source/WebInspectorUI/UserInterface/Views/AlignContentEditor.css:39
> +    margin-left: 0;

Do we really want `margin-inline-start` here?

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.css:109
> +.inline-swatch.align-content > span {
> +    background-size: 9px 9px;
> +    background-repeat: no-repeat;
> +}

Why doesn't the current `inline-swatch.image > span` rule get us this?

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.css:129
> +    .inline-swatch.align-content > span {

Why invert the span and not the svg?

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:197
> +                // TODO: Display a glyph for an unrecognized value.

I think we will want an actual state for values we haven't drawn an icon for before this can land.

>> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:327
>> +        popover.present(bounds.pad(2), [WI.RectEdge.MIN_Y]);
> 
> I did some digging on why we try to display the popover to the left of the anchor. It's been like this at least since 2014, possibly since the 1st inline swatch was introduced: https://trac.webkit.org/browser/webkit/trunk/Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js?rev=164543#L710.
> 
> I believe the better default for all popovers is to be positioned above the anchor. It matches macOS more closely and it doesn't cover CSS property names in the style editor.

I agree that displaying above/below is perhaps better, since it reduces the opportunity for the popover to cover some part of the property the author is working with, however I'd argue in the direction of presenting below the swatch by default, since it is more likely that the values the author is most concerned with is higher in the Styles panel, and if we display the popover below by default there won't be an awkward transition between the first few items probably presenting the popover below the swatch anyways (since there won't be enough space above the swatch for things like color) and then transitioning a few items down to the popover fitting above.

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:376
> +    _updateAlignContentPopover()

Many of the other editors we display in popovers are implemented as separate classes, and I kinda feel like we should do the same here. The editor is likely to grow in complexity to handle the other alignment properties, for example, and this class is already quite complicated - it would make it easier to iterate on and understand IMO if it were implemented in it's own class, like the ColorPicker, BoxShadowEditor, etc. are.

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:385
> +        let values = ["start", "center", "end", "space-between", "space-around", "space-evenly", "stretch"];

Nit: `const`

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:557
> +        let valueToFileName = {

Nit: `const`

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:567
> +        return glyphIdentifier ? `Images/${glyphIdentifier}` : "";

Should probably return null instead of an empty string to make it clearer that there there is no glyph path to satisfy the value.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:850
> +        let text = tokens.map((token) => token.value).join("");
> +        let swatch = this._createInlineSwatch(WI.InlineSwatch.Type.AlignContent, tokens, text);
> +        return [swatch];

This won't work for multi-token values, like `first baseline` or `unsafe center`, right? This should probably be made to work similar to `_addColorTokens` above so that the known part gets tokenized.

[Nikita Vasilyev]

Comment on attachment 442423 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Images/AlignContentCenter.svg:1
>> +<!-- Copyright © 2021 Apple Inc. All rights reserved. -->
> 
> Text encoding issue here. Same for all the other new SVGs as well.

The code review tool doesn't show unicode properly. I copy/pasted this line with  ©, that is used in most (all?) our SVG files.

[Devin Rousso]

Comment on attachment 442423 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/AlignContentEditor.css:30
> +    background-color: var(--background-color-content);

I feel like this also needs some sort of `border-color` to help clarify where the parts of the glyph are relative to the box as a whole.  IMO the glyph white background color is not a subtle enough distinction from the lightgrey popover background color.

> Source/WebInspectorUI/UserInterface/Views/AlignContentEditor.css:33
> +.align-content-editor .glyph {

Why not merge this with the above?

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:258
> -            popover.present(bounds.pad(2), [WI.RectEdge.MIN_X]);
> +            popover.present(bounds.pad(2), [WI.RectEdge.MIN_Y]);

This should be a separate patch.  It doesn't have anything to do with visualizing/editing alignment values.

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:387
> +            let glyphElement = WI.ImageUtilities.useSVGSymbol(this._getAlignContentGlyphPath(value), "glyph", value);

please make sure that each glyph has a `title` so that it's possible for a developer to hover to find out what the symbol represents if it's not immediately clear to them

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:389
> +            if (value === this._value)
> +                glyphElement.classList.add("selected");

NIT: `glyphElement.classList.toggle("selected", value === this._value)`

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:558
> +            "start": "AlignContentStart.svg",

This kinda strikes me as odd for some reason 🤔 Do we have to use `WI.ImageUtilities.useSVGSymbol` or can we use CSS `background-image` instead?  If so, we could just set the CSS `class` based on the value and then have a declarative list in CSS of what each icon should be.

NIT: I feel like we should include the `Images/` in each of these paths so it's explicitly clear where they come from (e.g. a hardcoded path, not some relative/composed thing).

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:630
> +        if (this._property.name === "align-content")

What about all the other alignment properties (e.g. `justify-content`, `align-items`, etc.)?

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:850
>> +        return [swatch];
> 
> This won't work for multi-token values, like `first baseline` or `unsafe center`, right? This should probably be made to work similar to `_addColorTokens` above so that the known part gets tokenized.

since we're keying off the property name, we should also consider supporting resolving any CSS variable values in the value and having the swatch show based on that instead of just treating the CSS variable as an unknown value

[Nikita Vasilyev]

Comment on attachment 442423 [details]
Patch

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

>> Source/WebInspectorUI/ChangeLog:10
>> +        start, center, end, space-between, space-around, space-evenly, and stretch.
> 
> I really think we probably want to determine the writing direction and apply it's same effect to the visual representations for each of these values. E.g. `start` may be left-aligned in a horizontal layout in a LTR language, but would be right-aligned (mirror the icon on the X-axis) for an RTL element. Alignment also isn't always in the y-axis, where I would expect the icons to be rotated -90deg so that `start` is aligned to the top, for example. I don't think any of the icons need to be redrawn for these states, just mirrored/rotated.

I agree that we should do this, but I suggest to do this as a follow up. Aside from RTL text, `flex-direction` takes values "row", "row-reverse", "column", "column-reverse", so it affects both axis and direction. `grid-auto-flow` also affects direction.

>> Source/WebInspectorUI/ChangeLog:21
>> +        * UserInterface/Views/AlignContentEditor.css: Added.
> 
> Can we name this something like `AlignmentInlineSwatchEditor.css` to make clear it both related to inline swatches and makes it clearer that we will reuse these styles for other alignment properties like `justify-content`, `align-items`, `justify-items`? This kinda applies throughout in the naming of things.

Sounds good.

>> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:387
>> +            let glyphElement = WI.ImageUtilities.useSVGSymbol(this._getAlignContentGlyphPath(value), "glyph", value);
> 
> please make sure that each glyph has a `title` so that it's possible for a developer to hover to find out what the symbol represents if it's not immediately clear to them

The "title" attribute is already set — it's the 3rd argument of `useSVGSymbol`.

>> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:558
>> +            "start": "AlignContentStart.svg",
> 
> This kinda strikes me as odd for some reason 🤔 Do we have to use `WI.ImageUtilities.useSVGSymbol` or can we use CSS `background-image` instead?  If so, we could just set the CSS `class` based on the value and then have a declarative list in CSS of what each icon should be.
> 
> NIT: I feel like we should include the `Images/` in each of these paths so it's explicitly clear where they come from (e.g. a hardcoded path, not some relative/composed thing).

I can't use `background-image`, unfortunately, because currentColor keyword doesn't work in SVGs set via CSS.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:630
>> +        if (this._property.name === "align-content")
> 
> What about all the other alignment properties (e.g. `justify-content`, `align-items`, etc.)?

The plan is to add those as well (a little more context in the radar), but "align-content" is the 1st one I'm adding.

[Nikita Vasilyev]

Comment on attachment 442423 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.css:109
>> +}
> 
> Why doesn't the current `inline-swatch.image > span` rule get us this?

`inline-swatch.image` is specific to the image inline swatch type. My swatch doesn't have "image" class.

[Nikita Vasilyev]

Created attachment 443556 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 442423 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:850
>>> +        return [swatch];
>> 
>> This won't work for multi-token values, like `first baseline` or `unsafe center`, right? This should probably be made to work similar to `_addColorTokens` above so that the known part gets tokenized.
> 
> since we're keying off the property name, we should also consider supporting resolving any CSS variable values in the value and having the swatch show based on that instead of just treating the CSS variable as an unknown value

I tried Patrick's suggestion in my latest patch. There are cases when it really doesn't work well. Say, we have `align-content: unsafe center`. When I click on a swatch to change `center` to `space-between`, the value becomes `unsafe space-between`. It's invalid, so the entire property is ignored. "unsafe" only works for certain properties, e.g. "start", "center", "end".

It seemed like a good idea at 1st, but it turned out to introduce footguns. I think we should revert to the approach in my first patch.

[Nikita Vasilyev]

(In reply to Patrick Angle from comment #6)
> Comment on attachment 442423 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=442423&action=review
> 
> > Source/WebInspectorUI/ChangeLog:10
> > +        start, center, end, space-between, space-around, space-evenly, and stretch.
> 
> I really think we probably want to determine the writing direction and apply
> it's same effect to the visual representations for each of these values.
> E.g. `start` may be left-aligned in a horizontal layout in a LTR language,
> but would be right-aligned (mirror the icon on the X-axis) for an RTL
> element. Alignment also isn't always in the y-axis, where I would expect the
> icons to be rotated -90deg so that `start` is aligned to the top, for
> example. I don't think any of the icons need to be redrawn for these states,
> just mirrored/rotated.

I ultimately want to do this, but my latest patch doesn't include any of this logic.

[Devin Rousso]

Comment on attachment 442423 [details]
Patch

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

>>>> Source/WebInspectorUI/ChangeLog:10
>>>> +        start, center, end, space-between, space-around, space-evenly, and stretch.
>>> 
>>> I really think we probably want to determine the writing direction and apply it's same effect to the visual representations for each of these values. E.g. `start` may be left-aligned in a horizontal layout in a LTR language, but would be right-aligned (mirror the icon on the X-axis) for an RTL element. Alignment also isn't always in the y-axis, where I would expect the icons to be rotated -90deg so that `start` is aligned to the top, for example. I don't think any of the icons need to be redrawn for these states, just mirrored/rotated.
>> 
>> I agree that we should do this, but I suggest to do this as a follow up. Aside from RTL text, `flex-direction` takes values "row", "row-reverse", "column", "column-reverse", so it affects both axis and direction. `grid-auto-flow` also affects direction.
> 
> I ultimately want to do this, but my latest patch doesn't include any of this logic.

If this is expected as a followup then please file a bug for it and include FIXME comments in relevant parts of the code so that this isn't forgotten.

>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:630
>>> +        if (this._property.name === "align-content")
>> 
>> What about all the other alignment properties (e.g. `justify-content`, `align-items`, etc.)?
> 
> The plan is to add those as well (a little more context in the radar), but "align-content" is the 1st one I'm adding.

ditto (Source/WebInspectorUI/ChangeLog:10)

>>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:850
>>>> +        return [swatch];
>>> 
>>> This won't work for multi-token values, like `first baseline` or `unsafe center`, right? This should probably be made to work similar to `_addColorTokens` above so that the known part gets tokenized.
>> 
>> since we're keying off the property name, we should also consider supporting resolving any CSS variable values in the value and having the swatch show based on that instead of just treating the CSS variable as an unknown value
> 
> I tried Patrick's suggestion in my latest patch. There are cases when it really doesn't work well. Say, we have `align-content: unsafe center`. When I click on a swatch to change `center` to `space-between`, the value becomes `unsafe space-between`. It's invalid, so the entire property is ignored. "unsafe" only works for certain properties, e.g. "start", "center", "end".
> 
> It seemed like a good idea at 1st, but it turned out to introduce footguns. I think we should revert to the approach in my first patch.

ditto (Source/WebInspectorUI/ChangeLog:10)

[Devin Rousso]

Comment on attachment 443556 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/AlignmentInlineSwatchEditor.js:65
> +        this._element.removeChildren();

This seems unnecessarily destructive.  The only thing that changes between `_update` calls is the `"selected"` element.  Why can't we just create the children once and update the `"selected"` element instead?

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.css:107
> +    background-size: 9px 9px;

Do we specifically have to use `9px` or can we use `cover` instead?  If so, please merge this with the above using `:is()`.

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:192
> +        else if (this._type === WI.InlineSwatch.Type.AlignContent) {

NIT: Can we make this whole thing into a `switch`?

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:301
> +            this._valueEditor.addEventListener(WI.AlignmentInlineSwatchEditor.Event.ValueChanged, this._valueEditorValueDidChange, this);

This also should be removed in `didDismissPopover`.

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:546
> +    AlignContent: "inline-swatch-type-align-content",

It's a bit odd to me that you're calling it `WI.AlignmentInlineSwatchEditor` yet simultaneously limiting this to just `AlignContent`.  I think this (and everything else that's only specific to `AlignContent`) to just be `Alignment` so that when we do expand this to all alignment properties we don't have to also do a bunch of renaming.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:851
> +                let swatch = this._createInlineSwatch(WI.InlineSwatch.Type.AlignContent, [token], token.value);

NIT: I'd inline this.

[Nikita Vasilyev]

Created attachment 444087 [details]
Patch

[Devin Rousso]

Comment on attachment 444087 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/AlignmentInlineSwatchEditor.js:26
> +WI.AlignmentInlineSwatchEditor = class AlignmentInlineSwatchEditor extends WI.Object

Why not just `WI.AlignmentEditor`?

> Source/WebInspectorUI/UserInterface/Views/AlignmentInlineSwatchEditor.js:89
> +WI.AlignmentInlineSwatchEditor.AlignContentGlyphs = {

Can we name this something more generic like `ValueGlyphs`?  None of these values are unique to CSS `align-content` and could be reused for other properties too.

I'd also rename the SVG files to just be `Alignment*.svg` too.

> Source/WebInspectorUI/UserInterface/Views/AlignmentInlineSwatchEditor.js:99
> +WI.AlignmentInlineSwatchEditor.UnknownValueAlignmentGlyph = "Images/AlignmentUnknown.svg";

ditto (:89) with `UnknownValueGlyph`

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.css:61
> +.inline-swatch:not(.read-only):matches(.bezier, .box-shadow, .spring, .variable, .align-content):hover {

`.alignment`?

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.css:65
> +.inline-swatch:not(.read-only):matches(.bezier, .box-shadow, .spring, .variable, .align-content):active {

ditto (:61)

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.css:102
> +.inline-swatch.image > span,
> +.inline-swatch.alignment > span {

```
    .inline-swatch:is(.image, .alignment) > span {
```

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:194
> +            if (this._type === WI.InlineSwatch.Type.Color) {

NIT: you could avoid this extra check by having a fallthrough
```
    case WI.InlineSwatch.Type.Color:
        if (this._allowChangingColorFormats())
            this._swatchElement.title = WI.UIString("Click to select a color\nShift-click to switch color formats");
        else
            this._swatchElement.title = WI.UIString("Click to select a color");
        // fallthrough

    case WI.InlineSwatch.Type.Gradient:
        this._swatchInnerElement.style.background = value ? value.toString() : null;
        break;
```

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:201
> +        case WI.InlineSwatch.Type.Image:

Style: please add newlines before each `case` so it's easier to read :)

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:306
> +            // FIXME: <https://webkit.org/b/233054> Web Inspector: Add a swatch for align-items and align-self
> +            // FIXME: <https://webkit.org/b/233055> Web Inspector: Add a swatch for justify-content, justify-items, and justify-self

NIT: I'd also put these in more places (e.g. inside `WI.SpreadsheetStyleProperty.prototype._replaceSpecialTokens`, `WI.AlignmentEditor.ValueGlyhps`, etc.) so that if another engineer comes along and wants to do the work, they don't have to figure out the other spots that they need to adjust

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:846
> +    _addAlignContentTokens(tokens)

NIT: `_addAlignmentTokens`

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:848
> +        let text = this._resolveVariables(tokens.map((token) => token.value).join(""));

we should at least have a FIXME bug to handle multi-token values like `unsafe flex-start`

[Nikita Vasilyev]

Comment on attachment 444087 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/AlignmentInlineSwatchEditor.js:89
>> +WI.AlignmentInlineSwatchEditor.AlignContentGlyphs = {
> 
> Can we name this something more generic like `ValueGlyphs`?  None of these values are unique to CSS `align-content` and could be reused for other properties too.
> 
> I'd also rename the SVG files to just be `Alignment*.svg` too.

Note that `align-content: start` and `justify-content: start` will have different glyphs.

[Nikita Vasilyev]

Created attachment 444580 [details]
Patch

[Devin Rousso]

Comment on attachment 444087 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/AlignmentInlineSwatchEditor.js:89
>>> +WI.AlignmentInlineSwatchEditor.AlignContentGlyphs = {
>> 
>> Can we name this something more generic like `ValueGlyphs`?  None of these values are unique to CSS `align-content` and could be reused for other properties too.
>> 
>> I'd also rename the SVG files to just be `Alignment*.svg` too.
> 
> Note that `align-content: start` and `justify-content: start` will have different glyphs.

Oh hmm that's a good point.  It also probably should take into account the writing mode.  Perhaps we should name this with physical (as opposed to logical) directions in mind, e.g. `Images/AlignVerticalStart.svg` for `align-*` when horizontal and `justify-*` when vertical.

[Devin Rousso]

Comment on attachment 444580 [details]
Patch

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

r=me, nice work :)

> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:53
> +    static isAlignContentValue(value)

This should be changed to `isAlignmentValue` since `WI.AlignmentEditor.ValueGlyphs` is not specific to `align-content` anymore.

[Nikita Vasilyev]

Comment on attachment 444087 [details]
Patch

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

>>>> Source/WebInspectorUI/UserInterface/Views/AlignmentInlineSwatchEditor.js:89
>>>> +WI.AlignmentInlineSwatchEditor.AlignContentGlyphs = {
>>> 
>>> Can we name this something more generic like `ValueGlyphs`?  None of these values are unique to CSS `align-content` and could be reused for other properties too.
>>> 
>>> I'd also rename the SVG files to just be `Alignment*.svg` too.
>> 
>> Note that `align-content: start` and `justify-content: start` will have different glyphs.
> 
> Oh hmm that's a good point.  It also probably should take into account the writing mode.  Perhaps we should name this with physical (as opposed to logical) directions in mind, e.g. `Images/AlignVerticalStart.svg` for `align-*` when horizontal and `justify-*` when vertical.

Yeah, it will be either this or mirroring/rotating glyphs. It should get more clear soon when I jump working on the other swatches.

[Nikita Vasilyev]

Comment on attachment 444580 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:53
>> +    static isAlignContentValue(value)
> 
> This should be changed to `isAlignmentValue` since `WI.AlignmentEditor.ValueGlyphs` is not specific to `align-content` anymore.

The listed values are specific to `align-content` and, it appears, `justify-content` (which is likely going to have different glyphs anyway).
align-{items, self} and justify-{items, self} don't support space-between, space-evenly, and etc. And again, the glyphs might be different.

[EWS]

Committed r285983 (244380@main): <https://commits.webkit.org/244380@main>

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