333910 - nsITheme for menu/scrollbox arrows

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Stefan [:stefanh]]

Mano thought (bug 301105, comment #30) it would be possible to use HITheme/Old Quickdraw to get sub-menu arrows and scrollbox arrows.

Comment 1

[Stefan [:stefanh]]

http://developer.apple.com/documentation/Carbon/Reference/Appearance_Manager/Reference/reference.html

Those theme menu item types could be used:

kThemeMenuItemHierarchical
kThemeMenuItemScrollUpArrow
kThemeMenuItemScrollDownArrow

Comment 2

[Stefan [:stefanh]]

Moving this to cocoa since I doubt this will happen on branch.

Comment 3

[Stefan [:stefanh]]

The menu item types i comment #1 still works. But I've found it hard to use kThemeMenuItemHierarchical - NS_THEME_MENUARROW is supposed to just be the arrow and when drawing the menuarrow over the menuitem I get some weird artifact beside the arrow. CUIDraw feels a lot easier here (Markus showed me what icon to use here - it's "image.MenuSubmenu"). For the scroll up/down arrows I'd suggest a NS_THEME_MOZ_MAC_MENUSCROLLER_UP/DOWN (or something like that).

Comment 4

[Stefan [:stefanh]]

Attachment: wip

First iteration. This works, except that the vertical position of the up/down arrows doesn't exactly matches the OS. Some clean-up needs to be done also.

Comment 5

[Stefan [:stefanh]]

Attachment: 333910.1.diff

Pretty much a final patch (will split it later).

Comment 6

[Stefan [:stefanh]]

I'm going to handle the menu-scrollers  a bit different: I'm just going to do the arrows (with CUIDraw), then we can use the already existing NS_THEME_BUTTON_ARROW_UP/DOWN. New patch coming up soon.

Comment 7

[Stefan [:stefanh]]

Attachment: widget part

Comment 8

[Stefan [:stefanh]]

Attachment: css part

Comment 9

[Stefan [:stefanh]]

Attachment: Adjustments for Bookmarks panel

Comment 10

[Stefan [:stefanh]]

Comment on attachment 8468577 [details] [diff] [review]
widget part

I think this is ready for review now. We center the icons if the macRect is wider/higher than the drawRect. For the menuarrow, I flip the cgContext in rtl mode, not sure if that's evil or not.

I'll wait a bit before I'll let Dao look at the css part (Gijs will look at the Bookmarks panel adjustments).

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 8468577 [details] [diff] [review]
widget part

Instead of doing different things based on aImageName I think it would be better to have the caller of DrawMenuIcon choose the intended behavior. For example, you could create an enum MenuIconDrawingFlags { CENTER_HORIZONTALLY, RESPECT_RTL } inside nsNativeThemeCocoa and pass the right values in a "uint32_t aFlags" argument.

Comment 12

[Stefan [:stefanh]]

Ah, yes - will do.

Comment 13

[Stefan [:stefanh]]

Sorry for the delay here, but I found an issue with drawing menu icons with CUIDraw on earlier OS X versions (bug 1054733). Once bug 1054733 is resolved I'll post an updated patch here.

Comment 14

[Stefan [:stefanh]]

Attachment: New version (complete)

Here's a new patch - just putting it here for reference. Will split later once I've done some tests.

Comment 15

[Stefan [:stefanh]]

Attachment: Widget part - new version

I used a bool instead of an enum - we only care of the rtl if we don’t center the icon horizontally.

It appears that Apple use a separate image for the menu arrow in rtl mode, so I’m doing that as well. Also, the left/right arrows that we use have different width on 10.6:

10.6.8:
"NSCoreUIImageRep 0x1a1651a0 Size={8, 10}”
10.7.0:
"NSCoreUIImageRep 0x1d52a570 Size={9, 10}”

I had to special-case that in the css as well since there’s 1px less right margin in 10.6-menus.

I’m a bit bothered by how I handle these differences in nsNativeThemeCocoa. Is there a better way?

Comment 16

[Stefan [:stefanh]]

Attachment: css part - new version

Here's the css part. I'll let Dao look at the toolkit part and Gijs at the browser part.

Comment 17

[:Gijs (he/him)]

(In reply to Stefan [:stefanh] from comment #16)
> Created attachment 8485854 [details] [diff] [review]
> css part - new version
> 
> Here's the css part. I'll let Dao look at the toolkit part and Gijs at the
> browser part.

Did you mean to set review? flags or are you waiting for the other patch? :-)

Comment 18

[Stefan [:stefanh]]

(In reply to :Gijs Kruitbosch from comment #17)
> Did you mean to set review? flags or are you waiting for the other patch? :-)
I'm waiting. Feels safer to let Markus look at the widget part first :-)

Comment 19

[Markus Stange [:mstange]]

Comment on attachment 8485853 [details] [diff] [review]
Widget part - new version

Review of attachment 8485853 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Comment 20

[Stefan [:stefanh]]

Comment on attachment 8485854 [details] [diff] [review]
css part - new version

Dao, mind take a look at the toolkit part? Regarding the scrollbox changes: I have to admit I haven't tested them, but I don't think anyone uses the autorepeatbuttons outside menus, so I've just use the same icons as the ones for the scrollbutton-up/down.

Gijs: re the browser changes. You shouldn't really see any difference here, apart from the up/down arrows looking better. Atm there are no rule covering the disabled state (same as before), but that's possible to achieve if you care about it.

Comment 21

[:Gijs (he/him)]

Comment on attachment 8485854 [details] [diff] [review]
css part - new version

Review of attachment 8485854 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!

Comment 22

[:Gijs (he/him)]

Comment on attachment 8485854 [details] [diff] [review]
css part - new version

Review of attachment 8485854 [details] [diff] [review]:
-----------------------------------------------------------------

Per IRC discussion, I'm looking at the toolkit changes, too.

Generally, this looks fine. Should be landable this week.

I admit I don't really understand why the scrollbox changes are necessary... are they just simplifications? I only found horizontal scrollboxes (e.g. the inspector pane in the devtools) so it's hard to be sure what you're changing there. It looks OK, but I'd feel happier if you explained why you changed what you did change :-).

There are some other comments here though, so I'm holding off on granting review - can you let me know thoughts on the below and/or update the patch?

::: toolkit/themes/osx/global/global.css
@@ +312,5 @@
>  .popup-internal-box > autorepeatbutton {
>    height: 15px;
>    position: relative;
> +  list-style-image: none;
> +  -moz-image-region: auto;

It's not clear to me why you've set -moz-image-region everywhere. When looking with DOMI, there are very, very many of these rules being applied in the menus I've checked. Why is it necessary at all? I would have thought that considering the list-style-image:none rule, setting the image region is unnecessary.

::: toolkit/themes/osx/global/menu.css
@@ +80,4 @@
>    -moz-box-pack: end;
>  }
>  
> +/* The native menuarrow we use is 9px wide on 10.7+ and 8px wide on 10.6. */

This comment is odd. The margin is normally 9px, and for a smaller arrow you're making it bigger?

Also, we tend to use media queries to check for lion, so:

@media (-moz-mac-lion-theme) {
/* CSS goes here */
}

::: toolkit/themes/osx/global/scrollbox.css
@@ +44,5 @@
>    -moz-image-region: auto; /* cut off inheritance */
>  }
>  
>  /* Vertical disabled */
> +

Nit: no need for extra whitespace

Comment 23

[Stefan [:stefanh]]

(In reply to :Gijs Kruitbosch from comment #22)
> Comment on attachment 8485854 [details] [diff] [review]
> css part - new version
> 
> Review of attachment 8485854 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Per IRC discussion, I'm looking at the toolkit changes, too.
> 
> Generally, this looks fine. Should be landable this week.
> 
> I admit I don't really understand why the scrollbox changes are necessary...
> are they just simplifications? I only found horizontal scrollboxes (e.g. the
> inspector pane in the devtools) so it's hard to be sure what you're changing
> there. It looks OK, but I'd feel happier if you explained why you changed
> what you did change :-).

Absolutely :-) I was actually a bit unsure of how to handle this, but I think it's the right approach: Since I remove the autorepeatbutton image and the new -moz-appearance rules are for menus, I use the scrollbutton icons instead. So the changes in scrollbox.css are only about that - these changes should not affect .scrollbutton-up/down at all. I simply make the autorepeatbutton-up/down use the same icons as the scrollbutton-up/down. I honestly don't know if anyone uses the autorepeatbuttons for anything else than menus, but... I think the normal way to use scrollboxes outside menus is to use an arrowscrollbox with clicktoscroll="true", which is what the inspector scrollbox does. That would give you a different binding than what menupopus use.

> 
> There are some other comments here though, so I'm holding off on granting
> review - can you let me know thoughts on the below and/or update the patch?
> 
> ::: toolkit/themes/osx/global/global.css
> @@ +312,5 @@
> >  .popup-internal-box > autorepeatbutton {
> >    height: 15px;
> >    position: relative;
> > +  list-style-image: none;
> > +  -moz-image-region: auto;
> 
> It's not clear to me why you've set -moz-image-region everywhere. When
> looking with DOMI, there are very, very many of these rules being applied in
> the menus I've checked. Why is it necessary at all? I would have thought
> that considering the list-style-image:none rule, setting the image region is
> unnecessary.

Yeah, that's for resetting any potential inheritance. "auto" is default and you normally do this because you don't want any -moz-image-region rule to be inherited from a parent. It might work fine without it in those cases - I see now that the autorepeatbutton already have this in scrollbox.css, so I can check and see if I can skip some of those rules.


> ::: toolkit/themes/osx/global/menu.css
> @@ +80,4 @@
> >    -moz-box-pack: end;
> >  }
> >  
> > +/* The native menuarrow we use is 9px wide on 10.7+ and 8px wide on 10.6. */
> 
> This comment is odd. The margin is normally 9px, and for a smaller arrow
> you're making it bigger?

I had to take a look at my different screenshot again, since I became unsure of why I special-cased this. I make the space to the right of the menuarrow the same on 10.6 and 10.7+. This is a negative margin, so by using 1px more negative margin I shrink the distance by 1px. I re-checked the different "native" distances on 10.6, 10.7.5 and 10.9.4 and it's a bit messy. In native menus it's 11px on 10.6, 10px on 10.7.5 and 12px on 10.9.4/5. I think it's reasonable to use the same distance here, so I've picked 12px (most "modern").
 
> Also, we tend to use media queries to check for lion, so:
> 
> @media (-moz-mac-lion-theme) {
> /* CSS goes here */
> }
Ah, ok - sure. I'm actually checking for 10.6, but "@media not all and (-moz-mac-lion-theme)" should work, I guess.


> ::: toolkit/themes/osx/global/scrollbox.css
> @@ +44,5 @@
> >    -moz-image-region: auto; /* cut off inheritance */
> >  }
> >  
> >  /* Vertical disabled */
> > +
> 
> Nit: no need for extra whitespace

Ah, sorry - will fix!

Comment 24

[Stefan [:stefanh]]

Attachment: css part, updated

- Skipped all added -moz-image-region's
- Now using a media query
- Fixed whitespace (found some more - I removed too few lines)

If the comment is unclear I can change it to something else.

Comment 25

[:Gijs (he/him)]

Comment on attachment 8498257 [details] [diff] [review]
css part, updated

Review of attachment 8498257 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks for the patch!

I noticed I bitrotted the widget part a tiny bit because I changed the tooltip thing which is in the context there, so I've just gone ahead and folded the patches and pushed with the nit below:

remote:   https://hg.mozilla.org/integration/fx-team/rev/af792af2f5c7

::: toolkit/themes/osx/global/menu.css
@@ +80,4 @@
>    -moz-box-pack: end;
>  }
>  
> +/* The native menuarrow we use is 9px wide on 10.7+ and 8px wide on 10.6. */

Nit: maybe add: so the arrow needs to be 'pulled in' one pixel more by the negative margin in order to align properly

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/af792af2f5c7