415543 - "Choose Application…" menu item should have an icon

Status: RESOLVED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, defect)

Description

[Vlado Valastiak [:wladow]]

Attachment: align "Choose application" menu-item correctly

Comment 1

[Vlado Valastiak [:wladow]]

Attachment: screenshot: before -> after

Comment 2

[Asaf Romano (gone)]

Comment on attachment 301278 [details] [diff] [review]
align "Choose application" menu-item correctly

It has no icon, thus the margins should be applied some other way (i would like at how the bookmarks and history menus get this right).

Comment 3

[Vlado Valastiak [:wladow]]

Attachment: set margin and height for menuitem

This patch corrects margin for menuitems in win and gnome and sets menuitem height to be the same with iconic menuitems. No need to edit pinstripe.

Comment 4

[Vlado Valastiak [:wladow]]

Attachment: screenshot: vista and ubuntu

Comment 5

[Asaf Romano (gone)]

Comment on attachment 301356 [details] [diff] [review]
set margin and height for menuitem

r=mano

Comment 6

[Vlado Valastiak [:wladow]]

Comment on attachment 301356 [details] [diff] [review]
set margin and height for menuitem

This does not work for XP correctly, needs to investigate little more what's wrong here.

Comment 7

[Vlado Valastiak [:wladow]]

Attachment: remove iconic class, update menuitem class

This patch solves the problem entirely. It removes "menuitem-iconic" class for iconic handlers menuitems and sets appropriate properties for all the menuitems.

Comment 8

[Asaf Romano (gone)]

I don't think this is right (this is the exact class we should be using for.. iconic menuitems). Practically speaking,  it'd probably break Pinstripe.

Comment 9

[Vlado Valastiak [:wladow]]

I made some tests before attaching the patch with WinVista, WinXP, Ubuntu 7.10 and Mac OS X 10.5 and it seemed to work correctly on every platform. The same approach is used f.e. here: http://lxr.mozilla.org/mozilla/source/browser/components/preferences/handlers.css#50 Similar context menus in Applications prefpane do not use iconic class either.

Comment 10

[Vlado Valastiak [:wladow]]

Mano, pls take a look here: http://lxr.mozilla.org/mozilla/source/toolkit/content/xul.css#833
If I understand this correctly, menuitem-iconic class isn't really needed as menupopup's item is binded as iconic menuitem automatically. That's what I see in DOMi too. If we use menuitem without any specific class, DOMi says it consists of menu-iconic-left box and menu-iconic-text label. If we use menuitem-iconic class, DOMi says it consists of menu-iconic-left box, menu-iconic-text and menu-accel-container box. We don't need to have accel-container. According to this, usage of menuitem-iconic is unnecessary and the last patch could this the issue. Or is there anything that I missed completely? 

Comment 11

[Shawn Wilsher :sdwilsh]

(In reply to comment #10)
> Mano, pls take a look here:
> http://lxr.mozilla.org/mozilla/source/toolkit/content/xul.css#833
> If I understand this correctly, menuitem-iconic class isn't really needed as
> menupopup's item is binded as iconic menuitem automatically. That's what I see
> in DOMi too. If we use menuitem without any specific class, DOMi says it
> consists of menu-iconic-left box and menu-iconic-text label. If we use
> menuitem-iconic class, DOMi says it consists of menu-iconic-left box,
> menu-iconic-text and menu-accel-container box. We don't need to have
> accel-container. According to this, usage of menuitem-iconic is unnecessary and
> the last patch could this the issue. Or is there anything that I missed
> completely? 
Mano - any comments here?  I'd hate for this to miss 3.0, and then miss 3.1 too...

Comment 12

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

To follow up we need to confirm that we are doing this right for every drop down, not this specific drop down.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 302683 [details] [diff] [review]
remove iconic class, update menuitem class

The removal of menuitem-iconic isn't needed, right? As far as I can tell it doesn't change the styling, only whether we get menuitem-iconic or menuitem-iconic-noaccel binding (which makes no difference visually). Seems like just the CSS changes would be sufficient (making menu-icon-left always visible regardless of menuitem type). The changes to menu-iconic-left's padding look arbitrary - are they?

While you're changing these, seems like we should just use the simpler ".menu-iconic-left" selector and avoid the "#handlersMenuList > menupopup > menuitem >", given that there is only one menulist on this page.

Comment 14

[Vlado Valastiak [:wladow]]

(In reply to comment #13)
> (From update of attachment 302683 [details] [diff] [review])
> The removal of menuitem-iconic isn't needed, right? As far as I can tell it
> doesn't change the styling, only whether we get menuitem-iconic or
> menuitem-iconic-noaccel binding (which makes no difference visually). Seems
> like just the CSS changes would be sufficient (making menu-icon-left always
> visible regardless of menuitem type).
No, it does change styling and that's the biggest issue here. A menu-item with this class looks completely different to one without this class. And a menu-item with this class looks differently in various OS versions like Vista Aero, Vista Basic and XP.

I still think attachment 301278 [details] [diff] [review] is the way we should go. It's easy non-risky patch working correctly for all platforms and OS versions. Other ways to fix this sound just like unneeded hackery which does not make us sure, that all the items will look exactly the same on various platforms and OS versions. I've tested various possible solutions with Vista, XP, Ubuntu, Mac OS X, none of them worked as well as this one.

Feel free to take this bug, because I apparently 
don't know how to fix this with css hack only. Note that Vista Aero, Vista Classic, Windows XP need different margin and item height values within one .css file.

Comment 15

[Vlado Valastiak [:wladow]]

Attachment: use icon for Choose Application menuitem

What if we used an icon for this menu-item? The one used in this screenshot is the icon used for similar menu-items in our Applications prefpane.

Comment 16

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

Comment on attachment 362067 [details]
use icon for Choose Application menuitem

Current icons in the application prefpane are kind of messed up. This is actually the icon for "always ask," the choice for "choose application" should be an application window icon.  Details in bug 429725

and this mockup:

https://bug429725.bugzilla.mozilla.org/attachment.cgi?id=316486

Comment 17

[Vlado Valastiak [:wladow]]

Attachment: use icon for the Choose Application menuitem, v2

Use application.png instead. The screenshots taken from Vista, XP, Ubuntu. Currently I have no Mac running machine available for testing, but I suppose that http://mxr.mozilla.org/mozilla1.9.1/source/browser/themes/pinstripe/browser/preferences/application.png will be picked up properly.

Comment 18

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

Comment on attachment 362629 [details]
use icon for the Choose Application menuitem, v2

looks great

Comment 19

[Vlado Valastiak [:wladow]]

Attachment: patch

The patch for the attachment 362629 [details] 'use icon for the Choose Application menuitem, v2'

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 364295 [details] [diff] [review]
patch

The "Choose application" icon is rather ugly on mac, but I suppose that's a separate issue.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Actually, it would be easier for themers if this item was styled using CSS (e.g. using #chooseApplicationMenuItem with list-style-image) rather than hardcoding the "src" in code. Any chance you could investigate that approach?

Comment 22

[Vlado Valastiak [:wladow]]

Attachment: patch, v2

works as expected with vista, xp, ubuntu
needs to be tested with osx

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 367195 [details] [diff] [review]
patch, v2

Works fine on Mac as well, thanks!

Comment 24

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/d1776607d429

Comment 25

[Dão Gottwald [:dao]]

Attachment: linux screenshot

something isn't right here...

Comment 26

[Dão Gottwald [:dao]]

So chrome://browser/skin/preferences/application.png is 15x15 pixels. I suggest you use moz-icon://dummy.exe?size=16 instead. We should probably do that on all platforms, but I'm not sure if dummy.exe works on OS X.

Comment 27

[Vlado Valastiak [:wladow]]

Attachment: ubuntu screenshot

I don't see the issue with
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090315 Minefield/3.2a1pre
hence I can't test the approach you suggested. Pls, could you do that?

Comment 28

[Dão Gottwald [:dao]]

You can test it nonetheless, no?

Comment 29

[Vlado Valastiak [:wladow]]

Oh wait, I'm a little bit confused now: are you really asking me to drop the application.png icon and to use 
> list-style-image: url("moz-icon://dummy.exe?size=16");
instead? Is that acceptable? Or am I completely out of line here?

Comment 30

[Dão Gottwald [:dao]]

Why wouldn't that be acceptable?

Comment 31

[Vlado Valastiak [:wladow]]

Because it's a workaround on how to get an icon. I'd say the purer solution is to fix the icon size in follow-up bug.

Comment 32

[Dão Gottwald [:dao]]

It is not a workaround. We use moz-icon in directory listings and in gnomestripe all over the place. It's the correct way to get an icon from the operating system. In fact, using your own icon when the OS provides it is a workaround.

Comment 33

[Vlado Valastiak [:wladow]]

Attachment: m-c patch, v3

Comment 34

[Vlado Valastiak [:wladow]]

Attachment: use icon for the Choose Application menuitem, v3

Tested with Vista, XP, Ubuntu, no Mac OS available for this purpose, sorry.

Comment 35

[Dão Gottwald [:dao]]

Comment on attachment 367762 [details] [diff] [review]
m-c patch, v3

Sorry, no OS X here either.

Comment 36

[Dão Gottwald [:dao]]

Comment on attachment 367763 [details]
use icon for the Choose Application menuitem, v3

That's strange, I get a real application icon for this on Ubuntu, very similar to our application.png.

Comment 37

[Vlado Valastiak [:wladow]]

It's Ubuntu 7.10, maybe you're using a newer version?

Comment 38

[Dão Gottwald [:dao]]

Yeah, I'm using 8.10.

Comment 39

[:Gavin Sharp [email: [e-mail]]]

This doesn't work on Mac (I get a generic icon), so you should omit the change from pinstripe I suppose.

Comment 40

[Vlado Valastiak [:wladow]]

Attachment: m-c patch, v3.1

Comment 41

[:Gavin Sharp [email: [e-mail]]]

I wonder whether we have bugs for using moz-icon rather than application png elsewhere... or did we add application.png to intentionally be different than the .exe icon?

Comment 42

[Dão Gottwald [:dao]]

I don't know such a bug, but I was going file one.

Comment 43

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

>I wonder whether we have bugs for using moz-icon rather than application png
>elsewhere... or did we add application.png to intentionally be different than
>the .exe icon?

Varies by platform:

Linux: we should try to extract the application icon from the system if possible, since it varies by OS theme

OS X: there isn't an icon available for extraction, currently we have an application.png icon that is roughly based on the .app appearance, we'll likely be updating this file in the next few days

XP: We should go with our application.png that we have, Microsoft forgot to update the .exe icon from Windows 95 to XP, so we updated it for them in the Luna style.

Vista: We should go with our application.png.  The extracted .exe icon isn't that bad, but I think ours actually looks a little nicer.

Comment 44

[Vlado Valastiak [:wladow]]

Attachment: m-c patch, v3.2

Oook, just gnomestripe...

Comment 45

[Dão Gottwald [:dao]]

(In reply to comment #43)
> XP: We should go with our application.png that we have, Microsoft forgot to
> update the .exe icon from Windows 95 to XP, so we updated it for them in the
> Luna style.
> 
> Vista: We should go with our application.png.  The extracted .exe icon isn't
> that bad, but I think ours actually looks a little nicer.

We also use the moz-icon for .exe (e.g. in the applications prefpane, along with appliocation.png), though, so that's inconsistent.

Comment 46

[Dão Gottwald [:dao]]

Comment on attachment 367869 [details] [diff] [review]
m-c patch, v3.2

http://hg.mozilla.org/mozilla-central/rev/55b118c2235b

Comment 47

[Vlado Valastiak [:wladow]]

Attachment: m-191 patch

combined patch for 1.9.1, has ui-r=faaborg, r=gavin

Comment 48

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

Comment on attachment 367979 [details] [diff] [review]
m-191 patch

a191=beltzner

Comment 49

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/707568d6382a

Comment 50

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

This bug's priority relative to the set of other polish bugs is:
P3 - Polish issue that is in a secondary interface, occasionally encountered, or is not easily identifiable.