Last Comment Bug 301105 - [Mac] Classic: Use native-styled menus/menupopups
: [Mac] Classic: Use native-styled menus/menupopups
Status: RESOLVED FIXED
: fixed-seamonkey1.1a, relnote
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: 1.8 Branch
: PowerPC Mac OS X
: -- normal (vote)
: seamonkey1.1alpha
Assigned To: Stefan [:stefanh]
: themes
Mentors:
: 306284 (view as bug list)
Depends on:
Blocks: 46175 52103 179760 sm1.1
  Show dependency treegraph
 
Reported: 2005-07-17 08:32 PDT by Stefan [:stefanh]
Modified: 2006-06-25 14:57 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Testcase for xul menus (3.29 KB, application/vnd.mozilla.xul+xml)
2005-09-04 10:15 PDT, Stefan [:stefanh]
no flags Details
New menu-arrow.gif, should go in classic/global/mac/menu (847 bytes, image/gif)
2005-09-16 07:47 PDT, Stefan [:stefanh]
no flags Details
New menu-arrow-hov.gif, should go in classic/global/mac/menu (854 bytes, image/gif)
2005-09-16 07:48 PDT, Stefan [:stefanh]
no flags Details
autorepeat-arrow-dn.gif, should probably be in a new mac/global/scrollbox dir (847 bytes, image/gif)
2005-09-16 08:01 PDT, Stefan [:stefanh]
no flags Details
autorepeat-arrow-dn.gif, should probably be in a new classic/global/mac/scrollbox dir (847 bytes, image/gif)
2005-09-16 08:06 PDT, Stefan [:stefanh]
no flags Details
autorepeat-arrow-up.gif, should probably be in a new classic/global/mac/sc (850 bytes, image/gif)
2005-09-16 08:08 PDT, Stefan [:stefanh]
no flags Details
Make mac menus/menupopups native-styled (12.10 KB, patch)
2005-09-17 07:35 PDT, Stefan [:stefanh]
asaf: review-
Details | Diff | Review
native-styled menus/menupopups - better version (12.59 KB, patch)
2005-11-11 11:15 PST, Stefan [:stefanh]
no flags Details | Diff | Review
Even better version (10.20 KB, patch)
2005-11-15 10:24 PST, Stefan [:stefanh]
no flags Details | Diff | Review
correct version (12.66 KB, patch)
2005-12-04 05:42 PST, Stefan [:stefanh]
asaf: review-
mnyromyr: review+
Details | Diff | Review
empty scrollbox (19.48 KB, image/png)
2006-04-13 03:43 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details
scrollbox before patch (36.35 KB, image/gif)
2006-04-13 04:58 PDT, Stefan [:stefanh]
no flags Details
Testcase - empty menupopup (527 bytes, application/vnd.mozilla.xul+xml)
2006-04-13 08:41 PDT, Stefan [:stefanh]
no flags Details
menu-check.png, should go in global/mac/menu (346 bytes, image/png)
2006-04-13 09:27 PDT, Stefan [:stefanh]
no flags Details
menu-check-hov.png, should go in global/mac/menu (299 bytes, image/png)
2006-04-13 09:28 PDT, Stefan [:stefanh]
no flags Details
menu-check-dis.png, should go in global/mac/menu (369 bytes, image/png)
2006-04-13 09:29 PDT, Stefan [:stefanh]
no flags Details
New version addressing Mano's comments (15.30 KB, patch)
2006-04-13 09:49 PDT, Stefan [:stefanh]
asaf: review+
stefanh: review+
neil: superreview-
Details | Diff | Review
Empty scrollbox - latest patch (8.74 KB, image/gif)
2006-04-17 09:39 PDT, Stefan [:stefanh]
no flags Details
New version addressing Neil's comments (15.40 KB, patch)
2006-05-12 13:39 PDT, Stefan [:stefanh]
stefanh: review+
neil: superreview+
csthomas: approval‑seamonkey1.1a+
Details | Diff | Review
New menu-arrow.dis [checked in on trunk] (847 bytes, image/gif)
2006-05-24 12:54 PDT, Stefan [:stefanh]
neil: review+
neil: superreview+
Details
Screenshots comparing old arrow with new (23.79 KB, image/gif)
2006-05-24 13:01 PDT, Stefan [:stefanh]
no flags Details
1.8 diff of autocomplete.css (1.30 KB, patch)
2006-05-26 17:12 PDT, Stefan [:stefanh]
no flags Details | Diff | Review
Use correct font for menuitems (896 bytes, patch)
2006-06-21 09:36 PDT, Stefan [:stefanh]
no flags Details | Diff | Review
Keep bold style (Checked in trunk & 1.8 branch) (692 bytes, patch)
2006-06-22 05:46 PDT, Stefan [:stefanh]
asaf: review+
neil: superreview+
csthomas: approval‑seamonkey1.1a+
Details | Diff | Review

Description Stefan [:stefanh] 2005-07-17 08:32:12 PDT
Since Mano now have implemented NS_THEME_MENU_POPUP and NS_THEME_MENUITEM in bug
118025 we can now use -moz-appearance: menuitem, menupopup in order to make our
xul menus/menupopups more in line with the native ones.
Comment 1 Stefan [:stefanh] 2005-08-29 08:17:30 PDT
*** Bug 306284 has been marked as a duplicate of this bug. ***
Comment 2 Stefan [:stefanh] 2005-09-04 09:55:45 PDT
At http://hem.fyristorg.com/s_her/050903/classic.jar you'll find a classic.jar
package at that can be used for testing, comments are welcome. Just re-name it
to classic.jar, navigate to the chrome folder and replace the existing
classic.jar. Should work fine on a nightly trunk build (files are taken from the
050903 build). Note that the files are going to be cleaned up before i post any
patches here.

Apart from switching to native-styled menus (colors etc) icons and text now line
up in menus. I also replaced "radio"-styled checkmarks (dots) with normal
checkmarks and the scrollbox now uses the same arrow as in the native menus
(ripped the arrow from pinstripe).

to-do:

1) Fix menuitems in bookmark menus, so the hover/select color extend all the way
to the right. Currently, there's a white gap at the right.

2) Get new arrows for the sub-menus (right-arrows) - I'll just flip the 
scrollbox arrow 90°.

3) Clean up the files
Comment 3 Stefan [:stefanh] 2005-09-04 10:04:12 PDT
> Just re-name it to classic.jar

Err, no need to rename it, of course...
Comment 4 Stefan [:stefanh] 2005-09-04 10:15:37 PDT
Created attachment 194850 [details]
Testcase for xul menus

Just attaching a testcase to demonstrate how xul menus will look like. Atm, the
only issue is that the switch to blue background-color is a bit slow when you
select a menu.
Comment 5 Stefan [:stefanh] 2005-09-04 10:22:02 PDT
 
> Just attaching a testcase to demonstrate how xul menus will look like. Atm, the
> only issue is that the switch to blue background-color is a bit slow when you
> select a menu.

Note that Firefox' Pinstripe theme doesn't seem to handle this correct. Hmm, it
seems that the scrollbox-arrow (down-arrow in menus) that I use is Pinstripe's
right-arrow (flipped 90°).

Comment 6 Samuel Sidler (old account; do not CC) 2005-09-05 01:03:49 PDT
Alright, I gave this a shot today and just have a few comments/questions. Before
the negative...... It's a great improvement. It integrates with OS X much better
than SM used to. *Thank you* Now... onto the questions.

For the record, my UA is: 
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050828
SeaMonkey/1.1a

1) First thing I noticed was that the lack of highlight on the location bar.
When typing in a URL and pressing the down arrow, the suggestions (assuming
you've been to enough websites) should be highlight indicating they're being
selected. Right now we just bold the description text (but what if there isn't
any?). Keep in mind, this is only for auto-complete. Clicking the actual down
arrow on the right allows you to select options with the light blue, standard
highlight color. Which brings me to the next question...

2) The selection color in the location bar when clicking the down arrow is light
blue. I think this should be replaced with the dark blue "standard" highlight
color used in the menus.

3) On submenus... When opening a submenu from SM, we're a few pixels low from
where we should be (I think). If you'll notice the application menu's submenus,
the highlight from the submenu and the top item match up perfectly (pixel
perfect even). However, our menus are about 3 or 4 pixels too low from matching
up. Can this be fixed? (Note: there is one exception to this when the submenu is
too close to the top of the screen, or in our case menubar, the highlights don't
quite match up. They're about 6 or 7 pixels low.)

Anyway, besides what you mentioned, that's all I've noticed so far (Is there a
bug for improving the look of tabs on Mac?). Great work. :)
Comment 7 Stefan [:stefanh] 2005-09-05 08:46:14 PDT
(In reply to comment #6)

Simon, thanks for the feedback :)

> 1) First thing I noticed was that the lack of highlight on the location bar.
> When typing in a URL and pressing the down arrow, the suggestions (assuming
> you've been to enough websites) should be highlight indicating they're being
> selected. Right now we just bold the description text (but what if there isn't
> any?).

Ah, yeah. Forgot that, will fix it.
 
> 2) The selection color in the location bar when clicking the down arrow is light
> blue. I think this should be replaced with the dark blue "standard" highlight
> color used in the menus.

That might be difficult, unfortunately. The only way to get the blue color
(correctly) is to give the item a complete menu-look (same background-color as
the menus will be applied).
 
> 3) On submenus... When opening a submenu from SM, we're a few pixels low from
> where we should be (I think). If you'll notice the application menu's submenus,
> the highlight from the submenu and the top item match up perfectly (pixel
> perfect even). However, our menus are about 3 or 4 pixels too low from matching
> up. Can this be fixed?

Hmm, I have to have a further look at this...
Comment 8 Stefan [:stefanh] 2005-09-16 07:47:01 PDT
Created attachment 196325 [details]
New menu-arrow.gif, should go in classic/global/mac/menu
Comment 9 Stefan [:stefanh] 2005-09-16 07:48:13 PDT
Created attachment 196326 [details]
New menu-arrow-hov.gif, should go in classic/global/mac/menu
Comment 10 Stefan [:stefanh] 2005-09-16 08:01:42 PDT
Created attachment 196329 [details]
autorepeat-arrow-dn.gif, should probably be in a new mac/global/scrollbox dir
Comment 11 Stefan [:stefanh] 2005-09-16 08:04:34 PDT
Comment on attachment 196329 [details]
autorepeat-arrow-dn.gif, should probably be in a new mac/global/scrollbox dir

Hang on, lots of wrong stuff...
Comment 12 Stefan [:stefanh] 2005-09-16 08:06:52 PDT
Created attachment 196331 [details]
autorepeat-arrow-dn.gif, should probably be in a new classic/global/mac/scrollbox dir
Comment 13 Stefan [:stefanh] 2005-09-16 08:08:13 PDT
Created attachment 196332 [details]
autorepeat-arrow-up.gif, should probably be in a new classic/global/mac/sc
Comment 14 Stefan [:stefanh] 2005-09-17 07:35:26 PDT
Created attachment 196420 [details] [diff] [review]
Make mac menus/menupopups native-styled

This patch assumes:

1) that autorepeat-arrows (attachment #196331 [details] and 196332) are in a new
classic/global/mac/scrollbox directory.

2) that the new menu-arrow.gif and menu-arrow-hover.gif ( attachment #196325 [details]
and #196326) are in classic/global/mac/menu.

The patch doesn't indent icons in certain menulists, (like the one in Sidebar
from where you choose search engine).
Comment 15 neil@parkwaycc.co.uk 2005-09-18 09:29:50 PDT
(In reply to comment #12)
>autorepeat-arrow-dn.gif should probably be in a new
classic/global/mac/scrollbox dir
Or possibly classic/global/mac/arrow ?
Comment 16 Stefan [:stefanh] 2005-09-18 15:02:58 PDT
Comment on attachment 196420 [details] [diff] [review]
Make mac menus/menupopups native-styled

Mano,
can you take a look at this, please? The directory/directories for the images
might be located  in a different place, but I feel I'm done with the other
stuff (css etc).
Comment 17 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-10-30 01:23:00 PDT
Comment on attachment 196420 [details] [diff] [review]
Make mac menus/menupopups native-styled

Menuitems aren't padded enough with this patch (at least one more pixel at the left side, not sure about vertical paddings).

Index: themes/classic/global/mac/autocomplete.css
===================================================================
 
 /* ::::: autocomplete popups ::::: */
 
 .autocomplete-result-popup,
 .autocomplete-history-popup {
-  border-width: 1px;
-  -moz-border-top-colors: ThreeDDarkShadow;
-  -moz-border-right-colors: ThreeDDarkShadow;
-  -moz-border-bottom-colors: ThreeDDarkShadow;
-  -moz-border-left-colors: ThreeDDarkShadow;
+  -moz-appearance: none !important;

Why is this change?
 
Index: themes/classic/global/mac/menu.css
===================================================================

 
+.autocomplete-history-popup > menuitem[_moz-menuactive="true"] {
+  color: inherit !important;
+  background-color: Highlight !important;
+

} ?

Index: themes/classic/global/mac/popup.css
===================================================================

 menupopup,
 popup {
-  -moz-appearance: menu;
-  border-top: 2px solid;
-  border-right: 3px solid;
-  border-bottom: 3px solid;
-  border-left: 2px solid;
-  -moz-border-top-colors: #000000 #FFFFFF;
-  -moz-border-right-colors: #000000 #000000 #777777;
-  -moz-border-bottom-colors: #000000 #000000 #777777;
-  -moz-border-left-colors: #000000 #FFFFFF;
-  background-color: Menu;
+  -moz-appearance: menupopup;
+  padding-bottom: 4px;
+  padding-top: 4px;

Why do the first and last menuitems need more padding? This also seem to break menulists (their scrollbar is misplaced).
Comment 18 Stefan [:stefanh] 2005-10-30 14:17:04 PST
Comment on attachment 196420 [details] [diff] [review]
Make mac menus/menupopups native-styled

(In reply to comment #17)
> (From update of attachment 196420 [details] [diff] [review] [edit])
> Menuitems aren't padded enough with this patch (at least one more pixel at the
> left side, not sure about vertical paddings).
>

I didn't changed the left padding, I had some trouble when I compared with the OS menuitems since the text is smaller in our menuitems. But I'll add 1px to left/right padding.

> Index: themes/classic/global/mac/autocomplete.css
> ===================================================================
> 
>  /* ::::: autocomplete popups ::::: */
> 
>  .autocomplete-result-popup,
>  .autocomplete-history-popup {
> -  border-width: 1px;
> -  -moz-border-top-colors: ThreeDDarkShadow;
> -  -moz-border-right-colors: ThreeDDarkShadow;
> -  -moz-border-bottom-colors: ThreeDDarkShadow;
> -  -moz-border-left-colors: ThreeDDarkShadow;
> +  -moz-appearance: none !important;
> 
> Why is this change?

Since the history drop-down is a menu/menupopup it will look like a menu with the changes in menu.css ("-moz-appearance: menuitem;"). The grey background will shine through.
> 
> Index: themes/classic/global/mac/menu.css
> ===================================================================
> 
> 
> +.autocomplete-history-popup > menuitem[_moz-menuactive="true"] {
> +  color: inherit !important;
> +  background-color: Highlight !important;
> +
> 
> } ?

I want to keep the text black upon select/hover. Without the color style rule the text would turn white because of the "-moz-mac-menutextselect" rule at line 177 in menu.css. I also want the background to behave the same as the autocomplete-popups. With this change the history-dropdown in the url bar will look/behave the same as the autocomplete-popup. We could hardcode the blue background on hovered/selected menuitems if you prefer the history-drop-down to behave/look different from the autocomplete popup (we could also make the background of the items in both widgets turn blue upon hover/select

In Firefox, both widgets look the same in the url bar, though. And i thought that from a user pof it would be more consistent to make them both appear the same.

One way of making the history-dropdown to look like a menu without the grey background is to hardcode the background-color to blue upon select/hover. If we do that, I think we should do it for the autocomplete-popup as well. I can't use "-moz-mac-menuselect" (actually, it also returns a hard-coded color), since it works only on trunk. On branch it returns the old-style color.

> 
> Index: themes/classic/global/mac/popup.css
> ===================================================================
> 
>  menupopup,
>  popup {
> -  -moz-appearance: menu;
> -  border-top: 2px solid;
> -  border-right: 3px solid;
> -  border-bottom: 3px solid;
> -  border-left: 2px solid;
> -  -moz-border-top-colors: #000000 #FFFFFF;
> -  -moz-border-right-colors: #000000 #000000 #777777;
> -  -moz-border-bottom-colors: #000000 #000000 #777777;
> -  -moz-border-left-colors: #000000 #FFFFFF;
> -  background-color: Menu;
> +  -moz-appearance: menupopup;
> +  padding-bottom: 4px;
> +  padding-top: 4px;
> 
> Why do the first and last menuitems need more padding? This also seem to break
> menulists (their scrollbar is misplaced).

The padding that is set for the menuitems in menu.css doesn't affect the bottom/top of the menu when you hover over the first/last menuitem in a menu. In native menus there's a white space above/under the first/last menutiem when you hover it. I missed the issue with the menulist, though. Looking at toolkit's popup.css, I now see that the padding should be added to the ".popup-internal-box" instead. Will fix that.
Comment 19 Stefan [:stefanh] 2005-11-11 11:15:35 PST
Created attachment 202699 [details] [diff] [review]
native-styled menus/menupopups - better version

I've used 2 more pixels left/right padding on menuitems,they now look exactly as Apples. The height of the menuitems are now set to a minimum of 19px, which is the same height as the native ones has without icons (in menus and context menus). I also fixed the top/bottom distance to the seaparator.

I've used less margin etc in menulists since the font is rather small.

 .autocomplete-history-popup > menuitem {
+  -moz-appearance: none !important;
   max-width: none !important;
   font: message-box;
+  height: 1.3em;
+  min-height: 18px;
+  padding: 0px 2px 0px 2px;
+  margin: 0;
+}

The last 4 lines of styles are copied from tree.css. This is to make items in the urlbar history-popup look the same as items in the autocomplete one.

I also fixed the issue with the padding in menulists.

As the last patch, this one also uses the images in previous attachments.
Comment 20 Stefan [:stefanh] 2005-11-15 10:03:15 PST
Comment on attachment 202699 [details] [diff] [review]
native-styled menus/menupopups - better version

> The height of the menuitems are now set to a minimum of 19px, which
> is the same height as the native ones has without icons (in menus and context
> menus).

After using the patch for some time I start to feel that this might not be the best idea. Since we're using a smaller font (and I can't see any way of changing the font-size without hard-coding it) than Apple, menuitems appear to have too much vertical space.
Comment 21 Stefan [:stefanh] 2005-11-15 10:24:59 PST
Created attachment 203143 [details] [diff] [review]
Even better version

This patch is identical to the previous one, except for:

1) No min-heights
2) A single whitespace correction
Comment 22 Stefan [:stefanh] 2005-12-04 05:42:02 PST
Created attachment 204949 [details] [diff] [review]
correct version

New, corrected version: included diff of jar.mn file, corrected an issue with the autocomplete popup's top/bottom padding (shouldn't be any). Otherwise the same as previous patch.

The question here is (in my point of view) the height of the menuitems. Should the height be set to a minimum of 19px or shouldn't there be any height set at all? One could argue that it's more correct not specifying any height, it does looks like apples own menuitems vary in height (according to font-size etc). Currently, with this patch, when no min-height is specified, menuitems in a "normal" context-menu will have a height of 18px.
Comment 23 Stefan [:stefanh] 2005-12-21 11:15:21 PST
Comment on attachment 204949 [details] [diff] [review]
correct version

Giving this to Mnyromyr instead...
Comment 24 Stefan [:stefanh] 2006-02-23 13:59:27 PST
Comment on attachment 204949 [details] [diff] [review]
correct version

Mano, if you don't have the time for this, please bump the request to mnyromyr. It didn't made it for 1.0, but I would really like to have this for 1.1. Once we're past 1.1, this patch is pretty useless (assuming cocoa toolkit will be used on trunk).
Comment 25 Stefan [:stefanh] 2006-03-05 16:17:33 PST
Comment on attachment 204949 [details] [diff] [review]
correct version

I haven't heard a sound from Mano for some time. Mnyromyr, can you take a look at this?
Comment 26 Karsten Düsterloh 2006-03-26 13:00:32 PST
Comment on attachment 204949 [details] [diff] [review]
correct version

>Index: themes/classic/global/mac/autocomplete.css
>===================================================================
>+  -moz-appearance: none !important;
>+  border-top: 1px solid ThreeDDarkShadow !important;
>+  padding: 0 !important;

Are all these !importants here (and in the other files) really necessary? 
The first one here isn't, I didn't check the others...

Anyway, this patch does work, I do indeed now see native menuitems in our menus, which is very nice.
But you really should get a review from a "Mac-savvy" person, which I'm not (yet), though...
Comment 27 Stefan [:stefanh] 2006-04-06 08:52:29 PDT
Comment on attachment 204949 [details] [diff] [review]
correct version

Asaf, can you please add a comment about when you plan to look at this?
Comment 28 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-04-09 12:20:59 PDT
Comment on attachment 204949 [details] [diff] [review]
correct version

Going to review this on Wednesday now that my mac is finally back... Sorry for the latency, Stefan.
Comment 29 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-04-13 03:43:01 PDT
Created attachment 218270 [details]
empty scrollbox

First thing I noticed after applying this patch.

In other news, nice work for remote xul, feel free to port it back to Pinstripe :)
Comment 30 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-04-13 03:51:10 PDT
Also, we need to replace the submenu arrow images (and please open a new bug on nsINativeThemeing that, and the scrollbox images; there are values for them in the HITheme/Old Quickdraw API IIRC).
Comment 31 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-04-13 04:04:08 PDT
Comment on attachment 204949 [details] [diff] [review]
correct version

Also:
 * Modifiers (at least command and shift) are not visible in popup menus after applying this patch. You can test it in  the webdeveloper-toolbar extension.
 * We need new images for checked menuitems, esp. for the :hover case.
Comment 32 Stefan [:stefanh] 2006-04-13 04:37:26 PDT
(In reply to comment #30)
> Also, we need to replace the submenu arrow images (and please open a new bug on

Hmm, the sub-menus have new images - see attachment #196325 [details] and attachment #196326 [details]. Did you replace the old images?

> nsINativeThemeing that, and the scrollbox images; there are values for them in
> the HITheme/Old Quickdraw API IIRC).
> 

(In reply to comment #31)
> (From update of attachment 204949 [details] [diff] [review] [edit])
> Also:
>  * Modifiers (at least command and shift) are not visible in popup menus after
> applying this patch. You can test it in  the webdeveloper-toolbar extension.

That's bug 331803 ;-)

>  * We need new images for checked menuitems, esp. for the :hover case.

Hmm, I suppose I could take Pinstripe's.

Comment 33 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-04-13 04:44:09 PDT
(In reply to comment #32)
> (In reply to comment #30)
> > Also, we need to replace the submenu arrow images (and please open a new bug on
> 
> Hmm, the sub-menus have new images - see attachment #196325 [details] [edit] and attachment
> #196326 [edit]. Did you replace the old images?

oops, sorry.

> > nsINativeThemeing that, and the scrollbox images; there are values for them in
> > the HITheme/Old Quickdraw API IIRC).
> > 
> 
> (In reply to comment #31)
> > (From update of attachment 204949 [details] [diff] [review] [edit] [edit])
> > Also:
> >  * Modifiers (at least command and shift) are not visible in popup menus after
> > applying this patch. You can test it in  the webdeveloper-toolbar extension.
> 
> That's bug 331803 ;-)

Hrm, I didn't notice it Seamonkey before applying this patch.
> 
> >  * We need new images for checked menuitems, esp. for the :hover case.
> 
> Hmm, I suppose I could take Pinstripe's.
> 

ok.
Comment 34 Stefan [:stefanh] 2006-04-13 04:58:49 PDT
Created attachment 218274 [details]
scrollbox before patch
Comment 35 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-04-13 05:03:20 PDT
OK, ignore the modifiers comment; i tested an earlier build.
Comment 36 Stefan [:stefanh] 2006-04-13 08:41:14 PDT
Created attachment 218294 [details]
Testcase - empty menupopup

Just attaching a testcase here. The reason that the scrollbox-arrows appear in the empty "Imported IE Favorites" seems to be because menupopups has a "min-width: 1px" rule in popup.css. If you remove that rule nothing will appear.

Pinstripe doesn't have any min-height rule. But in Firefox you'll notice a tiny "menupopup" in the testcase - guess it comes from the margin added to the autorepeatbutton in Pinstripe's scrollbox.css.
Comment 37 Stefan [:stefanh] 2006-04-13 09:27:28 PDT
Created attachment 218300 [details]
menu-check.png, should go in global/mac/menu
Comment 38 Stefan [:stefanh] 2006-04-13 09:28:32 PDT
Created attachment 218301 [details]
menu-check-hov.png, should go in global/mac/menu
Comment 39 Stefan [:stefanh] 2006-04-13 09:29:25 PDT
Created attachment 218302 [details]
menu-check-dis.png, should go in global/mac/menu
Comment 40 Stefan [:stefanh] 2006-04-13 09:49:55 PDT
Created attachment 218306 [details] [diff] [review]
New version addressing Mano's comments

New version (please use images in previous 3 attachments):

1) Uses Pinstripe's checkmarks in menuitems
2) Fixes the empty scrollbox by removing the "min-width: 1px;" rule in popup.css and adding margin to the autorepeatbutton in scrollbox.css
Comment 41 Stefan [:stefanh] 2006-04-13 11:03:09 PDT
Comment on attachment 218306 [details] [diff] [review]
New version addressing Mano's comments

Just transfering Mnyromyr's r+...
Comment 42 Stefan [:stefanh] 2006-04-13 13:49:50 PDT
> (and please open a new bug on
> nsINativeThemeing that, and the scrollbox images; there are values for them in
> the HITheme/Old Quickdraw API IIRC).
>

Filed bug 333910.

Comment 43 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-04-14 01:07:41 PDT
Comment on attachment 218306 [details] [diff] [review]
New version addressing Mano's comments

r=mano
Comment 44 Karsten Düsterloh 2006-04-14 16:21:47 PDT
When I apply this last changes then the scrollbox for empty bookmark folders vanishes - almost. I still see a thin 1px(?) wide vertical white line, about 2/3 the height of the folder entry...
Comment 45 Stefan [:stefanh] 2006-04-17 09:22:57 PDT
(In reply to comment #44)
> When I apply this last changes then the scrollbox for empty bookmark folders
> vanishes - almost. I still see a thin 1px(?) wide vertical white line, about
> 2/3 the height of the folder entry...
> 
That's by purpose. It's the margin I added in scrollbox.css. Better show something than nothing at all. Compare with how it looks now (screenshot in attachment #218274 [details]).
Comment 46 neil@parkwaycc.co.uk 2006-04-17 09:30:38 PDT
(In reply to comment #45)
>Compare with how it looks now
*I* can't...
Comment 47 Stefan [:stefanh] 2006-04-17 09:39:53 PDT
Created attachment 218698 [details]
Empty scrollbox - latest patch
Comment 48 Stefan [:stefanh] 2006-04-25 02:05:42 PDT
Comment on attachment 218306 [details] [diff] [review]
New version addressing Mano's comments

Neil, any chance you could look at this soon? It would be nice to finally have this on branch for 1.1a.
Comment 49 neil@parkwaycc.co.uk 2006-05-03 06:54:59 PDT
Comment on attachment 218306 [details] [diff] [review]
New version addressing Mano's comments

>-  skin/classic/global/menu/menu-check.gif                       (global/mac/menu/menu-check.gif)
>-  skin/classic/global/menu/menu-check-dis.gif                   (global/mac/menu/menu-check-dis.gif)
>-  skin/classic/global/menu/menu-check-hov.gif                   (global/mac/menu/menu-check-hov.gif)
>+  skin/classic/global/menu/menu-check.png                       (global/mac/menu/menu-check.png)
>+  skin/classic/global/menu/menu-check-dis.png                   (global/mac/menu/menu-check-dis.png)
>+  skin/classic/global/menu/menu-check-hov.png                   (global/mac/menu/menu-check-hov.png)
Why the change from .gif to .png? Do radio menuitems use tick marks too?

>+.menu-iconic,
>+.menuitem-iconic {
>+  padding: 2px 2px 2px 18px !important;
>+  margin: 0 !important;
>+}
>+
>+.menu-iconic[checked="true"],
>+.menuitem-iconic[checked="true"],
>+.menuitem-iconic[value] {
>+  padding-left: 0px !important;
>+}
Why do these change only when the menuitem is checked or has a value?
[menus are never checked]


> menulist > menupopup > menuitem,
> .menulist-menupopup > menuitem {
>   max-width: none;
>-  padding: 0px 20px 0px 0px;
>+  padding-right: 20px;
Why this change?
Comment 50 Stefan [:stefanh] 2006-05-03 11:04:21 PDT
(In reply to comment #49)
> (From update of attachment 218306 [details] [diff] [review] [edit])
> >-  skin/classic/global/menu/menu-check.gif                       (global/mac/menu/menu-check.gif)
> >-  skin/classic/global/menu/menu-check-dis.gif                   (global/mac/menu/menu-check-dis.gif)
> >-  skin/classic/global/menu/menu-check-hov.gif                   (global/mac/menu/menu-check-hov.gif)
> >+  skin/classic/global/menu/menu-check.png                       (global/mac/menu/menu-check.png)
> >+  skin/classic/global/menu/menu-check-dis.png                   (global/mac/menu/menu-check-dis.png)
> >+  skin/classic/global/menu/menu-check-hov.png                   (global/mac/menu/menu-check-hov.png)
> Why the change from .gif to .png?

I took Pinstripe's tick marks (see comment #40). Do you want me to convert them to gif's?

> Do radio menuitems use tick marks too?

I've never seen those "radio"-styled marks on mac. It looks pretty weird having those in some of the menuitems. So, yes - they should use tick marks as well. Hmm... oh, I could just use the style rules for the "checkbox menuitem" (rename the commented title to "Checked menuitems" instead of "Checkbox menuitems").

 > >+.menu-iconic,
> >+.menuitem-iconic {
> >+  padding: 2px 2px 2px 18px !important;
> >+  margin: 0 !important;
> >+}
> >+
> >+.menu-iconic[checked="true"],
> >+.menuitem-iconic[checked="true"],
> >+.menuitem-iconic[value] {
> >+  padding-left: 0px !important;
> >+}
> Why do these change only when the menuitem is checked or has a value?

I actually spotted two errors here. What I want here is the text to line up vertically, text in a checked menuitem should align vertically with text in a unchecked menuitem. Tick marks should appear "outside". But I see now that I missed 2px here:

+.menuitem-iconic[value] {
+  padding-left: 0px !important;

padding-left should be 2px (since the icon is 16x16). I should also add "menuitem-iconic[disabled="true"]"

> [menus are never checked]

I suppose I wanted to be on the safe side :-/

> 
> > menulist > menupopup > menuitem,
> > .menulist-menupopup > menuitem {
> >   max-width: none;
> >-  padding: 0px 20px 0px 0px;
> >+  padding-right: 20px;
> Why this change?
>

I want the same top, left and bottom padding as menuitems in menupopups. 
Comment 51 neil@parkwaycc.co.uk 2006-05-05 06:42:18 PDT
(In reply to comment #50)
>>Why the change from .gif to .png?
>I took Pinstripe's tick marks (see comment #40). Do you want me to convert them
>to gif's?
Either that or I suppose you could just use absolute paths.

>Hmm... oh, I could just use the style rules for the "checkbox menuitem" (rename
>the commented title to "Checked menuitems" instead of "Checkbox menuitems").
Yes, that would work too.

>>>+.menu-iconic[checked="true"],
>>>+.menuitem-iconic[checked="true"],
>>>+.menuitem-iconic[value] {
>>>+  padding-left: 0px !important;
>>>+}
>>Why do these change only when the menuitem is checked or has a value?
>I actually spotted two errors here. What I want here is the text to line up
>vertically, text in a checked menuitem should align vertically with text in a
>unchecked menuitem. Tick marks should appear "outside". But I see now that I
>missed 2px here:
> 
>+.menuitem-iconic[value] {
>+  padding-left: 0px !important;
> 
>padding-left should be 2px (since the icon is 16x16). I should also add
>"menuitem-iconic[disabled="true"]"
>>[menus are never checked]
> I suppose I wanted to be on the safe side :-/
I'm not following you. Why does the padding depend on whether the tick mark image is assigned to the image or not? In other themes, the image size is set to 16px whether or not the tick is present. And I have no idea why you might want to change padding depending on whether the item is disabled or has a value.
Comment 52 Stefan [:stefanh] 2006-05-08 09:34:36 PDT
> I'm not following you. Why does the padding depend on whether the tick mark
> image is assigned to the image or not? In other themes, the image size is set
> to 16px whether or not the tick is present. And I have no idea why you might
> want to change padding depending on whether the item is disabled or has a
> value.

Uh, forget what I said... I should of course use ".menuitem-iconic[type="checkbox"]" etc. Thinking of it, I would actually prefer to use variants of .menu-iconic-left to indent the icons/tick marks. Not sure if it will work, though (will check).

Comment 53 neil@parkwaycc.co.uk 2006-05-08 09:52:03 PDT
Comment on attachment 218306 [details] [diff] [review]
New version addressing Mano's comments

Minusing for now as I'm expecting an updated patch which addresses the outstanding comments.
Comment 54 Stefan [:stefanh] 2006-05-12 13:39:03 PDT
Created attachment 221843 [details] [diff] [review]
New version addressing Neil's comments

Changes since last patch:

-- menu.css --

* ".menu-iconic-left" is used to indent icons. Icons in menulists menuitems, and tick marks in menuitems are not indented with ".menu-iconic-left", though. In the previous patch icons (like in the bm menu) didn't line up 100% with label text in menuitems without icons - now they do.

* Removed the style rules for checked radio menuitems.

FYI, this:
+.menu-iconic,
+.menuitem-iconic {
+  margin-top: 0 !important;
+}
is because of the negative top margin set in communicator/bookmarks/bookmarksToolbar.css#130. I just simplyfied it (used margin: 0 !important; before).

-- jar.mn --

* Now using Pinstripe's tick marks straight from /toolkit/themes/pinstripe/global/menu.

Transferring r+
Comment 55 Robert Kaiser (not working on stability any more) 2006-05-22 09:06:19 PDT
Checked in by request of stefanh:

Checking in themes/classic/jar.mn;
/cvsroot/mozilla/themes/classic/jar.mn,v  <--  jar.mn
new revision: 1.142; previous revision: 1.141
done
Checking in themes/classic/global/mac/autocomplete.css;
/cvsroot/mozilla/themes/classic/global/mac/autocomplete.css,v  <--  autocomplete.css
new revision: 1.14; previous revision: 1.13
done
Checking in themes/classic/global/mac/menu.css;
/cvsroot/mozilla/themes/classic/global/mac/menu.css,v  <--  menu.css
new revision: 1.41; previous revision: 1.40
done
Checking in themes/classic/global/mac/popup.css;
/cvsroot/mozilla/themes/classic/global/mac/popup.css,v  <--  popup.css
new revision: 1.11; previous revision: 1.10
done
Checking in themes/classic/global/mac/scrollbox.css;
/cvsroot/mozilla/themes/classic/global/mac/scrollbox.css,v  <--  scrollbox.css
new revision: 1.6; previous revision: 1.5
done
Checking in themes/classic/global/mac/menu/menu-arrow.gif;
/cvsroot/mozilla/themes/classic/global/mac/menu/menu-arrow.gif,v  <--  menu-arrow.gif
new revision: 1.2; previous revision: 1.1
done
Checking in themes/classic/global/mac/menu/menu-arrow-hov.gif;
/cvsroot/mozilla/themes/classic/global/mac/menu/menu-arrow-hov.gif,v  <--  menu-arrow-hov.gif
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/themes/classic/global/mac/scrollbox/autorepeat-arrow-dn.gif,v
done
Checking in themes/classic/global/mac/scrollbox/autorepeat-arrow-dn.gif;
/cvsroot/mozilla/themes/classic/global/mac/scrollbox/autorepeat-arrow-dn.gif,v  <--  autorepeat-arrow-dn.gif
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/themes/classic/global/mac/scrollbox/autorepeat-arrow-up.gif,v
done
Checking in themes/classic/global/mac/scrollbox/autorepeat-arrow-up.gif;
/cvsroot/mozilla/themes/classic/global/mac/scrollbox/autorepeat-arrow-up.gif,v  <--  autorepeat-arrow-up.gif
initial revision: 1.1
done
Comment 56 Stefan [:stefanh] 2006-05-22 11:29:55 PDT
I'll let this bake on trunk for a while before asking branch approval.
Comment 57 Stefan [:stefanh] 2006-05-24 12:52:42 PDT
Hm. We need a new disabled menu-arrow. Now when I think of it, KaiRo actually asked me about that when he was about to check in the patch. For some reason I thought it was okay to not having a new menu-arrow-dis.gif.
Comment 58 Stefan [:stefanh] 2006-05-24 12:54:10 PDT
Created attachment 223216 [details]
New menu-arrow.dis [checked in on trunk]

Neil, can you r/sr the image so it can be checked in?
Comment 59 Stefan [:stefanh] 2006-05-24 13:01:35 PDT
Created attachment 223217 [details]
Screenshots comparing old arrow with new
Comment 60 Stefan [:stefanh] 2006-05-26 13:26:21 PDT
Moving this to a product where the correct flags can be set...
Comment 61 Stefan [:stefanh] 2006-05-26 16:47:42 PDT
Comment on attachment 221843 [details] [diff] [review]
New version addressing Neil's comments

Since I'll be gone for two weeks I request approval now. There's no need to hurry, though - this is just in case things should tighten up for 1.1a before I get back (seems unlikely, but anyway...) . If that is the case and the patch gets approval, I kindly request the granter to check in the patch.
Comment 62 Stefan [:stefanh] 2006-05-26 16:55:05 PDT
To avoid any confusion if someone checks in the patch while I'm away - here's what needs to get in:

1) Attachment #196325 [details], attachment #196326 [details] and attachment #223216 [details] in themes/classic/global/mac/menu

2) A 'scrollbox' directory needs to be created in themes/classic/global/mac

3) Attachment #196331 [details] and attachment #196332 [details] can then be checked in to the new scrollbox directory

4) Attachment #221843 [details] [diff] (the patch)

Note: since bug 311737 doesn't seems to have been checked in on the 1.8 branch (at least the mac version of autocomplete.css hasn't been changed), the patch will fail in autocomplete.css - I'll attach diff for the branch version of autocomplete.css
Comment 63 Stefan [:stefanh] 2006-05-26 17:12:36 PDT
Created attachment 223513 [details] [diff] [review]
1.8 diff of autocomplete.css
Comment 64 Stefan [:stefanh] 2006-06-15 14:46:23 PDT
Comment on attachment 223513 [details] [diff] [review]
1.8 diff of autocomplete.css

The patch in bug 311737 have now landed on the 1.8 branch.
Comment 65 Stefan [:stefanh] 2006-06-21 09:36:54 PDT
Created attachment 226500 [details] [diff] [review]
Use correct font for menuitems

Since bug 335683 is fixed on trunk/1.8 we can now use the correct font in mac menuitems. Before, we used the standard system fonts for menus/menuitems - with this patch we'll use the real menuitemfont (1px larger on the locales I know). I also removed the bold font for default menuitems - I haven't seen this in mac apps (well, firefox seem to use it in the search pop-down for the default selected search engine, but that's it). I also belive it's a bit of an overkill to use it and with the larger font it also becomes a little bit too large.
Comment 66 Stefan [:stefanh] 2006-06-21 09:40:35 PDT
Comment on attachment 226500 [details] [diff] [review]
Use correct font for menuitems

Sight, forgot to set the review flags - see previous comment for an explanation
Comment 67 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-06-22 00:56:27 PDT
Comment on attachment 226500 [details] [diff] [review]
Use correct font for menuitems

Let's leave the bold style rule for now (At least for xul-default-themes consistency).
Comment 68 Stefan [:stefanh] 2006-06-22 05:46:31 PDT
Created attachment 226635 [details] [diff] [review]
Keep bold style (Checked in trunk & 1.8 branch)

> Let's leave the bold style rule for now (At least for xul-default-themes
> consistency).

Sure (note: Pinstripe doesn't have it - but Winstripe does).
Comment 69 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-06-22 06:00:55 PDT
Comment on attachment 226635 [details] [diff] [review]
Keep bold style (Checked in trunk & 1.8 branch)

Weird, as far as i remember, i did see the bold style rule in Pinstripe (in Firefox's searchbar) but LXR tells me i'm wrong. Nevertheless, we should have a separate bug for this.

r=mano.
Comment 70 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-06-23 12:58:48 PDT
Comment on attachment 221843 [details] [diff] [review]
New version addressing Neil's comments

a=me for 1.1a
Comment 71 Stefan [:stefanh] 2006-06-24 16:18:38 PDT
Comment on attachment 226635 [details] [diff] [review]
Keep bold style (Checked in trunk & 1.8 branch)

This will complete the native style of our menus/menuitems - correct system font for menuitems.
Comment 72 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-06-24 18:29:21 PDT
Comment on attachment 226635 [details] [diff] [review]
Keep bold style (Checked in trunk & 1.8 branch)

a=me for 1.1a
Comment 73 Ian Neal 2006-06-25 14:40:39 PDT
Comment on attachment 221843 [details] [diff] [review]
New version addressing Neil's comments

Checking in (1.8 branch)
jar.mn;
new revision: 1.136.2.6; previous revision: 1.136.2.5
global/mac/autocomplete.css;
new revision: 1.12.8.2; previous revision: 1.12.8.1
global/mac/menu.css;
new revision: 1.40.28.1; previous revision: 1.40
global/mac/popup.css;
new revision: 1.10.28.1; previous revision: 1.10
global/mac/scrollbox.css;
new revision: 1.5.150.1; previous revision: 1.5
global/mac/menu/menu-arrow.gif;
new revision: 1.1.150.1; previous revision: 1.1
global/mac/menu/menu-arrow-hov.gif;
new revision: 1.1.150.1; previous revision: 1.1
global/mac/menu/menu-arrow-dis.gif;
new revision: 1.1.150.1; previous revision: 1.1
global/mac/scrollbox/autorepeat-arrow-dn.gif;
new revision: 1.1.4.1; previous revision: 1.1
global/mac/scrollbox/autorepeat-arrow-up.gif;
new revision: 1.1.4.1; previous revision: 1.1
done
Comment 74 Ian Neal 2006-06-25 14:46:39 PDT
Comment on attachment 226635 [details] [diff] [review]
Keep bold style (Checked in trunk & 1.8 branch)

Checking in (trunk)
menu.css;
new revision: 1.42; previous revision: 1.41
done
Checking in (1.8 branch)
menu.css;
new revision: 1.40.28.2; previous revision: 1.40.28.1
done
Comment 75 Stefan [:stefanh] 2006-06-25 14:57:45 PDT
Everything is in on trunk and branch now - thanks!

Note You need to log in before you can comment on or make changes to this bug.