451124 - Fix vertical alignment of context menu submenus

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Markus Stange [:mstange]]

Attachment: fix

On Mac OS X, native context menus are vertically offset by -4px. We should do the same; it looks better, especially when opening sub menus.

Comment 1

[Mike Connor [:mconnor]]

Comment on attachment 334373 [details] [diff] [review]
fix

good catch, thanks!

Comment 2

[Markus Stange [:mstange]]

Attachment: screenshot

(The rounded corners in the screenshot are from bug 391984 and should be ignored.)

Comment 3

[Markus Stange [:mstange]]

Wow, that was fast! :)

Comment 4

[Markus Stange [:mstange]]

Argh... the patch breaks a lot of popup menu mochitests.
I need to fix them before this can land.

Comment 5

[Markus Stange [:mstange]]

Attachment: fix tests

This makes the tests pass.

I've changed the tests to check the popup's margin box instead of its border box. I created two helper functions, getMargins() and getMarginBox(), and put them in popup_shared.js.
The screen position test in test_popup_scaled.xul is now cheating a little (because it needs to know the scale factor in order to pass), but I couldn't find a better way of doing it.

Comment 6

[Neil Deakin]

Huh, if you're trying to change the offset of context menus, why are you changing the top position of all menus?

Comment 7

[Markus Stange [:mstange]]

Oh... sounds like a good idea. How can I tell context menus from the other menus?

Comment 8

[Neil Deakin]

You would need to change the context menu offset at http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsMenuPopupFrame.cpp#1000

Comment 9

[Markus Stange [:mstange]]

Thanks, I've just tested it. However, that doesn't really do what I need here; it only seems to work for top level context menus. I need the offset to apply to sub menus, first and foremost.

And I'd rather have the offset in the CSS, since it's platform and theme-specific.

Comment 10

[Neil Deakin]

OK, so now I'm confused. What do you want this to apply to? Context menus, or submenus?

Comment 11

[Markus Stange [:mstange]]

Both :)

But submenus are more important: I want the blue bar to look like in the screenshot.
The offset for top level context menus is not really important, but that's what native context menus do.

Comment 12

[Markus Stange [:mstange]]

Attachment: Only align sub menus

Let's do this instead. Are all the combinations of popup and menupopup necessary?

Comment 13

[Neil Deakin]

Comment on attachment 334804 [details] [diff] [review]
Only align sub menus

There should be spaces around the >

'popup' is deprecated so I wouldn't bother with those ones, except maybe 'popup menu > menupopup' which is still used in places.

You may want to ask the other Neil as he's more knowledgeable about theme issues.

Comment 14

[Markus Stange [:mstange]]

Attachment: updated patch

Comment 15

[[e-mail]]

(In reply to comment #13)
> 'popup' is deprecated so I wouldn't bother with those ones, except maybe 'popup
> menu > menupopup' which is still used in places.
Right, our 131 popups (fx+tb+sm+cr) are all context menus, so they never have menu parents, but I would suggest using child selectors before menu too.

Comment 16

[[e-mail]]

In fact bug 253661 fixed this for windows, but I notice it also used a -moz-margin-start, presumably to alter the overlap of the submenu.

Interestingly for menus near the bottom of the screen, Windows pops the submenu up (as if it was anchored bottomleft bottomright instead of topleft topright) but we don't seem to do this (I tried in 1.8 and 1.6 too just to be sure).

Comment 17

[[e-mail]]

Comment on attachment 334899 [details] [diff] [review]
updated patch

r=me if you add in the >s to match winstripe.

Comment 18

[Markus Stange [:mstange]]

Attachment: Add >s

Oops. I would have done that the first time but I had forgotten that XBL-generated elements don't count in the selector.

Comment 19

[Stefan [:stefanh]]

http://hg.mozilla.org/mozilla-central/index.cgi/rev/061c59896168