118025 - NS_THEME_MENU* implementations (Mac)

Status: RESOLVED FIXED

(Core Graveyard :: Skinability, enhancement)

Description

[Hixie (not reading bugmail)]

This covers the implementation of NS_THEME_MENUBAR, NS_THEME_MENU_POPUP and
NS_THEME_MENUITEM for XUL menus on Mac.

Note: NS_THEME_MENUBAR may not be appropriate for this platform.

Comment 1

[Mike Pinkerton (not reading bugmail)]

no way, no day.

Comment 2

[m_mozilla]

any comments on the bugs which this blocks?

are there better alternatives, or should they be WONTFIX'ed as well?

    bug 34572
    bug 117584

-matt

Comment 3

[Ari]

Don't we need this to make xul popup menus look like native menus, or is that
not the plan?

Comment 4

[Mike Pinkerton (not reading bugmail)]

oh duh, i guess we need this for menu bg's. 

::DrawThemeMenuBackground().

my bad.

Comment 5

[Mike Pinkerton (not reading bugmail)]

where did 099 go? was i asleep? was i dreaming? this is not my beautiful house.
this is not my beautiful wife.

Comment 6

[Mike Pinkerton (not reading bugmail)]

wrong keyword

Comment 7

[Peter Trudelle]

nsbeta1- per ADT triage team

Comment 8

[Mike Pinkerton (not reading bugmail)]

i would like this for rtm. how do i mark that?

Comment 9

[saari (gone)]

ADT sez we're planning using nsbeta1 all the way through

Comment 10

[Mike Pinkerton (not reading bugmail)]

then this should be +

Comment 11

[saari (gone)]

How horrible will things look if it isn't implemented? 

Comment 12

[Asaf Romano (gone)]

Attachment: partial patch (see comment 15)

I'm not passing the right *menu* rect to DrawThemeMenuItem, but 1) So far, i'm
not able to get this rect 2) It seems it doesn't affect anything.

We need this patch in order to improve the menus in pinstripe (which hardcodes
menubackgorund and the hover-color).

Comment 13

[Asaf Romano (gone)]

Attachment: patch for pinstripe

The most important improvment - we now follow the theme hover color (instead of
hard-coding it in the stylesheet).

Comment 14

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 172744 [details] [diff] [review]
partial patch (see comment 15)

looks ok to me. not sure why i couldn't get this working a few years back.

r=pink

Comment 15

[Asaf Romano (gone)]

Comment on attachment 172744 [details] [diff] [review]
partial patch (see comment 15)

I'll add the missing setthemebackround calls (for calls) on checkin

Comment 16

[Asaf Romano (gone)]

simon! :)

Comment 17

[Simon Fraser [no longer active]]

Comment on attachment 172744 [details] [diff] [review]
partial patch (see comment 15)

>Index: gfx/src/mac/nsNativeThemeMac.cpp

>+void
>+nsNativeThemeMac::DrawMenu ( const Rect& inBoxRect, PRBool inIsDisabled )
>+{
>+  ::EraseRect(&inBoxRect);

Should you set the background color before calling EraseRect()?

>+  ThemeMenuType menuType = inIsDisabled ? kThemeMenuTypeInactive : kThemeMenuTypePopUp;
>+  ::DrawThemeMenuBackground(&inBoxRect, menuType);
>+}
>+

>+  // XXX: pass the right menu rect!
>+  ::DrawThemeMenuItem(&inBoxRect, &inBoxRect, inBoxRect.top,
>+                      inBoxRect.bottom, menuItemState, itemType, NULL, 0);

Does the commment mean that this still needs fixing?

sr=sfraser with those addressed.

Comment 18

[Asaf Romano (gone)]

(In reply to comment #17)
> (From update of attachment 172744 [details] [diff] [review] [edit])
> >Index: gfx/src/mac/nsNativeThemeMac.cpp
> 
> >+void
> >+nsNativeThemeMac::DrawMenu ( const Rect& inBoxRect, PRBool inIsDisabled )
> >+{
> >+  ::EraseRect(&inBoxRect);
> 
> Should you set the background color before calling EraseRect()?

yeah, as i mentioned in comment 15. fixing on checkin.

> >+  ThemeMenuType menuType = inIsDisabled ? kThemeMenuTypeInactive :
kThemeMenuTypePopUp;
> >+  ::DrawThemeMenuBackground(&inBoxRect, menuType);
> >+}
> >+
> 
> >+  // XXX: pass the right menu rect!
> >+  ::DrawThemeMenuItem(&inBoxRect, &inBoxRect, inBoxRect.top,
> >+                      inBoxRect.bottom, menuItemState, itemType, NULL, 0);
> 
> Does the commment mean that this still needs fixing?

Apparently, the Appearance Manager (on OS X) ignores the menu rect. Anyway yes;
and I'll probably address this, in this bug, in the 1.9 cycle.

Comment 19

[Asaf Romano (gone)]

checked in those, leaving open for further work.

Comment 20

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Mano, is this bug still relevant for anything (in which case it should move from the Graveyard), or should we just call it FIXED?