Bug 233054 – Web Inspector: Add a swatch for align-items and align-self

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

.

[Radar WebKit Bug Importer]

<rdar://problem/85613199>

[Nikita Vasilyev]

https://nv.github.io/webkit-inspector-bugs/css-alignment/ - demo.

[Nikita Vasilyev]

Created attachment 445912 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 445912 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:129
> +WI.AlignmentEditor.AlignItemsGlyphs = {

I went with this straightforward naming matching CSS property name. It seems align-items and align-self will use these glyphs and no other properties.

Devin previously proposed (https://bugs.webkit.org/show_bug.cgi?id=230065#c20):

> ... 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.

This wouldn't quite work — align-content and align-items both aligning vertically (by default, LTR, horizontal writing mode), yet they are different and, I think, need different icons.

[Nikita Vasilyev]

Created attachment 445915 [details]
[Video] With patch applied

[Patrick Angle]

Comment on attachment 445912 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).
> +

A summary of what values are now supported for which properties would be helpful along with an overview of the changes you made, particularly to `AlignmentEditor.js` to accommodate the addition properties.

> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:68
> +    static _glyphsForProperty(propertyName)

NIT: Can we move this private static method to the end of the set of static methods? Relatedly, does `isKnownValue` need to be a public static method, or could it also be private?

> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:77
> +        switch (propertyName) {
> +        case "align-content":
> +            return WI.AlignmentEditor.ValueGlyphs;
> +        case "align-items":
> +        case "align-self":
> +            return WI.AlignmentEditor.AlignItemsGlyphs;
> +        }
> +    }

We should include a `return null;` after the switch statement to make clear that returning null is the expected behavior for unsupported properties.

> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:83
> +            return null;

Would it be better to return `WI.AlignmentEditor.UnknownValueGlyph` here instead? I ask because on InlineSwatch.js:207 we used to default to that value if we didn't have a glyph, and I suspect we would want to keep that behavior? We may also want to assert that we do find a `glyphs` for the property name, since I don't think this should be reachable without having checked that already.

> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:119
>  WI.AlignmentEditor.ValueGlyphs = {

Given your comment below re: these icons not working for align-items/align-self, do these icons get used with any other property, or should this set of glyphs also be renamed to `WI.AlignmentEditor.AlignContentGlyphs`?

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:209
> +            value = value.text;

It seems strange that we would need to update the value itself as part of updating the swatch. Can you explain this a bit more? It also looks like _swatchElementClicked() expects value to be an Object, not a String when dealing with alignment, so it seems odd we would want to set the value to text here.

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

Don't forget to remove this :)

[Nikita Vasilyev]

Comment on attachment 445912 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:83
>> +            return null;
> 
> Would it be better to return `WI.AlignmentEditor.UnknownValueGlyph` here instead? I ask because on InlineSwatch.js:207 we used to default to that value if we didn't have a glyph, and I suspect we would want to keep that behavior? We may also want to assert that we do find a `glyphs` for the property name, since I don't think this should be reachable without having checked that already.

I don't think this should ever happen, actually. I should remove this and perhaps add an assert.

>> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:209
>> +            value = value.text;
> 
> It seems strange that we would need to update the value itself as part of updating the swatch. Can you explain this a bit more? It also looks like _swatchElementClicked() expects value to be an Object, not a String when dealing with alignment, so it seems odd we would want to set the value to text here.

I see this part is confusing. I'm only changing the variable that is used by WI.InlineSwatch.Event.ValueChanged a few lines below.

In SpreadsheetStyleProperty.js:

         swatch.addEventListener(WI.InlineSwatch.Event.ValueChanged, function(event) {
            let value = event.data.value && event.data.value.toString();

the `value` is expected to be the CSS property value, a string. I can add something like:

            if (swatch.type === WI.InlineSwatch.Type.Alignment)
                value = event.data.value.text;

which I don't like that much either.

[Nikita Vasilyev]

Comment on attachment 445912 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:119
>>  WI.AlignmentEditor.ValueGlyphs = {
> 
> Given your comment below re: these icons not working for align-items/align-self, do these icons get used with any other property, or should this set of glyphs also be renamed to `WI.AlignmentEditor.AlignContentGlyphs`?

I plan to either:
1. use them for `justify-content` and rotate -90deg.
2. only use them for align-items/align-self (the code to rotate for a very specific case looks a bit strange), and have separate icons for `justify-content`.
In case of 2, yes, it should be `WI.AlignmentEditor.AlignContentGlyphs`.

[Patrick Angle]

Comment on attachment 445912 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:209
>>> +            value = value.text;
>> 
>> It seems strange that we would need to update the value itself as part of updating the swatch. Can you explain this a bit more? It also looks like _swatchElementClicked() expects value to be an Object, not a String when dealing with alignment, so it seems odd we would want to set the value to text here.
> 
> I see this part is confusing. I'm only changing the variable that is used by WI.InlineSwatch.Event.ValueChanged a few lines below.
> 
> In SpreadsheetStyleProperty.js:
> 
>          swatch.addEventListener(WI.InlineSwatch.Event.ValueChanged, function(event) {
>             let value = event.data.value && event.data.value.toString();
> 
> the `value` is expected to be the CSS property value, a string. I can add something like:
> 
>             if (swatch.type === WI.InlineSwatch.Type.Alignment)
>                 value = event.data.value.text;
> 
> which I don't like that much either.

Ah, I see this doesn't change anything outside of this function (and handler of the event dispatched below) now. However, I'm also confused/concerned by the `WI.InlineSwatch.Type.Image` case above, where the value is an object and doesn't get flattened to text before dispatching the even below. It seems like we should either match the existing "leave the value as is" or fully commit to flattening the value down to text. Right now it just seems we are being inconsistent (it's completely possible the logic in `...Image` is flawed and we are lucky it works at all).

[Nikita Vasilyev]

Comment on attachment 445912 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:68
>> +    static _glyphsForProperty(propertyName)
> 
> NIT: Can we move this private static method to the end of the set of static methods? Relatedly, does `isKnownValue` need to be a public static method, or could it also be private?

Yes, it could be private, thanks.

>>> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:209
>>> +            value = value.text;
>> 
>> It seems strange that we would need to update the value itself as part of updating the swatch. Can you explain this a bit more? It also looks like _swatchElementClicked() expects value to be an Object, not a String when dealing with alignment, so it seems odd we would want to set the value to text here.
> 
> I see this part is confusing. I'm only changing the variable that is used by WI.InlineSwatch.Event.ValueChanged a few lines below.
> 
> In SpreadsheetStyleProperty.js:
> 
>          swatch.addEventListener(WI.InlineSwatch.Event.ValueChanged, function(event) {
>             let value = event.data.value && event.data.value.toString();
> 
> the `value` is expected to be the CSS property value, a string. I can add something like:
> 
>             if (swatch.type === WI.InlineSwatch.Type.Alignment)
>                 value = event.data.value.text;
> 
> which I don't like that much either.

A better option, I think, would be to define toString on the the value object.

[Nikita Vasilyev]

Created attachment 446086 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 446086 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:119
> -WI.AlignmentEditor.ValueGlyphs = {
> +WI.AlignmentEditor.AlignContentGlyphs = {

I'm planning to post a patch for "Bug 233055 - Web Inspector: Add a swatch for justify-content, justify-items, and justify-self" tomorrow.
I suggest to delay the naming discussion of until then.

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:209
> +            value = value.text;

Whoops, forgot to remove this.

[Nikita Vasilyev]

Created attachment 446088 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 446088 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:40
> -        for (let [value, path] of Object.entries(WI.AlignmentEditor.ValueGlyphs)) {
> +        for (let [value, path] of WI.AlignmentEditor._glyphsForProperty(propertyName)) {

Oops, I broke it by removing `Object.entries` 🤦‍♂️

[Nikita Vasilyev]

Created attachment 446185 [details]
Patch

[Patrick Angle]

Comment on attachment 446185 [details]
Patch

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

r=me with a few nits.

While I'm thinking about it, please open bug(s) for the visual adjustments we discussed this morning (button size/margin/padding, cleaning up the tiny icons, etc.)

> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:41
> +        let glyphs = Object.entries(WI.AlignmentEditor._glyphsForProperty(propertyName));
> +        for (let [value, path] of glyphs) {

Nit: I'd keep `glyphs` inlined.

> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:64
> +        return glyphs[value] || WI.AlignmentEditor.UnknownValueGlyph;

Just in case, can we `return glyphs?.[value] || ...` so that when failing the assertion we still return a sensical value instead of raising another exception here?

[Devin Rousso]

Comment on attachment 446185 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:120
>      "start": "Images/AlignmentStart.svg",

NIT: we should probably rename the images to match as well

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:855
> +        let valueObject = {

This is really hacky :/

Can we maybe make a `WI.AlignmentData` wrapper model object so it's more clear?  Ideally it'd also have a `WI.AlignmentData.Type` that would map/replace the `propertyName` so that we're using a frontend value (which means it's more easy to change) instead of one from the page/backend (even if it is a CSS property name).

NIT: I'd just inline this

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:858
> +            toString: function() { return this.text; }

Style: missing trailing `,`

[Patrick Angle]

Comment on attachment 446185 [details]
Patch

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

Removing r+ for now; Devin makes a good argument for a WI.AlignmentData or similar object that will want to be reviewed.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:855
>> +        let valueObject = {
> 
> This is really hacky :/
> 
> Can we maybe make a `WI.AlignmentData` wrapper model object so it's more clear?  Ideally it'd also have a `WI.AlignmentData.Type` that would map/replace the `propertyName` so that we're using a frontend value (which means it's more easy to change) instead of one from the page/backend (even if it is a CSS property name).
> 
> NIT: I'd just inline this

+1 good idea

[Nikita Vasilyev]

Created attachment 446339 [details]
Patch

This turned out to be more changes than I anticipated, but now it's more consistent with our existing inline swatches and editors.

[Nikita Vasilyev]

Comment on attachment 446185 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:855
>>> +        let valueObject = {
>> 
>> This is really hacky :/
>> 
>> Can we maybe make a `WI.AlignmentData` wrapper model object so it's more clear?  Ideally it'd also have a `WI.AlignmentData.Type` that would map/replace the `propertyName` so that we're using a frontend value (which means it's more easy to change) instead of one from the page/backend (even if it is a CSS property name).
>> 
>> NIT: I'd just inline this
> 
> +1 good idea

Devin: I've added WI.AlignmentData, but I'm not sure what you mean by "Ideally it'd also have a `WI.AlignmentData.Type` that would map/replace the `propertyName` so that we're using a frontend value (which means it's more easy to change) instead of one from the page/backend (even if it is a CSS property name)".
I have, however, added `WI.AlignmentData.Type` and an assert in WI.AlignmentData constructor.

[Nikita Vasilyev]

Created attachment 446341 [details]
Patch

The Changelog was slightly off.

[Nikita Vasilyev]

(In reply to Patrick Angle from comment #16)
> While I'm thinking about it, please open bug(s) for the visual adjustments
> we discussed this morning (button size/margin/padding, cleaning up the tiny
> icons, etc.)

Bug 234036 - Web Inspector: Revamp visual design of Alignment editor

[Patrick Angle]

Comment on attachment 446341 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/AlignmentData.js:32
> +        this._propertyName = propertyName;

Can we instead just store and support getting a WI.AlignmentData.Type here?

> Source/WebInspectorUI/UserInterface/Models/AlignmentData.js:40
> +    get propertyName() { return this._propertyName; }
> +
> +    get text() { return this._text; }

Nit: Unnecessary newline between

> Source/WebInspectorUI/UserInterface/Models/AlignmentData.js:53
> +    AlignContent: "align-content",
> +    AlignItems: "align-content",
> +    AlignSelf: "align-content",

Shouldn't these have unique values?

> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:28
> +    constructor(propertyName)

Along the lines of AlignmentData.js:32, this could then just take a WI.AlignmentData.Type to eliminate needing to reference raw property names at all in this class.

> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:53
> +    static isAlignmentAwarePropertyName(propertyName)

Ditto :28

> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:58
> +    static getGlyphPath(propertyName, value)

Ditto :28

> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:65
> +    static _glyphsForProperty(propertyName)

Ditto :28

> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:96
> +    set alignment(alignment)
>      {
> -        if (this._value && WI.AlignmentEditor.isAlignContentValue(this._value)) {
> -            let previousGlyphElement = this._valueToGlyphElement.get(this._value);
> -            previousGlyphElement.classList.remove("selected");
> +        console.assert(alignment instanceof WI.AlignmentData);
> +
> +        if (this._alignment?.text) {
> +            let previousGlyphElement = this._valueToGlyphElement.get(this._alignment.text);
> +            previousGlyphElement?.classList.remove("selected");
>          }
> -        this._value = value;
> +        this._alignment = alignment;
>          this._updateSelected();
>      }

What prevents the WI.AlignmentData.prototype.propertyName (hopefully `.type` in the next iteration) from changing between invocations other than no existing code path doing so? It seems odd all the icons are pre-populated when creating the swatch, but technically there is no guarantee those options will be correct for the new WI.AlignmentData that is provided.

[Devin Rousso]

Comment on attachment 446341 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/AlignmentData.js:30
> +        console.assert(Object.values(WI.AlignmentData.Type).includes(propertyName), "propertyName is not supported: ", propertyName);

NIT: the description is redundant given that we're already using a `console.assert`

>> Source/WebInspectorUI/UserInterface/Models/AlignmentData.js:32
>> +        this._propertyName = propertyName;
> 
> Can we instead just store and support getting a WI.AlignmentData.Type here?

Yeah that was what I was suggesting.  We could/should have callers of this (or have a separate `static` method on this class) do the conversion from CSS property name to `WI.AlignmentData.Type`.

The idea behind this is to have a layer of separation between backend values and things used internally only by the frontend.  This way, if a new CSS property was added that happened to overlap with an existing frontend value, we'd be able to add support for it with minimal changes (e.g. just adding a `case` for it when converting from the CSS property name to `WI.AlignmentData.Type`).  We do this when converting protocol enums to frontend enums (e.g. `WI.CSSManager.protocolStyleSheetOriginToEnum`).

>> Source/WebInspectorUI/UserInterface/Models/AlignmentData.js:40
>> +    get text() { return this._text; }
> 
> Nit: Unnecessary newline between

Actually no this should be there.  We prefer to add spacing around `get` `set` pairs if they're simple enough to be inlined.

> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:43
> +                this.alignment = new WI.AlignmentData(this._alignment.propertyName, value);

Why are we creating a new `WI.AlignmentData` for this?  Can't we just `this._alignment.text = value`?

Also, this assumes that `this._alignment` is valid.  Is that always the case?

[Nikita Vasilyev]

Created attachment 446633 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 446633 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:-360
> -        case WI.InlineSwatch.Type.Alignment:
> -            this._valueEditor.value = value;
> -            break;
> -

I need to do this before showing the popover (at line 307), or the popover is too small (since there's no content in it when calculating its size).

[Patrick Angle]

Comment on attachment 446633 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:19
> +        Rename icons from Alignment to AlingContent since they are only used for `align-content` CSS property now.

NIT: s/AlingContent/AlignContent

> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:41
> +    static getGlyphPath(type, value)

NIT: This could just take a `WI.AlignmentData` instead of splitting the type and text out separately.

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:207
> +            let glyphPath = WI.AlignmentEditor.getGlyphPath(value.type, value.text);

See AlignmentEditor.js:41

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:306
> +            this._valueEditor = new WI.AlignmentEditor();

NIT: Parameterless constructor doesn't need `()`, so this line doesn't need to change.

>> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:-360
>> -
> 
> I need to do this before showing the popover (at line 307), or the popover is too small (since there's no content in it when calculating its size).

I wonder why all these values are set in this second switch statement instead of in the first one. It would be nice to know if there is a reason before we break that pattern. This is probably really a question for @Devin since he wrote the original implementation that set the value after filling the popover (back when color was the only type of swatch): https://trac.webkit.org/browser/webkit/trunk/Source/WebInspectorUI/UserInterface/Views/ColorSwatch.js?rev=194568

[Nikita Vasilyev]

Created attachment 446740 [details]
Patch

[Devin Rousso]

Comment on attachment 446633 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:41
>> +    static getGlyphPath(type, value)
> 
> NIT: This could just take a `WI.AlignmentData` instead of splitting the type and text out separately.

NIT: i'd also drop the "get" from the name

> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:89
> +        } else if (this._alignment?.text)

NIT: Is the `if` part of this actually necessary?  You already check if `previousGlyphElement` exists in `_removePreviouslySelected` before doing anything with it.

> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:106
> +        if (!this._alignment?.text)

ditto (:89)

>>> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:-360
>>> -
>> 
>> I need to do this before showing the popover (at line 307), or the popover is too small (since there's no content in it when calculating its size).
> 
> I wonder why all these values are set in this second switch statement instead of in the first one. It would be nice to know if there is a reason before we break that pattern. This is probably really a question for @Devin since he wrote the original implementation that set the value after filling the popover (back when color was the only type of swatch): https://trac.webkit.org/browser/webkit/trunk/Source/WebInspectorUI/UserInterface/Views/ColorSwatch.js?rev=194568

My guess is that we want to wait for the `WI.Popover` to be attached to the main DOM tree before updating the color/gradient/variable editor so that the editor can do self-sizing (e.g. we frequently have to sprinkle `codeMirror.refresh()` calls in places where we add a `CodeMirror` instance to a parent before the parent is in the main DOM tree because otherwise `CodeMirror` doesn't size properly).  Normally though there'd be a corresponding `popover.update()` so that after the editor has resized, we also resize the `WI.Popover`.  Not really sure what's going on here.  If we did want to adjust this it'd probably be pretty easy to test :)

[Patrick Angle]

Comment on attachment 446633 [details]
Patch

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

>>>> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:-360
>>>> -
>>> 
>>> I need to do this before showing the popover (at line 307), or the popover is too small (since there's no content in it when calculating its size).
>> 
>> I wonder why all these values are set in this second switch statement instead of in the first one. It would be nice to know if there is a reason before we break that pattern. This is probably really a question for @Devin since he wrote the original implementation that set the value after filling the popover (back when color was the only type of swatch): https://trac.webkit.org/browser/webkit/trunk/Source/WebInspectorUI/UserInterface/Views/ColorSwatch.js?rev=194568
> 
> My guess is that we want to wait for the `WI.Popover` to be attached to the main DOM tree before updating the color/gradient/variable editor so that the editor can do self-sizing (e.g. we frequently have to sprinkle `codeMirror.refresh()` calls in places where we add a `CodeMirror` instance to a parent before the parent is in the main DOM tree because otherwise `CodeMirror` doesn't size properly).  Normally though there'd be a corresponding `popover.update()` so that after the editor has resized, we also resize the `WI.Popover`.  Not really sure what's going on here.  If we did want to adjust this it'd probably be pretty easy to test :)

Given that, I'd suggest we go ahead and do what Nikita has done here, and also open a bug to evaluate the other swatch types and how we set their values.

[Nikita Vasilyev]

Created attachment 446786 [details]
Patch

[Devin Rousso]

Comment on attachment 446786 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Models/AlignmentData.js:62
> +    get text() { return this._text; }
> +    set text(text) { this._text = text; }

Aside: we should probably turn this into a similar enum to `WI.AlignmentData.Type`, but we can do that as a followup

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:206
>          case WI.InlineSwatch.Type.Alignment:

I just realized, this needs `{` `}` around the body (i.e. after the `:` and on the line after the `break;`) in order to use `let` <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/switch#block-scope_variables_within_switch_statements>.

Alternatively, you could just inline `glyphPath` and remove the `let` entirely.

[Nikita Vasilyev]

Comment on attachment 446786 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:206
>>          case WI.InlineSwatch.Type.Alignment:
> 
> I just realized, this needs `{` `}` around the body (i.e. after the `:` and on the line after the `break;`) in order to use `let` <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/switch#block-scope_variables_within_switch_statements>.
> 
> Alternatively, you could just inline `glyphPath` and remove the `let` entirely.

Currently, glyphPath is scoped for the entire method and there aren't any other `glyphPath` definitions. Is the idea behind adding `{` `}` to make the code more error prone for future?
Meanwhile, I'll just inline it.

[Nikita Vasilyev]

Created attachment 446792 [details]
Patch

[EWS]

Committed r286875 (245105@main): <https://commits.webkit.org/245105@main>

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