630128 - Add icon to the 'Move to group' context menu

Status: RESOLVED WONTFIX

(Firefox :: Theme, defect)

Comment 1

[Raymond Lee [:raymondlee]]

See bug 626525#c34

Comment 2

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 3

[Ian Gilman [:iangilman]]

This is outside of my domain. Assigning review to Dolske, as he's been reviewing a number of menu changes.

Comment 4

[Justin Dolske [:Dolske]]

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

If you haven't already, please check builds on windows/OSX/Linux to ensure the icon placement (moz-image-region) is correct.

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: Screenshot of OSX

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: Screenshot of Win XP

Comment 7

[Raymond Lee [:raymondlee]]

Couldn't test it on Linux due to bug 625317

Comment 8

[Dão Gottwald [:dao]]

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

>--- a/browser/base/content/browser.xul
>+++ b/browser/base/content/browser.xul

>-      <menu id="context_tabViewMenu" label="&moveToGroup.label;"
>+      <menu id="context_tabViewMenu" label="&moveToGroup.label;" 

Bogus change, please undo.

>--- a/browser/themes/gnomestripe/browser/browser.css
>+++ b/browser/themes/gnomestripe/browser/browser.css

>+#context_tabViewMenu {
>+  list-style-image: url(chrome://browser/skin/tabview/tabview.png);
>+  -moz-image-region: rect(0, 16px, 16px, 0);
>+}
>+
>+/* download manager button */

There's no heading for each button in this file, so don't add that comment.

Comment 9

[Justin Dolske [:Dolske]]

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

faaborg asked for this in the other bug, but are we sure we want to do this? Seems odd to have the icon for OS X, and it seems to grab a lot of attention on Windows.

Comment 10

[Alex Faaborg [:faaborg] (Firefox UX)]

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

Let's go with the icon only on windows and Linux.

Comment 11

[Raymond Lee [:raymondlee]]

Attachment: v2

Only display icon on windows and linux

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 508801 [details] [diff] [review]
v2

>--- a/browser/themes/gnomestripe/browser/browser.css
>+++ b/browser/themes/gnomestripe/browser/browser.css

>+#context_tabViewMenu > .menu-iconic-left {
>+  visibility: visible;
>+}

You're overriding the icons-in-menus OS setting, don't do this.

Comment 13

[Raymond Lee [:raymondlee]]

Attachment: v3

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 508805 [details] [diff] [review]
v3

I don't like this change, but I'll defer to dolske.

Comment 15

[Alex Faaborg [:faaborg] (Firefox UX)]

Just had a chance to discuss with Dolske and Limi, we all agree that introduce a single icon into the menu is strange (since the overall plan is to give these icons to improve the search time for top items).  We don't have time to deploy icons to top items across all of our submenus, but this is a good menu to start with for Firefox 4.  So let's go ahead and also land icons for Reload and "Close Tab" to this menu as part of the same patch.  That satisfies dolske's concern about making this item extremely special (as opposed to just slightly special), and lets us introduce the design in one of the more dominant context menus in Firefox.

Comment 16

[Raymond Lee [:raymondlee]]

Attachment: v4

Added CSS to show icons for reload and close tab menu items.  In the gnomestripe/browser/browser.css, the reload and close tab menu items are already defined.

See
http://mxr.mozilla.org/mozilla-central/source/browser/themes/gnomestripe/browser/browser.css#1538

However, it doesn't show up in Fx on my linux due to the system setting. See this.  
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/xul.css#364

Comment 17

[Dão Gottwald [:dao]]

Reload and Close Tab don't strike me as particularly important, as they are mostly redundant. I may use Close Tab in some edge cases, but I'm sure I never use Reload. Of course, usage patterns vary. For me however this seems to just further increase the noise level.

Comment 18

[Justin Dolske [:Dolske]]

Fa

Comment 19

[Justin Dolske [:Dolske]]

(wtf)

Faaborg: I've forgotten our previous discussion on this, is there a reason you strongly want this for FF4? It does seem a bit odd to have this be the only context menu with icons, and it's not obvious to me that that price is worth having just to have an icon for Panorama. Let's just leave it iconless for FF4, and then iconify everything consistently for FF5?

Comment 20

[Alex Faaborg [:faaborg] (Firefox UX)]

This was a spin off from bug 626525, details on why we might want this icon for Firefox 4 are here: https://bugzilla.mozilla.org/show_bug.cgi?id=626525#c34 (short version so the user has some basis to figure out how they might be able to enable the command).

but if we don't get the icon landed, it isn't that big of a deal.  Just a nice to have.

Comment 21

[Justin Dolske [:Dolske]]

Ah, so the intention from that bug seems to have been to have Move To Group always present (but sometimes disabled). Instead what landed has it being sometimes hidden (and obviously enabled when visible). :-/ The icon's value was as a clue as to what the disabled menu was for -- makes sense to me now.

So, I'm going to suggest we not take this for FF4. The current net effect would be to just to add icons for the reload/close entries. Most people won't have used Panorama, and so Move To Group is entirely hidden for them (it would be visible if you've used it, but then you don't really need the icon).

The alternative would be to reopen 626525, switch to disabling instead of hiding, and then take this patch. Though at this point I'd kinda be inclined to just cut our risk and not fiddle with this more going into RC (ie, just sort it out fully for FF5).

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

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

(bookkeeping)

Comment 23

[Justin Dolske [:Dolske]]

Per comment 21, wontfix. We'll presumably do this in FF5+ (as part of adding icons to all menus), but let's not use this bug for that.