At the bottom of the color picker, display a list of CSS variables with colors that are available for the selected element.
<rdar://problem/58097565>
Created attachment 387947 [details] Patch
Created attachment 387948 [details] [Video] With patch applied
Created attachment 387949 [details] Patch
Comment on attachment 387949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387949&action=review I'm heading home for now. Didn't get all the way through it, but there's some stuff to work on and some questions to be answered. I like the direction so far :) > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:31 > + display: flex; > + flex-direction: column; > + flex-wrap: nowrap; NIT: I'd put these first. > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:34 > + --color-picker-padding: 5px; This is only ever used for horizontal padding, so we should be more specific with the name like `--color-picker-horizontal-padding`. > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:54 > white-space: nowrap; NIT: I'd put this at the bottom. > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:70 > + position: relative; NIT: I normally put `position` after `display` (and its related, like `flex-*`) properties. > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:96 > + margin-right: 5px; What about RTL? > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:110 > + vertical-align: -1px; > + margin: 0; NIT: I'd switch the order of these. > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:128 > + margin-right: 3px; > + margin-left: 1px; Ditto (96) > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:32 > + this.delegate = delegate; This should be `_delegate` so it's known to be private. > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:150 > + this._colorPalette.removeChildren(); > + if (!cssProperties.length) Style: missing newline > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:322 > + if (event.target.closest(".palette-items")) Rather than do this with `closest` (which is basically a `querySelector`), we should move this after `headingInner` is created and have it be `headingInner.contains(event.target)`. > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:324 > + }, {capture: true}); Why does this need to be capturing? > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:327 > + headingInner.classList.add("heading-inner"); This isn't necessary with `createChild`. > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:328 > + headingInner.append(WI.UIString("CSS variables")); This should be sticky to the top when scrolling the variables list in order to keep the context of what you're scrolling, as well as provide a quick way to close the section (instead of scrolling to the top and then closing). > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:331 > + const MAX_NUMBER_OF_INLINE_SWATCHES_THAT_FIT = 14; Style: we don't use this style. Please use normal camelCase for constants, or just inline it. > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:332 > + let size = Math.min(MAX_NUMBER_OF_INLINE_SWATCHES_THAT_FIT, cssProperties.length); NIT: since `14` is the maximum, I'd put that as the last argument so we know it's the upper bound based on the ordering of the arguments. > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:341 > + itemElement.append(swatch.element); Please use `appendChild` when only appending one node that you know for sure is a node. > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:344 > + let handleClick = this._handlePaletteClick.bind(this, property.name, colorString, itemElement); > + itemElement.addEventListener("click", handleClick); Why not just inline this? > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:347 > + this._colorPalette.addEventListener("toggle", (event) => { This event listener is going to get re-added each time `_displayColorPalette` is called. It should also probably be a prototype function, given that it doesn't use any local variables. > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:363 > + let handleClick = this._handlePaletteClick.bind(this, property.name, colorString, itemElement); > + itemElement.addEventListener("click", handleClick); Ditto (343) > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:388 > + _handlePaletteClick(propertyName, colorString, element) You shouldn't need to bind the `element` given that it will be the `event.target`. > Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:168 > + if (this._valueText.startsWith("var(")) `this._type === WI.InlineSwatch.Type.Variable` > Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:244 > + this._valueEditor.addEventListener(WI.ColorPicker.Event.SizeChanged, (event) => popover.update()); Style: since we're not using the return value of the arrow function, please wrap the body in `{` and `}`. > Source/WebInspectorUI/UserInterface/Views/Popover.js:204 > + this._contentPadding = padding || 0; Shouldn't this also update `this._container.style.padding`? > Source/WebInspectorUI/UserInterface/Views/Popover.js:323 > + this._container.style.padding = `${this._contentPadding}px`; What if `this._contentPadding` isn't a number? Should we assert that a number is always provided? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:539 > + swatch.addEventListener(WI.InlineSwatch.Event.Activated, (event) => { Why isn't this a prototype function?
Comment on attachment 387949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387949&action=review >> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:328 >> + headingInner.append(WI.UIString("CSS variables")); > > This should be sticky to the top when scrolling the variables list in order to keep the context of what you're scrolling, as well as provide a quick way to close the section (instead of scrolling to the top and then closing). I agree, completely. I couldn't make this work. I thought, It *should* be as easy as adding: .color-picker > .color-palette { overflow-y: hidden; } .details-content { overflow-y: auto; } Yet this makes scrolling disappear entirely! Please let me know if I'm missing something obvious.
(In reply to Nikita Vasilyev from comment #6) > Comment on attachment 387949 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=387949&action=review > > >> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:328 > >> + headingInner.append(WI.UIString("CSS variables")); > > > > This should be sticky to the top when scrolling the variables list in order to keep the context of what you're scrolling, as well as provide a quick way to close the section (instead of scrolling to the top and then closing). > > I agree, completely. I couldn't make this work. I thought, It *should* be as > easy as adding: > > .color-picker > .color-palette { > overflow-y: hidden; > } > > .details-content { > overflow-y: auto; > } > > Yet this makes scrolling disappear entirely! Please let me know if I'm > missing something obvious. I made a HTML page trying to figure out what's going on. https://jsbin.com/poxexik/edit?html,css,output When I used just div's, everything worked as expected. There's something about <details> element that prevents its children from acting as flexbox items. This behaviors is consistent across the browsers. Tangentially, Firefox DevTools showed that <details>'s children are NOT flexbox items, which confirmed that something strange is going on.
Comment on attachment 387949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387949&action=review >> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:388 >> + _handlePaletteClick(propertyName, colorString, element) > > You shouldn't need to bind the `element` given that it will be the `event.target`. event.target is the inner most element that is clicked. It could be the `element` but it could also be InlineSwatch or some other element. >> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:168 >> + if (this._valueText.startsWith("var(")) > > `this._type === WI.InlineSwatch.Type.Variable` This wouldn't work for the following case: - Click on #333 - Select a color from the palette (which inserts a variable definition). this._type is WI.InlineSwatch.Type.Color.
Created attachment 388355 [details] Patch
Comment on attachment 388355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388355&action=review r-, for reasons described in the ChangeLog > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:81 > + /* `flex-shrink: 0;` prevents scrolling. */ > + flex-shrink: revert; Is this still needed? > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:99 > + margin-left: -1px; Is there really no way to avoid a negative margin? They're pretty awful for performance :( Also, it should be `-webkit-margin-start: -1px;`. > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:323 > + let expandableView = new WI.ExpandableView("color-picker-css-variables", headerElement, contentElement); > + expandableView.element.classList.add("color-palette"); > + this._element.replaceChild(expandableView.element, this._colorPalette); Rather than create a new `WI.ExpandableView` each time we get new `cssProperties`, can we create this in the `constructor` and hide it until it's asked to be shown, and then save references for `this._headerElement` and `this._childElement` and just modify them instead? This could help decrease the number of times we re-fetch the value of the `this._expandedSetting` from `localStorage` as well. > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:352 > + let itemElement = contentElement.createChild("div", "color-palette-item"); It seems like the only difference between this `for` loop and the above `for` loop is `"div"` vs `"span"` (and the `title`). Perhaps you could make a local function that takes a `WI.CSSProperty` and an `isInline` boolean so we don't duplicate this logic. > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:372 > + if (element) > + element.classList.add("selected"); Please use `this._selectedColorPaletteElement`. > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:400 > + SizeChanged: "css-color-picker-size-changed", Can we use a more specific/indicative name for this? Perhaps `VariablesViewToggled`? > Source/WebInspectorUI/UserInterface/Views/ExpandableView.js:41 > + headerElement.addEventListener("click", this._onTitleClick.bind(this)); Wouldn't it be more accurate to say `_handleHeaderClick`? > Source/WebInspectorUI/UserInterface/Views/ExpandableView.js:75 > + let focusNode = selection && !selection.isCollapsed && selection.focusNode; This is very odd, as it results in either `false` or a `Node`, which are not the same type. Please rework this. ``` if (selection && !selection.isCollapsed && this._headerElement.contains(selection.focusNode)) ``` > Source/WebInspectorUI/UserInterface/Views/ExpandableView.js:87 > + this.dispatchEventToListeners(WI.ExpandableView.Event.Toggled, {expanded: shouldExpand}); Why do we need to return `shouldExpand` instead of having callers just call `get expanded`? Also, we should probably put this inside `_update` (which you could make `set expanded` if you want) so it's never missed. > Source/WebInspectorUI/UserInterface/Views/GradientEditor.css:61 > + --color-picker-padding: 0px; Style: you can drop the `px` > Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:301 > + console.assert(value instanceof WI.Color); This assertion already exists inside `WI.ColorPicker.prototype.set color`. > Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:319 > + let resolvedValue = this.delegate.inlineSwatchResolveVariable(value); We should assert that this is set if `this._type === WI.InlineSwatch.Type.Variable` inside the constructor. > Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:345 > + this._valueText = ""; Why? > Source/WebInspectorUI/UserInterface/Views/Popover.js:202 > + setContentPadding(padding) Why not `set contentPadding`? > Source/WebInspectorUI/UserInterface/Views/Popover.js:324 > + this._container.style.padding = `${this._contentPadding}px`; Style: I would prefer `this._container.style.setProperty("padding", this._contentPadding + "px");` as it is easier to search (`"padding"` vs `.padding`) in the code and in general it matches the CSS syntax (e.g. `"padding-left"` instead of `.paddingLeft`), but in this case it's not a big deal > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:-531 > - swatch.value = () => { > - return this._property.ownerStyle.nodeStyles.computedStyle.resolveVariableValue(innerElement.textContent); > - }; I don't think we should remove this. The reason I made it this way when it was first added was because a CSS variable's value can change and the Web Inspector frontend, which won't get picked up by this, meaning that the swatch/color will be inaccurate. We either need to have better instrumentation around when CSS variables update or have the clicking of the swatch be the point when we determine the actual resolved value. # STEPS TO REPRODUCE: 1. inspect any page 2. add `color: var(--DoesNotExistVariable);` to the inline style of any node => no swatch is shown next to `var(--DoesNotExistVariable)` 3. add `--DoesNotExistVariable: red;` to some _other_ CSS rule that is already being applied to the current node => still no swatch next to `var(--DoesNotExistVariable)` This issue is even more evident if the CSS variable _does_ exist. # STEPS TO REPRODUCE 1. inspect any page 2. add `--foo: red;` to the inline style of any node 3. add `color: var(--foo);` to the inline style of the same node as step #2 => red color swatch is shown next to `var(--foo)` 3. add `--foo: blue !important;` to some _other_ CSS rule that is already being applied to the current node => still a red color swatch next to `var(--foo)` which _should_ be blue > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:536 > + swatch.delegate = this; Is there a reason to not always set the delegate, like in the constructor? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:539 > + swatch.addEventListener(WI.InlineSwatch.Event.Activated, this._handleColorSwatchActivated.bind(this)); Can we merge this with the event listener that is added below, so we don't add it twice? Also, you don't need to `bind` when `addEventListener` to an `WI.Object`. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:948 > + let colorPicker = event.target.valueEditor; I don't think this is a good approach, as it reveals an internal mechanism of `WI.InlineSwatch` and now requires that any changes that are made also update this code. Please either add a check (instead of assuming) that the `valueEditor` is a `WI.ColorPicker` or have a different way to access the `WI.ColorPicker` (e.g. `event.data.colorPicker`).
Comment on attachment 388355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388355&action=review >> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:99 >> + margin-left: -1px; > > Is there really no way to avoid a negative margin? They're pretty awful for performance :( > > Also, it should be `-webkit-margin-start: -1px;`. I've never heard that negative margins are awful for performance. What makes negative margins different? >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:-531 >> - }; > > I don't think we should remove this. The reason I made it this way when it was first added was because a CSS variable's value can change and the Web Inspector frontend, which won't get picked up by this, meaning that the swatch/color will be inaccurate. We either need to have better instrumentation around when CSS variables update or have the clicking of the swatch be the point when we determine the actual resolved value. > > # STEPS TO REPRODUCE: > 1. inspect any page > 2. add `color: var(--DoesNotExistVariable);` to the inline style of any node > => no swatch is shown next to `var(--DoesNotExistVariable)` > 3. add `--DoesNotExistVariable: red;` to some _other_ CSS rule that is already being applied to the current node > => still no swatch next to `var(--DoesNotExistVariable)` > > This issue is even more evident if the CSS variable _does_ exist. > > # STEPS TO REPRODUCE > 1. inspect any page > 2. add `--foo: red;` to the inline style of any node > 3. add `color: var(--foo);` to the inline style of the same node as step #2 > => red color swatch is shown next to `var(--foo)` > 3. add `--foo: blue !important;` to some _other_ CSS rule that is already being applied to the current node > => still a red color swatch next to `var(--foo)` which _should_ be blue I confirm that swatches don't update in these two cases. I agree it isn't great. However, keeping this would not resolve the issue. It would actually make the swatches show the outdated color before you click on them. What we should really do is to update the entire style editor when the variable value is edited.
Comment on attachment 388355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388355&action=review >> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:81 >> + flex-shrink: revert; > > Is this still needed? It isn't. Thanks! >> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:323 >> + this._element.replaceChild(expandableView.element, this._colorPalette); > > Rather than create a new `WI.ExpandableView` each time we get new `cssProperties`, can we create this in the `constructor` and hide it until it's asked to be shown, and then save references for `this._headerElement` and `this._childElement` and just modify them instead? This could help decrease the number of times we re-fetch the value of the `this._expandedSetting` from `localStorage` as well. This wouldn't decrease the number of times we re-fetch the value of the `this._expandedSetting` from `localStorage` because: * _displayColorPalette gets called only once after clicking on InlineSwatch, and... * WI.ColorPicker gets instantiated every time InlineSwatch is clicked. >> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:352 >> + let itemElement = contentElement.createChild("div", "color-palette-item"); > > It seems like the only difference between this `for` loop and the above `for` loop is `"div"` vs `"span"` (and the `title`). Perhaps you could make a local function that takes a `WI.CSSProperty` and an `isInline` boolean so we don't duplicate this logic. Sounds good. >> Source/WebInspectorUI/UserInterface/Views/ExpandableView.js:41 >> + headerElement.addEventListener("click", this._onTitleClick.bind(this)); > > Wouldn't it be more accurate to say `_handleHeaderClick`? Yes, thanks. >> Source/WebInspectorUI/UserInterface/Views/ExpandableView.js:87 >> + this.dispatchEventToListeners(WI.ExpandableView.Event.Toggled, {expanded: shouldExpand}); > > Why do we need to return `shouldExpand` instead of having callers just call `get expanded`? > > Also, we should probably put this inside `_update` (which you could make `set expanded` if you want) so it's never missed. I'll remove `{expanded: shouldExpand}`. I'm calling `_update` in the constructor and I don't want to trigger WI.ExpandableView.Event.Toggled there. >> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:345 >> + this._valueText = ""; > > Why? I'm going to rename `this._valueText` to `this._variableText`. Why? Consider the following scenario: 1. Display color picker for `background: var(--my-bg)` CSS property. 2. In the color picker, modify the color to rgb(255, 0, 0). The CSS property should be `background: rgb(255, 0, 0)` now. It shouldn't resolve the variable any more. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:536 >> + swatch.delegate = this; > > Is there a reason to not always set the delegate, like in the constructor? I wanted to make it clear that `delegate` is used only for these two types. I don't have a strong preference here. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:948 >> + let colorPicker = event.target.valueEditor; > > I don't think this is a good approach, as it reveals an internal mechanism of `WI.InlineSwatch` and now requires that any changes that are made also update this code. Please either add a check (instead of assuming) that the `valueEditor` is a `WI.ColorPicker` or have a different way to access the `WI.ColorPicker` (e.g. `event.data.colorPicker`). This looked iffy to me as well. I'll make it `event.data.colorPicker`.
Created attachment 389897 [details] Patch
Created attachment 389911 [details] Patch ^ Forgot to update the changelog.
Comment on attachment 388355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388355&action=review >>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:-531 >>> - }; >> >> I don't think we should remove this. The reason I made it this way when it was first added was because a CSS variable's value can change and the Web Inspector frontend, which won't get picked up by this, meaning that the swatch/color will be inaccurate. We either need to have better instrumentation around when CSS variables update or have the clicking of the swatch be the point when we determine the actual resolved value. >> >> # STEPS TO REPRODUCE: >> 1. inspect any page >> 2. add `color: var(--DoesNotExistVariable);` to the inline style of any node >> => no swatch is shown next to `var(--DoesNotExistVariable)` >> 3. add `--DoesNotExistVariable: red;` to some _other_ CSS rule that is already being applied to the current node >> => still no swatch next to `var(--DoesNotExistVariable)` >> >> This issue is even more evident if the CSS variable _does_ exist. >> >> # STEPS TO REPRODUCE >> 1. inspect any page >> 2. add `--foo: red;` to the inline style of any node >> 3. add `color: var(--foo);` to the inline style of the same node as step #2 >> => red color swatch is shown next to `var(--foo)` >> 3. add `--foo: blue !important;` to some _other_ CSS rule that is already being applied to the current node >> => still a red color swatch next to `var(--foo)` which _should_ be blue > > I confirm that swatches don't update in these two cases. I agree it isn't great. > > However, keeping this would not resolve the issue. It would actually make the swatches show the outdated color before you click on them. > > What we should really do is to update the entire style editor when the variable value is edited. I see now that I wasn't entirely clear. What I meant when I said "this way" was to go back to using the [=] icon _instead_ of a color swatch for CSS variables that have color values. If we keep it so that we always use [=] for all CSS variables (including ones with a color value), then this code does not have the issue as described, as the resolved value is only fetched on activation, meaning that it always uses the most recent value. Updating the entire style editor when a variable is edited has possible performance and/or editing (e.g. preserving the currently focused/editing element) concerns, but I do agree that we should do something about our instrumentation. Until something is done to improve that instrumentation, however, we shouldn't introduce a bug, so please revert that part of this patch. NIT: also, this change doesn't really have anything to do with this bug as it is currently titled ("add a list of usable CSS variables for the selected node" vs "show a color swatch for CSS variables that have a color value") , and really should be its own change.
Displaying color preview is very helpful. It allows to see what the variable represents without clicking on [=]. It's especially handy when there are several variables used — you don't have to click variable one at the time to see previews. > NIT: also, this change doesn't really have anything to do with this bug as > it is currently titled ("add a list of usable CSS variables for the selected > node" vs "show a color swatch for CSS variables that have a color value") , > and really should be its own change. I think this change is related. Imagine you just clicking a color palette to pick a color --my-background. Clicking on "--my-background" inline swatch should display color picker. I talked to Greg about it and we both think that it would be better to show inline swatches with previews for variables with colors. We think it would be better to land this and address the issue with outdated previews as a follow-up.
Comment on attachment 389911 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389911&action=review > Source/WebInspectorUI/ChangeLog:12 > + Be default, we display only a handful of colors. "CSS Variables" section displays a single line s/Be/By > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:40 > + --color-picker-width: 266px; Why does it need to be `10px` wider? > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:41 > + --color-picker-horizontal-padding: 10px; Shouldn't we use the same padding regardless of whether we're going to show palettes or not? > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:50 > + padding-bottom: 5px; I don't think this is enough. It should match the padding on all the other sides of the `WI.Popover`. > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:54 > + flex-shrink: 0; Is this still needed? > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:56 > + padding: var(--color-picker-horizontal-padding); > + padding-bottom: 0; Can these be merged? ``` padding: 10px var(--color-picker-horizontal-padding) 0; ``` > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:85 > + font-size: 100%; Why is this needed? > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:93 > +.color-picker > .color-palette.expandable > .header .disclosure-button { Why no `>` between `.header > .disclosure-button`? > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:94 > + margin-left: -1px; How does this look in RTL? These types of styles can easily cause bugs in the future, such as if someone makes a change to `WI.ExpandableView` as they would likely have no idea that this rule exists, and could therefore make a change that wouldn't work here leading to an undiscovered bug. The way I've been trying to lessen this risk is by using unique CSS variables which can be searched for in the codebase to instantly identify where things are being used in a special way. ExpandableView.css ``` .expandable > .header > .disclosure-button { -webkit-margin-start: var(--expandable-header-disculosure-margin-start); --expandable-header-disclosure-margin-start-default: 0; --expandable-header-disclosure-margin-start: var(--expandable-header-disclosure-margin-start-override, var(--expandable-header-disclosure-margin-start-default)); } ``` ColorPicker.css ``` .color-picker > .color-palette.expandable > .header > .disclosure-button { --expandable-header-disclosure-margin-start-override: -1px; } ``` > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:99 > +.color-picker .color-palette.expanded > .header > .palette-items { > + display: inline; > +} This is overridden by the rule below. Do we need it? > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:101 > +.color-picker .color-palette.expanded > .header > .palette-items { Why no `>` between `.color-picker > .color-palette`? Ditto for all the ones below. > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:112 > + vertical-align: -1px; This makes it so that the bottom border of the `WI.InlineSwatch` is lined up with the text next to it, but that's not how it appears in the Styles panel or the Computed panel. Why are you adjusting it in this case? > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:125 > + border-color: var(--glyph-color-active); This causes the border of the `WI.InlineSwatch` to almost match the `background-color` but not entirely so there are some weird white artifacts around the corner. When a palette is selected, shouldn't we make the `border-color` something completely different from `--background-color-selected` (e.g. `white`)? > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:135 > + -webkit-margin-start: 1px; > + -webkit-margin-end: 3px; You just overrode the `margin` above, so why are you now resetting it back? Is there a reason not to use the default margins of `.inline-swatch`? Same general comment as 94. > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:339 > + for (let i = 0; i < size; ++i) { > + let property = cssProperties[i]; `for (let property of cssProperties) {`? > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:349 > + if (isInline) > + swatch.element.title = property.name; > + else > + swatch.element.title = colorString; Rather than override the `title` after, can we add an `options = {}` to the constructor of `WI.InlineSwatch` and allow callers to pass in their own `title` instead of "hacking" it after? > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:355 > + itemElement.addEventListener("click", this._handlePaletteClick.bind(this, property.name, colorString, itemElement)); Frankly, you're already creating a new function since you call `bind`, so you may as well just inline this function here instead of relying on `bind`. > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:360 > + displayPaletteItems(contentElement, false); Style: trailing falsy arguments can be omitted > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:365 > + this._element.classList.toggle("variables-expanded", expandableView.expanded); NIT: due to the nature of `.classList.toggle`, I'd recommend adding `!!expandableView.expanded` > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:368 > + /* Should this be removed? > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:403 > + _handlePaletteClick(propertyName, colorString, element, event) Won't `element === event.currentTarget`? > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:407 > + this.color = WI.Color.fromString(colorString); I think you also need to update the `WI.InlineSwatch` of this as well, as otherwise it'll be left with a stale value. # STEPS TO REPRODUCE: 1. inspect <https://devinrousso.com/demo/WebKit/test.html> 2. select the `<body>` in the Elements Tab 3. click the color swatch next to `background-color: var(--test1)` in the `*, body, div` CSS rule => the value should be hsla(0, 0%, 10%, 1) 4. change the value to something else 5. click the `test1` swatch in the *CSS Variables* section at the bottom of the popover => the value should change back to hsla(0, 0%, 10%, 1) 6. click somewhere else to dismiss the color popover 7. click the color popover again from step #3 => the value should be hsla(0, 0%, 10%, 1), but is instead the value from step #4 > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:410 > + if (this._delegate && this._delegate.colorPickerSetVariable) { NIT: `this._delegate?.colorPickerSetVariable` =D > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:411 > + let valueText = `var(${propertyName})`; NIT: why not inline this? > Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:54 > -webkit-margin-start: calc(-1 * var(--disclosure-button-size)); Ditto (ColorPicker.css:94) > Source/WebInspectorUI/UserInterface/Views/ExpandableView.css:44 > + background-image: url(../Images/DisclosureTriangles.svg#open-normal); What about RTL? > Source/WebInspectorUI/UserInterface/Views/ExpandableView.css:52 > + display: block; Rather than force the `.content` to be a block element, I've found that adding a `:not` is simpler and more flexible :) ``` .expandable:not(.expanded) > .content { display: none; } ``` > Source/WebInspectorUI/UserInterface/Views/ExpandableView.js:33 > + this._element.className = "expandable"; Can we use a different `className`? Something that isn't so generic? `.expandable` is also used by `WI.ConsoleMessageView`, `WI.CPUTimelineView`, and `WI.SourceCodeTextEditor`. > Source/WebInspectorUI/UserInterface/Views/GradientEditor.css:61 > + --color-picker-padding: 0; `--color-picker-horizontal-padding`? > Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:118 > + this._delegate = delegate || null; Why was this changed back from being a constructor parameter? I would much prefer having a non-modifiable `_delegate`. IMO, it's fine to have it as part of an `options = {}` too if we don't always need it. > Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:144 > + this._updateSwatch(false); Ditto (ColorPicker.js:360) > Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:244 > + this._valueEditor.addEventListener(WI.ColorPicker.Event.VariablesSectionToggled, (event) => { popover.update() }); Style: missing `;` > Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:318 > + let resolvedValue = this.delegate.inlineSwatchResolveVariable(value); We should assert that this is set if `this._type === WI.InlineSwatch.Type.Variable` inside the constructor. Also, please use `_delegate`. > Source/WebInspectorUI/UserInterface/Views/Popover.js:94 > + this._contentPadding = padding || 0; Shouldn't this also `this._container.style.setProperty("padding", this._contentPadding + "px");`, or at the very least call `_update(true)` if `this.visible`? > Source/WebInspectorUI/UserInterface/Views/Popover.js:323 > + console.assert(typeof this._contentPadding === "number"); NIT: this assertion should really be in the setter, as that's where the value would change > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:539 > + swatch.addEventListener(WI.InlineSwatch.Event.Activated, this._handleColorSwatchActivated, this); Can we merge this with the event listener that is added below, so we don't add it twice? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:937 > + colorVariables.sort((a, b) => { You could also use `a.name.extendedLocaleCompare(b.name)`
Comment on attachment 389911 [details] Patch Clearing r? and cq? due to the patch being stale. Please rebase, address outstanding review comments, and send a new patch :-)