331803 - Missing modifiers in menuitems

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Stefan [:stefanh]]

Firefox and Seamonkey trunk, Mac OS 10.3.9

In context menus that displays shortcuts, accel keys (Commmand etc) are missing. This makes the menuitems look a bit odd - it's just the accel key that is missing. Instead of, for instance, "Command" and "C" you now only see "C".

Steps to repro:

1) Launch Chatzilla from a Firefox trunk build and Ctrl-click in the content area
2) Look at the right side of the menupopup and note that menuitems only displays a single character for the shortcut ("C" instead of "Command C")

First seen: Firefox trunk 2006022306
Does not appear in Firefox trunk 2006022205

This affects Firefox (at least when running Chatzilla), SeaMonkey (Help window context menu, MailNews) and probably Thunderbird (iirc there are shortcuts in some mail folder/threadpane context menus)

Comment 1

[Stefan [:stefanh]]

Attachment: Screenshot of missing accelkeys in Chatzilla context menu

Comment 2

[Asaf Romano (gone)]

Is this really mac-only?

Comment 3

[Karsten Düsterloh]

I cannot confirm this bug on Windows neither with the SeaMonkey nightly of 2006041208, nor with my debug build of 20060304(sic!). I do, however, see it with my SM Mac debug build of 20060409. Classic theme on both systems.

Comment 4

[Stefan [:stefanh]]

Attachment: Testcase

This is not restricted to context menus, it appears that we don't show modifiers in XUL menuitems...

Comment 5

[Stefan [:stefanh]]

> First seen: Firefox trunk 2006022306
> Does not appear in Firefox trunk 2006022205

Using the testcase, this seems to be correct.

Comment 6

[Stefan [:stefanh]]

jshin, it seems that the patch in bug 205387 have caused this. Backing out This seems to be a regression from bug 205387 - backing out the changes in gfx/src/mac that was checked in 2006-02-22 18:37 (attachment #212815 [details] [diff] [review]) fixes this.

Comment 7

[Masatoshi Kimura [:emk]]

Attachment: patch (xpfe)

Requesting review since Mac developers said OK.
See bug 205387 comment #51 (and following comments).

Comment 8

[Masatoshi Kimura [:emk]]

Attachment: patch (toolkit)

Same as above, except that this is a toolkit part.

Comment 9

[Stefan [:stefanh]]

Neil doesn't have a mac (he should be able to sr the patch, though), but I can imagine Mano can take a look at this.

Comment 10

[[e-mail]]

Are these character codes correct? Only the first one looks right to me:

⇧⌠␀⌆

Comment 11

[[e-mail]]

Comment on attachment 219348 [details] [diff] [review]
patch (xpfe)

My fault; I meant to type ⇧⌃⌥⌘ which is of course correct.

Comment 12

[Masatoshi Kimura [:emk]]

Changing dependency.
This patch doesn't require the change of bug 205387.

neil:
Can I treat your r+ as sr+ per comment #9?

Comment 13

[Asaf Romano (gone)]

Comment on attachment 219349 [details] [diff] [review]
patch (toolkit)

r=mano on the toolkit patch, checked in on trunk.

Checking in platformKeys.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/global-platform/mac/platformKeys.properties,v  <--  platformKeys.properties
new revision: 1.3; previous revision: 1.2
done

Comment 14

[Asaf Romano (gone)]

Comment on attachment 219348 [details] [diff] [review]
patch (xpfe)

r=mano.

Comment 15

[[e-mail]]

(In reply to comment #12)
>Can I treat your r+ as sr+ per comment #9?
Sure; do you need that checked in?

Comment 16

[Masatoshi Kimura [:emk]]

(In reply to comment #15)
> >Can I treat your r+ as sr+ per comment #9?
> Sure; do you need that checked in?
Thank you. Please check in.

Comment 17

[[e-mail]]

Since both patches are now checked in, resolving as fixed on trunk.

Comment 18

[Jungshik Shin]

Comment on attachment 219349 [details] [diff] [review]
patch (toolkit)


> #the shift key - open up arrow symbol (ctrl-e)
>-VK_SHIFT=\u0005
>+VK_SHIFT=\u21e7

I guess '(ctrl-e)', '(ctrl-g)', etc are not necessary any more.

Comment 19

[Jungshik Shin]

Comment on attachment 219349 [details] [diff] [review]
patch (toolkit)

Asking for branch approval. This is 'the right' thing to do and will prevent a regression when bug 205387 is fixed on branch.

Comment 20

[[e-mail]]

Comment on attachment 219349 [details] [diff] [review]
patch (toolkit)

Don't forget attachment 219348 [details] [diff] [review] ;-)

Comment 21

[Jungshik Shin]

both xpfe and toolkit patches were landed on 1.8.1 branch.
btw, Kimura-san, I'm sorry I forgot to credit you with the patch in cvs check-in comment. 
another btw, I got rid of '(ctrl-....)'.