Build ID: 2000080208 M18 win98 The accelerator key combos for Back/Forward (which uses the arrow keys) and Home (which uses alt+home) on the Go menu are just "Alt+". Ben sez it might just be a general problem with not displaying special keys (like home and the arrow keys) in the accelerator key portion of menu items.

Yes, it is a general problem as there are no direct ASCII representations for things like the arrow keys. Special code will have to be added to turn those into things like "<-" for the arrow keys, or even images if people feel that is a better approach.

punt

reassigning to me, helpwanted. cc'ing dean tessman in case he wants to take a look at this (pink says a good place to start would be nsMenuFrame::BuildAcceleratorText, http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsMenuFrame.cpp#1329 )

We'll see what happens when I get home tonight. Hopefully there's a successful build sitting there. This doesn't sound like it would be overly difficult. Can anyone point me to a section in a XUL file for, say, Alt+Left, so I can see how these key bindings are described?

Great. I've finally got a reliable build going. Anyone care to point me to that XUL file now?

In the case of goforward/goback, these key bindings are pulled in by chrome://navigator/content/navigator.xul pulls in chrome://navigator/content/navigatorOverlay.xul which in turn pulls in chrome://navigator/content/platformNavigationBindings.xul navigatorOverlay.xul sets up the <menuitem>s for the Go menu, and the actual spec of the keycodes is in platformNavigationBindings.xul (different for each platform). On windows (which you work on right?), the keyset has keys like: <key id="goBackKb" alt="true" keycode="VK_LEFT" observes="Browser:Back"/> <key id="goForwardKb" alt="true" keycode="VK_RIGHT" observes="Browser:Forward"/> Hope that helps. Let me know if you need more info, perhaps the full bindings files for Mac and Unix. (And corrections to what I've said are welcome pink/saari).

That's exactly what I was looking for, John. Thanks. Shouldn't there be some place that it says that the text description for goBackKb is "Left Arrow" or something? I can't find that anywhere in the current chrome directory. But if I change platformNavigationBindings.xul to have key values for each <key> item, then I can get a result. eg. before: <key id="goBackKb" alt="true" keycode="VK_LEFT" observes="Browser:Back"/> after: <key id="goBackKb" alt="true" keycode="VK_LEFT" key="Left" observes="Browser:Back"/> I'm guessing that instead of having these key values hardcoded in the <key> elements, they should be stored in a .dtd somewhere, for example platformNavigationBindings.dtd... Sound right? platformNavigationBindings.dtd doesn't exist, so if there's another place that they should go then let me know. Maybe it's navigator.dtd. There are a couple of problems here: 1. It uppercases "Left" to read "LEFT". This should be easily addressed by uppercasing only if the keyChar has a length of one. 2. I can't put key="<" in the <key> element. This raises an assertion along the way. key=">" works fine. That screams bug to me. The assertion is "###!!! ASSERTION: element is null: 'element', file f:\mozsrc\mozilla\rdf\content\src\nsXULDocument.cpp, line 5301" (side note: I'd really like the left and arrows to read "<--" and "-->". They're a little more clear than the "<-" and "->" used in 4.x.) If I'm on the right track, I'll accept this bug and address at least the .xul and .dtd changes (assuming someone gives me proper .dtd names to use) and problem #1 above. As for the assertion in #2, if someone else has a chance to look into it, that would be spiffy!

We could also do what IE does, namely alt+Left Arrow and alt+Right Arrow. It doesn't look all that bad, and it's a bit clearer as well.

Yes, this works for me. We'll have to implement my patch, otherwise it will appear as "Alt+LEFT ARROW".

Anyone have any comments on which .dtd file to use? Is navigator.dtd fine, or should I create a new one or two?

(Mike: corrections welcome ...). I don't think there is code to map a text description of a key binding to its display value. You can use an entity to specify the value of the key attribute, and unix does this. See: http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/locale/en-US/unix However, in specifying the 'key', the keycode is then ignored (right? or am I dreaming). (i.e., key="foobar" keycode="VK_LEFT" means that "FOOBAR" is shown in the menu, but the key event never happens since you can generate ALT+FOOBAR as a single keystroke). Perhaps what needs to be done is (ugh) to hard-code the mapping of some common VK_* to short English mnemonics somewhere in the code that handles the drawing of the menus: e.g. VK_LEFT = "<-", VK_DELETE = "Del", VK_F5 = "F5", etc. (Mike? Better options?) This is bad for localization, and in the longer view, drawing images might be better (e.g., some of these keys, like "Backspace" have no unambiguous abbreviated form). But for current purposes, perhaps the simpler solution is OK for now. (As far as the assertion goes, that assertion is correct: the character "<" is illegal in XML content, it can only be used for markup, and the .dtd is an XML parsed file. However, there is the built-in-to-XML entity "&lt;" that you may use instead of "<", and get the correct result.)

> since you can generate since you can't generate

Huh. You think I would have checked to see if the shortcut key still worked, eh? It was late.

Uh, or just use acceltext on the menuitem, and be done with it.

Oh sure, if you want to take the easy way out (!).

*** Bug 49100 has been marked as a duplicate of this bug. ***

nominating for beta3.

*** Bug 50896 has been marked as a duplicate of this bug. ***

*** Bug 51480 has been marked as a duplicate of this bug. ***

I'm stuck here. Dean, any ideas?

If I could get my .xul changes recognized, then maybe. But I can't. I've deleted all the .jar files, disabled the XUL cache, but still nothing...

Shouldn't I just have to change the line in navigator.dtd: <!ENTITY goBackCmd.label "Back"> to something like: <!ENTITY goBackCmd.label "Backity"> to change the menu text?? No matter what I try, it always reads "Back". I've deleted my profile, disabled XUL caching, deleted all .jar files, deleted the standard .dat files, ...? Is there something lingering around from when I had XUL caching enabled?

Didn't Akanna add a pref setting somewhere that allows changing the shortcut key modifier from (eg. on Windows) Ctrl to Alt? If I'm not thinking of something else and there is indeed this pref, won't we have to take that into account? That means that we won't be able to hardcode "Alt+Left Arrow", only the "Left Arrow" part.

I thought "xulkey" is already mapped to Ctrl, Alt or some SUN key?

Dean: have you tried setting MOZ_DISABLE_JAR_PACKING to 1? I was never able to build with JAR packing once it was turned on, but everything works fine now that I disabled it. [incidentally, i'm not sure i like `backity' as the new back text, even if it is cute ;]

I'm not even doing any building. I'm just going in and modifying the files in dist\...\chrome directly. This used to work fine for me. What I don't understand is where the heck is it getting the plain "Back" text from?

Um. that's a damn fine question, I sure can't find it. John, do you know?

The bug on setting the XULkey (with instructions for changing the prefs) is bug 22515. You are correct that you can't hardcode the alt+ or ctrl+ part if you're using the xulkey. I don't know what the code is to set the UI strings, but you can probably tell by looking at the existing menus (if you can unravel the circuitous path through the DTDs).

In theory, anyways. The menu should be reading off of &goBackCmd.label, which is in navigator.dtd. But when I change that label in navigator.dtd, only the button text changes -- not the menu. And of course, `Back' produces far too many results in lxr. Where oh where is this text coming from?

It's not just me - yay!!

Adding hyatt.

My guess is that it's coming from en-US.loc, some sort of compiled locale file. Because when I delete that, I lose pretty much all text in the browser. Now the question is, how do I get rid of the need for that, if that really is where things are pulling from?

Yeah, that's it. It's a zip file with all of the .dtd and .properties files in it.

Man, that's a pain to update that .loc file each time I want to make a change...

Dean: I assume .loc is a JAR package extension. Since I build with JAR packaging off, which specific dtd in that package holds the "Back" string for the Go menu (so I, too, can edit it)?

navigator.dtd, in navigator\locale. (I finally have my "Backity" menu!)

My apologies for the large diversion about my chrome problems. For those that are interested, I'm OK now. I clobbered everything and rebuild in debug mode (was doing optimized) with jar packaging disabled (MOZ_DISABLE_JAR_PACKAGING). Now things work properly for me. Better than I expected, actually. If I change tasksOverlay.xul, all I have to do is open a new window to see the changes - I don't even have to exit!

Marking nsbeta3- per PDT review.

The quickest fix would be to do what John suggested, using the acceltext on the menuitem. The problem with this is that there is now a pref for what the modifier key is for accelerators (bug 22515). So we couldn't hardcode "Alt + Left Arrow", but we could hard-code "Left Arrow". But this would lead to incorrect text because of http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsMenuFrame.cpp#1348 which kicks out if any acceltext for the menu item is specified. But I think that John's on the right track. We should be able to use the acceltext but only specify the text for the key and not any modifier text. And the more that I look at it at this early hour, the more I think that it should be fairly simple... 1. We just shouldn't kick out as soon as we have acceltext. So get rid of lines 1350 and 1351 and whatever surrounds. 2. Instead, we should be able to use that with getElementById to then get the modifiers, since we'll be ignoring the key value if we have a value for acceltext. In theory this part should just require changing line 1370 from xulDocument->GetElementById(keyValue, getter_AddRefs(keyElement)); to something like if (!accelText.IsEmpty()) xulDocument->GetElementById(accelText, getter_AddRefs(keyElement)); else xulDocument->GetElementById(keyValue, getter_AddRefs(keyElement)); 3. Wrap the getting of keyChar (starts at line 1399) and the subsequent return if keyChar is empty in "if (!accelText.IsEmpty())". 4. Do something similar at line 1447, so that we use accelText if it's not empty, otherwise use keyChar. Of course, if CVS wasn't so bloody slow for me these days, I could get around to actually trying this idea and submitting a patch if it works. In the interim, does this sound like it's something that would work? As I mentioned before, this makes sense to me _in_theory_.

Nominating for rtm.

Just a warning that I might not have time to look at this for a couple days, at which point it might be too late... (also, this can't get + or ++ until there's a fix in hand)

Hopefully when I go home tonight I'll finally have a fresh pull. The pull_all will have been running for about 18 hours by then, so I hope it will be finished. It used to take an hour, but lately it's been quite slow. Here's hoping...

Yippeeeee - I finally have a recent pull that built. Taking this bug for at least the next couple of days, until I can try what I spouted out last night. (Blake, hope you don't mind.)

Here's an interesting question. Assuming that what I'm trying here works, I'd need a place to define a language-specific, platform-specific string (eg. "Left Arrow" for English / Windows, "[" for English / Unix, etc.). Is it possible to do that? If so, where would something like that go?

Ugh. GetElementById isn't working when I pass in accelText, but it works fine when keyValue is passed in (step 2 above). I'm stuck right now on how to get the element when I have the accelText.

OK, I understand what this is doing now, which would be the main reason that I now have "Alt+Left Arrow" and "Alt+Right Arrow" appearing beside my Go > Back / Forward menu items. The approach that I took was to add an <accel> item, similar to the <key> items in navigatorOverlay.xul. (This would eventually need to be moved out to some platform and language-specific location.) For now, I'm specifying the pointer to that <accel> in the "acceltext" attribute of the <menuitem>. The old behavior of being able to put text directly into the "acceltext" attribute is still supported. Eventually, if this is deemed the approach to follow, I'd like to create a new "accel" XUL atom. Anyway, I'll attach this as a patch if anybody wants to look at it. You'll also have to make the following addition to navigatorOverlay.xul. After "<!-- Go Menu -->", which is currently an empty section, add: <accel id="accel_goBack" acceltext="Left Arrow" modifiers="alt" /> <accel id="accel_goForward" acceltext="Right Arrow" modifiers="alt" />

Geez, it's another bad night for me. You'll also have to change the "menuitem-back" and "menuitem-forward" <menuitem>s to: <menuitem id="menuitem-back" value="&goBackCmd.label;" accesskey="&goBackCmd.accesskey;" observes="canGoBack" acceltext="accel_goBack" /> <menuitem id="menuitem-forward" value="&goForwardCmd.label;" accesskey="&goForwardCmd.accesskey;" observes="canGoForward" acceltext="accel_goForward" /> The change is the addition of the "acceltext" attribute.

> <accel id="accel_goBack" acceltext="Left Arrow" modifiers="alt" /> Does this mean we are using "alt-left arrow"? I think it would make more sense to use the xulkey (which is usually Ctrl, but might by Meta or whatever).

I'm working on just Windows right now. Why would we stray from what's been the standard shortcut keys for as long as I can remember. Actually, I thought that maybe it should use the xulkey for about five minutes (see bug 55931) but quickly changed my mind.

Cool. The acceltext can't be hardcoded in the xul, though (for internationalization purposes). So those should be in dtd's. Mike, could you review?

Yeah, I know about the hard-coding in the XUL. Read my comment dated "2000-10-11 19:51".

rtm-, don't see the huge importance of this fix for RTM; haven't seen an update in the bug for a week.

How about just adding a value attribute (with the text to display) to the <key> element and check for its existence? The logic would be like: keyString = !key.value.IsEmpty() ? key.value : key.key.ToUpperCase(); and then put the modifiers and keyString in aAccelString. I've got a patch nearly ready which I'll attach later (I'll be gone for a few hours).

Ah, I see now... The built accelerator text is stored in an attribute and you want to re-use that. Okay, my proposed solution: 1) If there's a acceltext attribute in the menu element, use that (either provided or generated), we're done. 2) See if we have a key element, if not, we're done. Note: actually we should do this check first, otherwise people will abuse acceltext to just display stuff without any associated key. 3) Use they key element's value, key or keycode attribute, in that order. 3a) use value as is 3b) use key uppercased 3c) use keycode only if it starts with VK_, cut that off 4) generate the modifiers string (internationalized?), append the result of 3, set the acceltext attribute on the menu element (triggering (1) next time around), and we're done. This may currently have one problem: key elements observe broadcasters, and I suspect there are places where people have put stuff in those broadcasters which doesn't belong there[0], one of them the value attribute with the text that should go on the menu. Those will need to be fixed. [0] They don't belong there because they are copied over to all observers, a key element has no business having a menu label in it.

*laughing* Testing my proposal, I found such funny accelerators as ``Ctrl+Find in This Page'', ``Alt+Back'' and ``Ctrl+Navigator'' So I see where say two or more items would actually appreciate getting the same label and thus getting a value from a broadcaster. That still doesn't mean the key element should get that value. However, all those elements do want to get the oncommand attribute. Solution: two broadcasters: <broadcaster id="foo_cmd" oncommand="doSomething();"/> <broadcaster id="foo_cmdlabel" value="La la la"/> <key id="foo_key" keycode="VK_LEFT" modifiers="alt" observes="foo_cmd"/> <menuitem id="foo_menu" observes="foo_cmdlabel"/> Unless the observes attribute takes a comma/space separated list of ids to observe, in which case you'd have: <broadcaster id="foo_cmd" oncommand="doSomething();"/> <broadcaster id="foo_label" value="La la la"/> <key id="foo_key" keycode="VK_LEFT" modifiers="alt" observes="foo_cmd"/> <menuitem id="foo_menu" observes="foo_cmd foo_label"/> This may be possible, I'll have to test that. There seems to be a third solution, which is less clean imo, but: <key id="foo_key" keycode="VK_LEFT" modifiers="alt"> <observes element="foo_cmd" attribute="oncommand"/> </key> That would be less intrusive and only take what we need from the broadcaster. cc'ing ben & hyatt for their input.

In quick testing, my third suggestion certainly (<observes/>) works, though I'd rather we fix the broadcasters and their clients. The attached patch now makes mozilla display ``Alt+Right'' with value set, ``Alt+RIGHT'' without it set (where keycode="VK_RIGHT", if that were lowercase it will show ``Alt+right'', though I believe those should be uppercase). If both key and keycode are set, it'll display the key (iirc key overrides keycode for keypresses if both are set).

Hm... I don't know that we want stripped down VK_ codes appearing in the UI (at least not until I get a list of all of them and see what they look like visually ;) Windows performs beutification manipulations, e.g. <Esc> or Esc for VK_ESCAPE. A string bundle keyed on the VK_ keycode would allow the provision of correct modifiers, but might be less performant (pink, hyatt?) - maybe not an issue as fallback to VK_* is not common. (e.g. VK_ESCAPE=<Esc>)

jag informs me that is what the 'value' attribute is for ;) in that case, ignore my previous post. cases where special keys are required can attach 'value' attributes.

Somebody please shoot me, I suck.

I've added all that I could to this one. Handing back to Blake.

Taking this bug. Instead of using a ``value'' attribute we could also use say a ``text'' or ``keytext'' attribute. This works around all those values currently copied into the key elements through broadcasters, and something like ``keytext'' would probably be clearer than ``value'' too.

Changed it to use |keytext| instead of |value|, removed a warning I got elsewhere in that file, and using |+= NS_LITERAL_STRING("...")| instead of |AppendWithConversion("...")| Question: why use |GetAttribute(NS_LITERAL_STRING("...", ...)| instead of |GetAttribute(kNameSpaceID_None, nsXULAtoms::..., ...)| ?

There's no xulAtom defined for modifiers, value, etc.

Not yet, but those are easily added :-) Any other reason?

That's the only reason that I had in my code. Since I was experimenting, I didn't want to mess around with atoms at that time.

*** Bug 61215 has been marked as a duplicate of this bug. ***

Is this patch ready to go...?

looks ok, i guess. *shrug* what are the lines at the top of the diff - nsBoxLayoutState state(mPresContext); menuPopup->MarkDirty(state); nsCOMPtr<nsIPresShell> shell; mPresContext->GetShell(getter_AddRefs(shell)); shell->FlushPendingNotifications(); - menuPopup->GetRect(rect); are these intentional? what do they do?

pinkerton: the first removed line causes a warning (shadowing another |state| in a higher scope, which turns out to be the same |state|), the second removed line captures the new |rect|, then throws it away, which seemed rather useless to me. So, any reason we shouldn't use |GetAttribute(kNameSpaceID_None, nsXULAtoms::key, ...)| etc. here?

a=hyatt, although you need to patch the Mac native menu code as well before this feature (keytext) can be used.

Nice... Mac menus only seem to allow one character to be set.

Of course I could hack my way around that by constructing an accelText and appending that to the main menu text, but that's so dirty I should probably wash my mouth now. What we could do is add support for keycode (VK_*), and, assuming Mac knows how to display something sensible for those non-printable keys, that would solve the problem too, leaving keytext to be the hack it is (I still prefer it over a keys.properties with printable text for all VK_* though) for non-Macs.

I like this patch - thanks! However, a small change could make it even more useful! If we converted the string to "proper-case", it would provide acceptable results in more cases, avoiding "keytext". This would allow the following results to look better: TAB, DELETE, ESCAPE, ENTER, HOME, END, LEFT, RIGHT, UP, INSERT, DOWN, plus a few other quesionable ones. It still leaves plenty that _dont_ give a good result (PAGE_DOWN etc), and a couple may be questionable ("Escape" may still be overridden manually to "Esc", as Ben said) but it still appears a worthwhile improvement. It appear that some earlier patches _did_ handle this (eg, the comment "Only uppercase keyCode if it has a length of 1") - hence I haven't bothered with a patch as I may be missing something. However, I would be happy to provide one if the idea was looked on favourably.

I've been thinking about that, and can easily do it if that is really desirable. I haven't made my mind up yet if it really is good. I'm actually thinking about taking out the keycode bit and force keytext to be used to make life easier for translators.

Mark Hammond and I have been discussing this and he pointed out something which has been nagging me too: <key id="key_foo" keycode="NS_FOO" keytext="&foo.keytext;" observes="cmd_foo"/> You're stuck with having to specify the "key" twice, once in keycode, once in keytext. We expect that most of the time, when you want NS_FOO, you also want &foo.keytext; So how about we have a keys.properties with a mapping, and this logic: var text; if (keytext) text = keytext; // allow override for both |key| and |keycode| else if (key) text = key.toUpperCase(); else if (keycode) text = keyStringBundle.getString(keycode.toUpperCase()); if (text) { accelText = modifiers + text; // do stuff } I'm not too happy with the stringbundle there, but it beats hardcoding as suggested earlier. On the other hand, keycode isn't used that often, and assuming chrome://communicator/content/keytexts.dtd, specifying the keytext attribute could be close to mechanical (for keycode="VK_FOO", have keytext="&foo.keytext;"), so I think having to specify it isn't that big an issue. The change to the above logic would be to remove the |else if (keycode)| block. hyatt, saari, pinkerton, what do you think?

This could work. However, you're placing knowledge of what is displayed in a menu in the xul key tag, which is a little weird.

With the keytext attribute I'm not really putting knowledge of what a menuitem should display in the key element, the key element provides a human readable text of itself which a menuitem (but also others) can use to display instead of the key or the keycode attribute. I discussed this with saari in irc and he understood this view.

New version coming up which uses atoms to get attributes (slightly faster) and I'm providing a default keys.properties with all the keycodes we currently use and then some.

some comments about your string bundle code. 1) don't define a static CID, use the string bundle contract id. nsCOMPtr<nsIStringBundleService> bundleService = do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv); 2) use nsXPIDLStrings, not PRUnichar *. nsXPIDLString str; rv = bundle->GetStringFromName("foo", getter_Copies(str));

r=sspitzer on the string bundle parts.

r=jst for all changes here (the last patch plus the changes that are only in the third-to-last attachment).

sr=hyatt

Thanks hyatt! Checked in, marking fixed.

Works great for me on Win32. Nice work, Jag! I've got F9 for My Sidebar (yay -- an easy way to open up My Sidebar </sarcasm>), three items under the Go menu, "Del" and some others in Composer, and the list goes on. I can't confirm this on Mac or *nix, though, so I won't verify it. This is XP, right, even though the OS for this bug is Win98? Build: 2001010820 Win2000 sp1

No, unfortunately this is only for those platforms using the XP menu code. Mac does its own menus, for which Saari opened 62030. Any other platforms which don't use the XP menu code will need a special treatment.

So this at least needs to be verified on *nix, then. Anyone...?

The Go menu on Linux shows strings like Alt+Left Arrow and Alt+Home (I have accel=alt). That's the desired behavior, right? Verified on Linux. I'll go ahead and mark verified since it doesn't apply to Mac native menus.

Akkana, can you do a quick check to make sure this displays properly on Linux when your accelerator is set to Ctrl?

With accel=ctrl, the strings in the Go menu correctly display as Alt+Left Arrow, Alt+Right Arrow, and Alt+Home