886019 - Need a better icon for .variables-view-delete

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Victor Porof [:vporof][:vp]]

Attachment: after

For some reason (after bug 873848) the existing icon was removed and we ended up using close-dark.png as a "delete icon" for items in the variables view.

The previous asset was gray (just like the Firefox tabs close button on OS X), and the current "blurred red" looks really bad IMHO, especially because we play with its opacity.

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: before

Comment 2

[Victor Porof [:vporof][:vp]]

What do you think shorlander? Should we just revert to using the previous icon or do you have something special in mind?

Comment 3

[Girish Sharma [:Optimizer]]

Attachment: revert to correct close icon for osx

My bad for not realizing that the tabview close icons are different on OSX.

Reverting to the correct icon now.

Comment 4

[Girish Sharma [:Optimizer]]

Comment on attachment 766299 [details]
revert to correct close icon for osx

wrong patch.

Comment 5

[Girish Sharma [:Optimizer]]

Victor says he's taking care of this.

Comment 6

[Victor Porof [:vporof][:vp]]

Attachment: v1

Comment 7

[Mihai Sucan [:msucan]]

Comment on attachment 766306 [details] [diff] [review]
v1

Review of attachment 766306 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good on Linux. I just want to understand the js changes you did, see the question below.

::: browser/devtools/debugger/debugger-panes.js
@@ +1392,5 @@
>      closeNode.addEventListener("click", this._onClose, false);
>      inputNode.addEventListener("blur", this._onBlur, false);
>      inputNode.addEventListener("keypress", this._onKeyPress, false);
>  
> +    aElementNode.className = "dbg-expression";

why is this change needed?

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +2372,5 @@
>      if (ownerView.delete) {
>        if (!this._isUndefined || !(ownerView.getter && ownerView.setter)) {
>          let deleteNode = this._deleteNode = this.document.createElement("toolbarbutton");
>          deleteNode.className = "plain variables-view-delete";
> +        deleteNode.setAttribute("ordinal", 2);

and this?

Comment 8

[Victor Porof [:vporof][:vp]]

(In reply to Mihai Sucan [:msucan] from comment #7)
> 
> ::: browser/devtools/debugger/debugger-panes.js
> @@ +1392,5 @@
> >      closeNode.addEventListener("click", this._onClose, false);
> >      inputNode.addEventListener("blur", this._onBlur, false);
> >      inputNode.addEventListener("keypress", this._onKeyPress, false);
> >  
> > +    aElementNode.className = "dbg-expression";
> 
> why is this change needed?

The previous CSS selector required a .title before the variables view delete node for it to be hidden when not hovered. That's not required anymore, so I removed this extra now redundant class.

> ::: browser/devtools/shared/widgets/VariablesView.jsm
> @@ +2372,5 @@
> >      if (ownerView.delete) {
> >        if (!this._isUndefined || !(ownerView.getter && ownerView.setter)) {
> >          let deleteNode = this._deleteNode = this.document.createElement("toolbarbutton");
> >          deleteNode.className = "plain variables-view-delete";
> > +        deleteNode.setAttribute("ordinal", 2);
> 
> and this?

Make sure the delete button always appears at the end. Right now when locks, F/S/N indicators etc. were present, the delete button position was not always the same for all items.

Comment 9

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/e557dc258388

Comment 10

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/mozilla-central/rev/e557dc258388