1302691 - missing [e-mail] and [e-mail] referenced from chrome://devtools/skin/toolbox.css

Status: RESOLVED DUPLICATE of bug 1450624

(DevTools :: Framework, defect, P2)

Description

[Florian Quèze [:florian]]

The test I'm writing in bug 1302570 identified these 2 issues:

missing chrome://devtools/skin/images/[e-mail] referenced from chrome://devtools/skin/toolbox.css
missing chrome://devtools/skin/images/[e-mail] referenced from chrome://devtools/skin/toolbox.css

These 2 files have been removed by bug 1225184 but are still used by the following CSS code:

http://searchfox.org/mozilla-central/rev/6b94deded39a868f248a492e74e05733d6c4ed48/devtools/client/themes/toolbox.css#258

#toolbox-dock-bottom-minimize::before {
  background-image: url("chrome://devtools/skin/images/[e-mail]");
}

#toolbox-dock-bottom-minimize.minimized::before {
  background-image: url("chrome://devtools/skin/images/[e-mail]");
}

Comment 1

[Drew Willcoxon :adw]

Florian, did you mean to file this in Firefox::Search?

Comment 2

[Florian Quèze [:florian]]

(In reply to Drew Willcoxon :adw from comment #1)
> Florian, did you mean to file this in Firefox::Search?

No, thanks for fixing it! (my awesomebar completed enter_bug.cgi to enter_bug.cgi?product=Firefox&component=Search and I forgot to fix the component before submitting)

Comment 3

[Brian Grinstead [:bgrins]]

Helen and Patrick, there's a 'minimize' button in the toolbox but it's always hidden.  It's perma-hidden right now with a Comment about Bug 1173849: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox.js#787-789).  We still have some CSS referencing an image that's been removed. I can think of a few options:

1) Remove the feature entirely since it's always hidden
2) Remove the CSS that's referencing the image (so we'd need to figure out styling later on if/when it's enabled)
3) Come up with an icon for it

Any opinions?

Comment 4

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> 1) Remove the feature entirely since it's always hidden
> 2) Remove the CSS that's referencing the image (so we'd need to figure out
> styling later on if/when it's enabled)
> 3) Come up with an icon for it
Time flies, we said we'd look at enabling this feature again in the future once we figured out a better icon for it and a better way to manage toolbox icons. Looks like we haven't done any of this yet.
I think the toolbox minimize feature is nice and may be useful to people, so if we can agree now that we want to do it, and find someone who will, then that'd be great. If, however, this feature isn't a priority now from a UX standpoint, then let's just remove it completely.
In other words: I let Helen choose :)

Comment 5

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> Helen and Patrick, there's a 'minimize' button in the toolbox but it's
> always hidden.  It's perma-hidden right now with a Comment about Bug
> 1173849:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/
> toolbox.js#787-789).  We still have some CSS referencing an image that's
> been removed. I can think of a few options:
> 
> 1) Remove the feature entirely since it's always hidden
> 2) Remove the CSS that's referencing the image (so we'd need to figure out
> styling later on if/when it's enabled)
> 3) Come up with an icon for it
> 
> Any opinions?

From the code alone, I'm not sure I understand what this invisible minimize button is referencing, and as such I can't offer much guidance for how to clean up the code :). Is this meant to replicate MacOS' yellow minimize button? Was this a minimized devtools palette meant to be more out of the way?

Generally I agree with Patrick's points above being blockers for any follow-up work continuing beyond fixing the test: the toolbar work needs to be completed before we can put more stuff into them. For that reason I can say from a UX standpoint, it isn't being prioritized.

Knowing that, there is still a code-cleanliness question. Since the code is currently blocked on other work, I would do the minimum that's viably needed and reference the test-breaking CSS to one of our dock-*.svgs, with a note that it is temporary until a proper UI is created for it.

Comment 6

[Tim Nguyen :ntim]

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #5)
> (In reply to Brian Grinstead [:bgrins] from comment #3)
> > Helen and Patrick, there's a 'minimize' button in the toolbox but it's
> > always hidden.  It's perma-hidden right now with a Comment about Bug
> > 1173849:
> > https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/
> > toolbox.js#787-789).  We still have some CSS referencing an image that's
> > been removed. I can think of a few options:
> > 
> > 1) Remove the feature entirely since it's always hidden
> > 2) Remove the CSS that's referencing the image (so we'd need to figure out
> > styling later on if/when it's enabled)
> > 3) Come up with an icon for it
> > 
> > Any opinions?
> 
> From the code alone, I'm not sure I understand what this invisible minimize
> button is referencing, and as such I can't offer much guidance for how to
> clean up the code :). Is this meant to replicate MacOS' yellow minimize
> button? Was this a minimized devtools palette meant to be more out of the
> way?
Helen, the minimize button was added in bug 867838, and was hidden in bug 1177463. It allows you to snap the toolbox to the tab bar only. Here are some screencasts:
https://dl.dropboxusercontent.com/u/714210/toolbox-minimize.gif
https://dl.dropboxusercontent.com/u/1212936/lice/toolbox-min-demo.gif

> Generally I agree with Patrick's points above being blockers for any
> follow-up work continuing beyond fixing the test: the toolbar work needs to
> be completed before we can put more stuff into them. For that reason I can
> say from a UX standpoint, it isn't being prioritized.
I think this was a really useful feature, and was one of our most popular requests on the old uservoice site. It just needed a bit of polishing (and ux review) to be complete, so I think it should be a trivial amount of work to re-enable it. Helen, any chance we can re-prioritize it ?

> Knowing that, there is still a code-cleanliness question. Since the code is
> currently blocked on other work, I would do the minimum that's viably needed
> and reference the test-breaking CSS to one of our dock-*.svgs, with a note
> that it is temporary until a proper UI is created for it.
+1 to this solution. We can change the references to existing images as placeholders until the feature gets re-enabled.

Comment 7

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #6)
> > Generally I agree with Patrick's points above being blockers for any
> > follow-up work continuing beyond fixing the test: the toolbar work needs to
> > be completed before we can put more stuff into them. For that reason I can
> > say from a UX standpoint, it isn't being prioritized.
> I think this was a really useful feature, and was one of our most popular
> requests on the old uservoice site. It just needed a bit of polishing (and
> ux review) to be complete, so I think it should be a trivial amount of work
> to re-enable it. Helen, any chance we can re-prioritize it ?
Tim, what was it blocked on? Patrick says in bug 1177463 that not everyone was in agreement about the UI; do you know why that was?

From the gifs alone it seems useful enough, but I'd love some more background.

Comment 8

[Tim Nguyen :ntim]

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #7)
> (In reply to Tim Nguyen :ntim (use needinfo?) from comment #6)
> > > Generally I agree with Patrick's points above being blockers for any
> > > follow-up work continuing beyond fixing the test: the toolbar work needs to
> > > be completed before we can put more stuff into them. For that reason I can
> > > say from a UX standpoint, it isn't being prioritized.
> > I think this was a really useful feature, and was one of our most popular
> > requests on the old uservoice site. It just needed a bit of polishing (and
> > ux review) to be complete, so I think it should be a trivial amount of work
> > to re-enable it. Helen, any chance we can re-prioritize it ?
> Tim, what was it blocked on? Patrick says in bug 1177463 that not everyone
> was in agreement about the UI; do you know why that was?
From my understanding, the feature was disabled because it didn't get input from UX (see https://bugzilla.mozilla.org/show_bug.cgi?id=867838#c45 ), and also because it needed polish (bug 1173849, bug 1178216). I don't think I've heard any specific disagreement though, maybe Patrick will know ?

Comment 9

[Patrick Brosset <:pbro>]

The only thing that prevented it from landing at the time was that the minimize button was placed in the toolbox toolbar, where we already have many things. So, we thought adding yet another icon up there could confuse people and take up important space.

Since we didn't resolve this issue back then, we thought it'd be better to just not land it rather than land it and then change it again later.

So, even if the feature seemed useful, it was never enough of a priority for us to revisit it since then.

So, bottom line is: if someone has time to spend on the UX/UI for this (to re-arrange the docking/settings/close icons maybe?) and then rebase the code and land it, that'd be great. But I suspect no one in the devtools team will have time to work on this now, knowing that this isn't one of our highest priorities.

So, to come back to comment 3: if this bug is a priority and no one has time to land the minimize feature right now, let's just delete the remaining code altogether. When we have time to work on this, we can always look at the patch in the bug.

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Fixed in bug 1450624.