When a variable in scope overrides another variable from a parent scope, users often get confused. It would be nice to indicate that with a strikethrough font, like the rule view does.

I got confuzzeled today!

Moving stuff to the Object Inspector component. Filter on COSMICMIGRAINE.

This works. I'd love to hear some opinions on whether this feels and looks okay. Only needs a small test.

Comment on attachment 8344175 [details] [diff] [review] v1 Naturally I found some bugs. I'm just going to write those text and ask for review directly.

Looks like this if anyone's interested to offer UI feedback.

Looks nice, but I'm not sure if the different background helps de-emphasize or highlight even more the overridden values. Have you tried changing the text colour instead?

Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=0f8eab080b13

Comment on attachment 8345797 [details] [diff] [review] v2 Review of attachment 8345797 [details] [diff] [review]: ----------------------------------------------------------------- I'm vary happy about this, I think it's these little things that make people love our debugger! Note the proper term is actually "overridden" not "overwritten", since it's a different variable hiding this one in the name lookup process, rather than a different value written to the same variable. Another term I've seen used for a similar case in Java is "shadowing", but "overriding" is more broadly used (even in Java). I'm also not sure we actually need the "overridden" label in the tooltip. I would argue that its usefulness is marginal, as the overriding can be already observed by the strike-through text, and it makes the tooltip convoluted. If we do keep it, I think we should move it past the property descriptors, next to the WebIDL label. Maybe even use the same green color to indicate its different nature. r=me with comments addressed. To be clear, I only feel strongly about overwritten -> overridden and allowing edits on overridden values, unless of course you can convince me otherwise :-) ::: browser/devtools/shared/widgets/VariablesView.jsm @@ +625,5 @@ > /** > + * Gets the scope owning a Variable or Property. > + * > + * @param Variable | Property > + * The variable or property to retrieven the owner scope for. "retrieve" @@ +2977,5 @@ > + return prevItem ? prevItem._isExpanded : false; > +}; > + > +/** > + * Checks if the an item's displayed value (a representation of the grip) -the @@ +2997,5 @@ > + return prevItem ? prevItem._valueString != aItem._valueString : false; > +}; > + > +/** > + * Checks if the an item was previously expanded, if it existed in a s/expanded/overridden/ ::: browser/themes/linux/devtools/widgets.css @@ +485,5 @@ > + transition-duration: .4s; > +} > + > +.variable-or-property[overwritten] { > + background: rgba(128,128,128,0.05); I don't think a slightly different background is useful (I instinctively interpret it as a new scope), but I'm not going to insist if you really like it. It's rather subtle. @@ +493,5 @@ > + /* Cross out the title for this variable and all child properties. */ > + font-style: italic; > + text-decoration: line-through; > + border-bottom: none !important; > + color: rgba(128,128,128,0.9); Color and underline are still useful visual cues, but I assume they would hurt readability combined with strike-through, so that's fine. @@ +505,5 @@ > +} > + > +.variable-or-property[overwritten] .title > .value { > + /* Disallow editing this variable and all child properties. */ > + pointer-events: none; What is the reasoning behind this? If the debugger can update these values just fine, why should we limit what the user can do with them? Since we are already clearly indicating that these values are not going to be used in the current context, modifying them and observing the effects on the program should at least help educate the user about overriding. @@ +595,5 @@ > } > > +.variable-or-property[overwritten] > tooltip > label.overwritten { > + -moz-padding-end: 4px; > + -moz-border-end: 1px dotted #000; This border and the WebIDL one are invisible on stock Ubuntu configuration, due to the black default tooltip background. I think you suggested using custom tooltips in the past, which might be the best path forward here. I think this can be followup material though.

(In reply to Panos Astithas [:past] from comment #9) Thanks for the review Panos! > Note the proper term is actually "overridden" not "overwritten", since it's > a different variable hiding this one in the name lookup process, rather than > a different value written to the same variable. Another term I've seen used > for a similar case in Java is "shadowing", but "overriding" is more broadly > used (even in Java). > You're right, I'll use overridden. > I'm also not sure we actually need the "overridden" label in the tooltip. I > would argue that its usefulness is marginal, as the overriding can be > already observed by the strike-through text, and it makes the tooltip > convoluted. If we do keep it, I think we should move it past the property > descriptors, next to the WebIDL label. Maybe even use the same green color > to indicate its different nature. Since no other debugger that I've ever used does this in its object inspector (strike-through overridden variables), I think a marginal percentage of our users might be confused as to what the different styling represents. Since tooltips are descriptive and non-invasive, I think it's better to show "overridden" if necessary. > > ::: browser/themes/linux/devtools/widgets.css > @@ +485,5 @@ > > + transition-duration: .4s; > > +} > > + > > +.variable-or-property[overwritten] { > > + background: rgba(128,128,128,0.05); > > I don't think a slightly different background is useful (I instinctively > interpret it as a new scope), but I'm not going to insist if you really like > it. It's rather subtle. > I like it. I'll meet you in the middle and make it even more subtle. > > @@ +505,5 @@ > > +} > > + > > +.variable-or-property[overwritten] .title > .value { > > + /* Disallow editing this variable and all child properties. */ > > + pointer-events: none; > > What is the reasoning behind this? If the debugger can update these values > just fine, why should we limit what the user can do with them? > > Since we are already clearly indicating that these values are not going to > be used in the current context, modifying them and observing the effects on > the program should at least help educate the user about overriding. I'm torn about this a bit. You're right about educating users, but these are variables that you don't actually change when editing. Performing an edit would only update the value in an upper scope, not in the one you're editing, so we're lying about editing that *specific* variable in that *specific* scope. At least that's my reasoning. If you still think I'm worrying for nothing, I'll allow editing.

(In reply to Victor Porof [:vp] from comment #10) > (In reply to Panos Astithas [:past] from comment #9) > > I'm also not sure we actually need the "overridden" label in the tooltip. I > > would argue that its usefulness is marginal, as the overriding can be > > already observed by the strike-through text, and it makes the tooltip > > convoluted. If we do keep it, I think we should move it past the property > > descriptors, next to the WebIDL label. Maybe even use the same green color > > to indicate its different nature. > > Since no other debugger that I've ever used does this in its object > inspector (strike-through overridden variables), I think a marginal > percentage of our users might be confused as to what the different styling > represents. Since tooltips are descriptive and non-invasive, I think it's > better to show "overridden" if necessary. Note that using strike-through for overridden values already happens in the page inspector, so I would expect our users to already be familiar with it. Furthermore, the survey data that I've seen put the page inspector in the #1 or #2 place in terms of tool popularity, so I would think that JS devs who have never used it should be a small minority. > > @@ +505,5 @@ > > > +} > > > + > > > +.variable-or-property[overwritten] .title > .value { > > > + /* Disallow editing this variable and all child properties. */ > > > + pointer-events: none; > > > > What is the reasoning behind this? If the debugger can update these values > > just fine, why should we limit what the user can do with them? > > > > Since we are already clearly indicating that these values are not going to > > be used in the current context, modifying them and observing the effects on > > the program should at least help educate the user about overriding. > > I'm torn about this a bit. You're right about educating users, but these are > variables that you don't actually change when editing. Performing an edit > would only update the value in an upper scope, not in the one you're > editing, so we're lying about editing that *specific* variable in that > *specific* scope. > > At least that's my reasoning. If you still think I'm worrying for nothing, > I'll allow editing. Hmm, you are right about this. I thought we were using the "assign" protocol request to modify Debugger.Environment in those cases, but that hasn't been implemented yet (bug 925786).

(In reply to Panos Astithas [:past] from comment #11) > > Hmm, you are right about this. I thought we were using the "assign" protocol > request to modify Debugger.Environment in those cases, but that hasn't been > implemented yet (bug 925786). Would "assign" cope with side-effects, like when editing a variable and writing "window.foo = 42"? I think that's why we didn't use it in the first place.

"It's complicated" :-) D.E.p.setVariable is safe per spec: https://wiki.mozilla.org/Debugger#Function_Properties_of_the_Debugger.Environment_Prototype_Object We can't currently guard against modifying immutable bindings though, from what I see in onAssign.