Need a better icon for .variables-view-delete

RESOLVED FIXED in Firefox 24

Status

RESOLVED FIXED
6 years ago
8 months ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

unspecified
Firefox 24

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Created attachment 766296 [details]
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.
(Assignee)

Comment 1

6 years ago
Created attachment 766297 [details]
before
(Assignee)

Comment 2

6 years ago
What do you think shorlander? Should we just revert to using the previous icon or do you have something special in mind?
Flags: needinfo?(shorlander)
Created attachment 766299 [details]
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.
Assignee: nobody → scrapmachines
Status: NEW → ASSIGNED
Attachment #766299 - Flags: review?(vporof)
Comment on attachment 766299 [details]
revert to correct close icon for osx

wrong patch.
Attachment #766299 - Attachment is obsolete: true
Attachment #766299 - Attachment is patch: false
Attachment #766299 - Flags: review?(vporof)
Victor says he's taking care of this.
Assignee: scrapmachines → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 6

6 years ago
Created attachment 766306 [details] [diff] [review]
v1
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attachment #766306 - Flags: review?(mihai.sucan)
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?
Attachment #766306 - Flags: review?(mihai.sucan) → review+
(Assignee)

Comment 8

6 years ago
(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.
(Assignee)

Updated

6 years ago
Flags: needinfo?(shorlander)
(Assignee)

Comment 10

6 years ago
https://hg.mozilla.org/mozilla-central/rev/e557dc258388
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Target Milestone: --- → Firefox 24

Updated

8 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.