.
<rdar://problem/48905785>
Created attachment 364758 [details] Patch
Created attachment 364774 [details] [Image] With patch applied Nice! With this patch, we show a go-to arrow after the variable on property hover. We already show inline swatches before variables. I wonder if we could minimize UI chrome. Generally, we don't want to clutter data with too many buttons.
Created attachment 364775 [details] [Image] go-to arrow inside of the variable popover What if we move the arrow inside of the variable popover? It would reduce the clutter a bit.
(In reply to Nikita Vasilyev from comment #3) > With this patch, we show a go-to arrow after the variable on property hover. > > We already show inline swatches before variables. I wonder if we could minimize UI chrome. Generally, we don't want to clutter data with too many buttons. One minor "difference" is that I made it so that the go-to arrow is placed immediately after the closing ")" of the variable, so it would appear in the middle of the CSS value if a variable is used inside another function/value (e.g. `hsla(0, 0%, var(--foreground-lightness) =>, 0.1)` would have an arrow where the "=>" is). (In reply to Nikita Vasilyev from comment #4) > Created attachment 364775 [details] > [Image] go-to arrow inside of the variable popover > > What if we move the arrow inside of the variable popover? It would reduce the clutter a bit. That makes me think that the clicking arrow will replace the CSS text with the computed value of the variable (e.g. what shift-clicking a variable swatch does right now). I'd like to keep the current shift-click functionality, as that matches the same "feel" as other swatches (e.g. shift-click a color swatch to change the format).
> (In reply to Nikita Vasilyev from comment #4) > > Created attachment 364775 [details] > > [Image] go-to arrow inside of the variable popover > > > > What if we move the arrow inside of the variable popover? It would reduce the clutter a bit. > > That makes me think that the clicking arrow will replace the CSS text with > the computed value of the variable (e.g. what shift-clicking a variable > swatch does right now). I'd like to keep the current shift-click > functionality, as that matches the same "feel" as other swatches (e.g. > shift-click a color swatch to change the format). Shift-click is not easy to discover. It would be okay to keep but I agree with Nikita that we should have some easy way to inline-replace from the popover. That was actually my first thought for this.
Comment on attachment 364758 [details] Patch i like it in the popover better.
(In reply to Joseph Pecoraro from comment #6) > Shift-click is not easy to discover. It would be okay to keep but I agree with Nikita that we should have some easy way to inline-replace from the popover. That was actually my first thought for this. (In reply to Timothy Hatcher from comment #7) > i like it in the popover better. So we have three types of interactions with a CSS variable: 1) show current value (currently done by clicking the [=] swatch) 2) replace with current value (currently done by shift-clicking the [=] swatch) 3) go to variable declaration (needs UI) All swatches have #1, some swatches (e.g. colors) have #2, but the only place that has #3 is the Computed tab, and that's done by putting a go-to arrow right after the value (it's also done in the Rules sidebar when enabling the "Show Jump to Effective Property Button" experimental setting). Putting a go-to arrow inside the popover is a very new UI, and could be confusing (if I were to look at that, having an arrow next to a "computed" value that's pointing directly at the declared value makes me think that it's going to do #2, not #3). Additionally, the functionality in that case wouldn't match as expected, as the popover shows the computed value, while the go-to arrow would jump to the variable's declaration (which may itself be defined as the value of another variable). It also seems weird to have a shift-click potentially cause scrolling, rather than some sort of "alternate" functionality (e.g. switching color formats). Given that the "Show Jump to Effective Property Button" is experimental, I'm inclined to prefer this approach as it makes the most sense in my mind (having the go-to arrow be right next to the value which we are going to jump from is our usual UI). One thing's for sure, being able to jump to a variable's definition is something that I've wished I could do many many times, so this is something that would be super useful.
Comment on attachment 364758 [details] Patch Marking as r? (so it's more visible), but cq- until thee is a consensus.
Created attachment 376558 [details] Patch
Created attachment 376559 [details] [Image] After Patch is applied
> (In reply to Timothy Hatcher from comment #7) > > i like it in the popover better. I also think putting it into the popover would be better. At the very least, having a ContextMenu on to do all the possible desired actions should be agreeable to everyone. • Show resolved value • Replace content with resolved value • Jump to variable definition if available > So we have three types of interactions with a CSS variable: > 1) show current value (currently done by clicking the [=] swatch) > 2) replace with current value (currently done by shift-clicking the [=] swatch) > 3) go to variable declaration (needs UI) > > All swatches have #1, some swatches (e.g. colors) have #2, but the only > place that has #3 is the Computed tab, and that's done by putting a go-to > arrow right after the value (it's also done in the Rules sidebar when > enabling the "Show Jump to Effective Property Button" experimental setting). I just tried this UI and it has problems. The feature is useful but I don't think it is UI to follow. • It is glitchy to try and click (you need to approach the button from the left otherwise it disappears when you mouse over the exact spot it should be). • It is at the end of the property so it bounces all over the place as you mouse over varying width properties. Making it even harder to use. • Being at the end it can cause a wrap (or worse is clipped) when appending it doesn't fit on the page. Compare this with the simplicity and consistency of the enable/disable checkbox in styles. > Putting a go-to arrow inside the popover is a very new UI, and could be > confusing (if I were to look at that, having an arrow next to a "computed" > value that's pointing directly at the declared value makes me think that > it's going to do #2, not #3). I'm not sure this is as confusing as you think. Each `var(<variable>, <fallback>)` already gets 1 icon associated with it. Most people will associate this 1 icon with the <variable> and that works. The optional fallback pattern is probably quite rare, but even when it is available it doesn't make this association of the icon association any worse. I agree the arrow in the popover could be slightly confusing. Ideally we'd want to present both options in the popover (replace, and go-to). By using the default go-to arrow the user should have in mind that it will jump to some place. Maybe we can come up with a better popover that allows all the option. I'm not sure we have an icon that describes "replace" or "overwrite". > Additionally, the functionality in that case > wouldn't match as expected, as the popover shows the computed value, while > the go-to arrow would jump to the variable's declaration (which may itself > be defined as the value of another variable). It also seems weird to have a > shift-click potentially cause scrolling, rather than some sort of > "alternate" functionality (e.g. switching color formats). Shift click to replace contents very closely aligns with the shift click on a swatch to switch color formats. That has the same potential drawbacks but doesn't seem problematic in practice. Likewise this is just a power-user feature. I always use the ContextMenu because that gets me exactly what I want very quickly. > One thing's for sure, being able to jump to a variable's definition is > something that I've wished I could do many many times, so this is something > that would be super useful. I think we are all in agreement that this is useful! I think where we disagree is whether or not adding an additional icon to save one click is worth the additional clutter. Sprinkling icons in the style rule text in years past caused clutter and visual noise. I'm loathe to reintroduce noise; especially when there is already an icon! Additionally, since users must click in the Styles sidebar to make edits, having an embedded go-to icon in the clickable region can cause accidental jumps. If a user intends to edit the variable name and accidentally clicks the go-to arrow, then they are suddenly jumped to a different place. I don't think we do that in any of our *editable* regions and would argue against that.
(In reply to Joseph Pecoraro from comment #12) > At the very least, having a ContextMenu on to do all the possible desired actions should be agreeable to everyone. Agreed. > • Show resolved value This seems slightly weird to me, as that's the default click behavior. None of our other swatches have a similar concept (e.g. the color swatch doesn't have a "Show Color Picker" context menu item). > • Replace content with resolved value > • Jump to variable definition if available At the very least, both of these. > > So we have three types of interactions with a CSS variable: > > 1) show current value (currently done by clicking the [=] swatch) > > 2) replace with current value (currently done by shift-clicking the [=] swatch) > > 3) go to variable declaration (needs UI) > > > > All swatches have #1, some swatches (e.g. colors) have #2, but the only place that has #3 is the Computed tab, and that's done by putting a go-to arrow right after the value (it's also done in the Rules sidebar when enabling the "Show Jump to Effective Property Button" experimental setting). > > I just tried this UI and it has problems. The feature is useful but I don't think it is UI to follow. > > • It is glitchy to try and click (you need to approach the button from the left otherwise it disappears when you mouse over the exact spot it should be). > • It is at the end of the property so it bounces all over the place as you mouse over varying width properties. Making it even harder to use. > • Being at the end it can cause a wrap (or worse is clipped) when appending it doesn't fit on the page. > > Compare this with the simplicity and consistency of the enable/disable checkbox in styles. When you say "this UI", are you referring to the "Show Jump to Effective Property Button" experimental feature or to attachment 376558 [details]? If the former, I agree, and I'd like to see another design before it becomes non-experimental. If the latter, I'm confused a bit as I changed the interaction in attachment 376558 [details] to be that the go-to arrow for variable declarations (specifically the `--xyz` part) is _always_ visible (just like the variable swatch). > > Putting a go-to arrow inside the popover is a very new UI, and could be confusing (if I were to look at that, having an arrow next to a "computed" value that's pointing directly at the declared value makes me think that it's going to do #2, not #3). > > I'm not sure this is as confusing as you think. Each `var(<variable>, <fallback>)` already gets 1 icon associated with it. Most people will associate this 1 icon with the <variable> and that works. The optional fallback pattern is probably quite rare, but even when it is available it doesn't make this association of the icon association any worse. That's true, but I'd draw a distinction between the `var()` and the `--xyz`, as this feature is focusing more on the latter and jumping to its definition (the fallback value is basically irrelevant here, unless it's yet another variable which would have it's own go-to arrow). > I agree the arrow in the popover could be slightly confusing. Ideally we'd want to present both options in the popover (replace, and go-to). By using the default go-to arrow the user should have in mind that it will jump to some place. Maybe we can come up with a better popover that allows all the option. I'm not sure we have an icon that describes "replace" or "overwrite". In my mind, seeing an arrow in a popover that points from one piece of text to another (in this case, from the resolved value to the `var()`) would make me thing "clicking this will move the text from before the arrow to override the text after the arrow" (e.g. a -> b). > I think where we disagree is whether or not adding an additional icon to > save one click is worth the additional clutter. > > Sprinkling icons in the style rule text in years past caused clutter and > visual noise. I'm loathe to reintroduce noise; especially when there is > already an icon! > > Additionally, since users must click in the Styles sidebar to make edits, having an embedded go-to icon in the clickable region can cause accidental jumps. If a user intends to edit the variable name and accidentally clicks the go-to arrow, then they are suddenly jumped to a different place. I don't think we do that in any of our *editable* regions and would argue against that. I agree that we should avoid clutter, but I also think that CSS variables are a very unique case that warrants some different considerations. Before CSS variables, there was no way to share a value between two properties. The closest you could get was to add additional selectors to the same rule. Even then, no matter what selectors you used, the value for the property was _constant_ and would _never_ change. CSS variables are the first user defined situation (not including `calc()`) in which the value of a property can change _without_ the actual source text of that property also changing. Because they are defined elsewhere, often the most common part of any workflow involving CSS variables is going to the actual location in the source code for the CSS variable to edit it there instead so that _all_ usages of that variable see the effect, rather than replacing the `var()` itself with the desired "static" content. Because of this, I think `--xyz` warrants some special attention/consideration above and beyond what we normally do for other CSS values. Ever since Web Inspector transitioned to using CSS variables, and especially after we started supporting dark mode, I often find myself jumping between the style of the particular piece of UI I'm building and Variables.css in order to make sure I'm using the right style, or to tweak the color of an existing/added CSS variable a bit. In those cases, even two clicks would get annoying. My ideal workflow would be as fast and straightforward as possible. I think attachment 376558 [details] is a good compromise because I don't think developers are likely to edit `var()` as often as they'd want to edit the underlying `--xyz`, so even though it _may_ make it _slightly_ more awkward to enter editing mode due to the positioning of the go-to arrow, the convenience of a single click to jump to the `--xyz` is worth it.
> > > [...] that's done by putting a go-to arrow right after the value (it's also done in the Rules sidebar when enabling the "Show Jump to Effective Property Button" experimental setting). > > > > I just tried this UI and it has problems. The feature is useful but I don't think it is UI to follow. > > > > • It is glitchy to try and click (you need to approach the button from the left otherwise it disappears when you mouse over the exact spot it should be). > > • It is at the end of the property so it bounces all over the place as you mouse over varying width properties. Making it even harder to use. > > • Being at the end it can cause a wrap (or worse is clipped) when appending it doesn't fit on the page. > > > > Compare this with the simplicity and consistency of the enable/disable checkbox in styles. > When you say "this UI", are you referring to the "Show Jump to Effective > Property Button" experimental feature or to attachment 376558 [details]? If > the former, I agree, and I'd like to see another design before it becomes > non-experimental. Yes, the former. That was why I responded to that part of your paragraph. > > > Putting a go-to arrow inside the popover is a very new UI, and could be confusing (if I were to look at that, having an arrow next to a "computed" value that's pointing directly at the declared value makes me think that it's going to do #2, not #3). > > > > I'm not sure this is as confusing as you think. Each `var(<variable>, <fallback>)` already gets 1 icon associated with it. Most people will associate this 1 icon with the <variable> and that works. The optional fallback pattern is probably quite rare, but even when it is available it doesn't make this association of the icon association any worse. > That's true, but I'd draw a distinction between the `var()` and the `--xyz`, I don't understand. Is there ever a case where the [=] icon does not directly correlate with a single --xyz? It must of course also be limited to `var()` functions as that is the only place where a --xyz variable can actually be materialized. My understanding is that today, the [=] icon is always placed a few characters before a --xyz, and always 1-to-1. > > I think where we disagree is whether or not adding an additional icon to > > save one click is worth the additional clutter. > > > > Sprinkling icons in the style rule text in years past caused clutter and > > visual noise. I'm loathe to reintroduce noise; especially when there is > > already an icon! You didn't address this concern, which is still where I think we disagree. > In those cases, even two clicks would get annoying. Do you feel the same way about color swatches?
(In reply to Joseph Pecoraro from comment #14) > > > > [...] that's done by putting a go-to arrow right after the value (it's also done in the Rules sidebar when enabling the "Show Jump to Effective Property Button" experimental setting). > > > > > > I just tried this UI and it has problems. The feature is useful but I don't think it is UI to follow. > > > > > > • It is glitchy to try and click (you need to approach the button from the left otherwise it disappears when you mouse over the exact spot it should be). > > > • It is at the end of the property so it bounces all over the place as you mouse over varying width properties. Making it even harder to use. > > > • Being at the end it can cause a wrap (or worse is clipped) when appending it doesn't fit on the page. > > > > > > Compare this with the simplicity and consistency of the enable/disable checkbox in styles. > > When you say "this UI", are you referring to the "Show Jump to Effective Property Button" experimental feature or to attachment 376558 [details]? If the former, I agree, and I'd like to see another design before it becomes non-experimental. > > Yes, the former. That was why I responded to that part of your paragraph. Ah, understood. Yes, I agree that the "Show Jump to Effective Property Button" is a very awkward and even intrusive UI. I think it should be rethought. > > > > Putting a go-to arrow inside the popover is a very new UI, and could be confusing (if I were to look at that, having an arrow next to a "computed" value that's pointing directly at the declared value makes me think that it's going to do #2, not #3). > > > > > > I'm not sure this is as confusing as you think. Each `var(<variable>, <fallback>)` already gets 1 icon associated with it. Most people will associate this 1 icon with the <variable> and that works. The optional fallback pattern is probably quite rare, but even when it is available it doesn't make this association of the icon association any worse. > > That's true, but I'd draw a distinction between the `var()` and the `--xyz`, > > I don't understand. Is there ever a case where the [=] icon does not directly correlate with a single --xyz? It must of course also be limited to `var()` functions as that is the only place where a --xyz variable can actually be materialized. > > My understanding is that today, the [=] icon is always placed a few characters before a --xyz, and always 1-to-1. Currently, the [=] is placed before the `var(`, NOT the `--xyz`. The [=] is really more of a swatch for the `var()` function, NOT the `--xyz` variable itself, as it also evaluates the fallback value (and follows any variable chain). If there's no fallback inside the `var()`, then yes, technically the [=] is 1-to-1 with BOTH the `var()` and the `--xyz`. This patch adds a guaranteed 1-to-1 to the `--xyz` variable itself, regardless of whether it's used as the primary value in the `var()` or as part of a fallback (e.g. `var(--a *, var(--b *))` where "*" would be the go-to arrow). > > > I think where we disagree is whether or not adding an additional icon to save one click is worth the additional clutter. > > > > > > Sprinkling icons in the style rule text in years past caused clutter and visual noise. I'm loathe to reintroduce noise; especially when there is already an icon! > > You didn't address this concern, which is still where I think we disagree. My response to that was included as part of my response to the paragraph after it. CSS variables are such a unique situation that I think they're possibly worth introducing UI specially for it. What's your response to my reasoning? > > In those cases, even two clicks would get annoying. > > Do you feel the same way about color swatches? The "action" of a color swatch is _very_ different than the action of the proposed CSS variable go-to arrow added by this patch. A color swatch's action is intended to be a "drill deeper" or "edit" action, which by definition requires more than one thought/interaction (althogh I do like the idea of having a color swatch that worked almost like a <select> on mousedown, showing a color wheel under the mouse). The proposed `--xyz` variable go-to arrow action is a _single_ action that has 0 "followup", as it's a pure context switch. Imagine if you had to click twice (not double click, but two separate clicks in two different locations) in order to navigate with an <a> from one page to another. That's the type of action that this UI adds. The reason it's special is because of the fact that `--xyz` variables in effect have _two_ linkages: 1. the location of the `...: var(--xyz);` in source code 2. the location of the `--xyz: ...;` in source code We already have a way to jump to 1, which is holding down command (⌘) and clicking. We don't have _any_ way to jump to 2. This patch adds that.
I'd like to revive the discussion on this enhancement, as I've been doing some work recently that would've been made _much_ easier by having a go-to to quickly jump to the declaration of an `--xyz` variable inside a `var()`. Given the lengthy discussion, unless anyone has any objections, I'm going to land this behind an experimental feature flag so that people can try living with it without it being on by default. I'll give it a week so anyone has a chance to respond :)
Created attachment 396151 [details] Patch
Comment on attachment 396151 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396151&action=review > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:794 > + arrowElement.title = WI.UIString("Go to variable"); Go to variable declaration?
Committed r259929: <https://trac.webkit.org/changeset/259929> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396151 [details].