106327 - VK_RETURN key (13) wrongly labelled "Return" on Windows and Linux, should be labelled "Enter" (e.g. to display "Ctrl+Enter" keyboard shortcut correctly for File | Send Now)

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[:deanis74]

I don't have a Return key on my keyboard.  I have an Enter key.  The shortcuts
in the File menu should be Ctrl+Enter and Ctrl+Shift+Enter.

Comment 1

[Jean-Francois Ducarroz]

-->varada

Comment 2

[:deanis74]

Changing VK_RETURN to VK_ENTER in messengercompose.dtd should do it.

Comment 3

[Ivan Tarapcik]

changing VK_RETURN to VK_ENTER changed shortcut text in 'File' menu, anyway the 
shortcut is not working.

Comment 4

[:deanis74]

The fact that it's not working is bug 50255.  This bug is just about changing
the text in the menu.

Comment 5

[Olga]

When I worked on this issue I checked keyboards around.  Only Mac has 'Return'.
Ctrl+Enter should be for Windows and Linux.
Ctrl+Return should be for Mac systems.

Comment 6

[Olga]

One more correction for Mac:
Cmd+Return should be for Mac systems (that special character).

Comment 7

[varada]

jennifer are you in agreement to change the wording in the file menu system?

Comment 8

[jglick]

Olga is correct. "Ctrl+Enter" for Pc/Linux, "Cmd key + Return" is only on Mac.

Comment 9

[Michael Buckland]

Discussed in Mail News bug meeting with QA, PjM, and Engineering.  Decision was
to minus this bug and move to 1.2.

Comment 10

[Sheela Ravindran]

nominating for the next release

Comment 11

[(not reading, please use [e-mail] instead)]

taking all of varada's bugs.

Comment 12

[esther]

Still a bug in 20030107 trunk builds for linux, winxp and mac osx  none of them
have the correct wording.  linux and xp still refer to the Return key Mac OSX
have no shortcut at all.

Comment 13

[Samir Gehani]

Mail triage team: nsbeta1+/adt3

Comment 14

[Brodie]

Attachment: patch 1

This is a patch to change VK_RETURN to VK_ENTER as described above. How do I
special case the Mac platform to leave it as VK_RETURN? Is there anything else
I need to do?

Comment 15

[[e-mail]]

Comment on attachment 149320 [details] [diff] [review]
patch 1

Our VK_ENTER is not really a key - none of our implementations generate it, see
bug 105990. In the mean time we use VK_RETURN everywhere.

Comment 16

[[e-mail]]

In theory you could make VK_RETURN display as Enter in Windows menu shortcuts
but that would involve moving keys.properties to global-platform.

Comment 17

[Brodie]

Would that be the best way to fix this then? I'm willing to do the legwork if 
someone can tell me what I need to do.

Comment 18

[[e-mail]]

Actually, because the Mac doesn't use keys.properties you could just rename the
Return entry to Enter. It would sure as hell be confusing though.

Why do we have both VK_RETURN and VK_ENTER anyway? Do we need them both?

Comment 19

[:deanis74]

That's always confused me, too.

Comment 20

[[e-mail]]

(In reply to comment #18)
>Actually, because the Mac doesn't use keys.properties you could just rename
>the Return entry to Enter. It would sure as hell be confusing though.
It turns out to be slightly less confusing. VK_ENTER isn't actually generated by
any platform so far as I know, the Enter key always generates VK_RETURN.

Comment 21

[Brodie]

From reading around a bit it seems that, in general, VK_ENTER is the keypad 
enter key and VK_RETURN is the main enter key. In moz they are both mapped to 
VK_RETURN and no-where is VK_ENTER ever generated. This seems to be by design 
because fixing it would create more bugs (I read somewhere).

So for this bug, changing the key.properties file to be "Enter" would be the 
correct solution right?

Comment 22

[[e-mail]]

(In reply to comment #21)
>From reading around a bit it seems that, in general, VK_ENTER is the keypad 
>enter key and VK_RETURN is the main enter key. In moz they are both mapped to 
>VK_RETURN and no-where is VK_ENTER ever generated. This seems to be by design 
>because fixing it would create more bugs (I read somewhere).
I believe you, but I'd still like to know where you read it.
>So for this bug, changing the key.properties file to be "Enter" would be the 
>correct solution right?
Well, until such time as VK_ENTER is actually used ;-)

Comment 23

[Brodie]

As I wrote it I knew that I should have had the proper reference. I've searched 
and can't find the place I read it now. It was in a bug linked from here 
(through some indirections). I can't say whether it was a throwaway comment or 
a policy statement though :-).

Regardless of if VK_ENTER is ever generated or not, the key that generates 
VK_RETURN is actually called "Enter" on keyboards other than Mac (according to 
comment 5), and so it should be named that in keys.properties. If VK_ENTER is 
ever implemented, it should be named to indicated that it is the keypad enter 
key.

Comment 24

[naveen]

Attachment: Better Patch

In my patch it checks for the OS first, if it is MAC OS it display's
ctrl+Return or if it is any other OS other then MAC then it displays it as
ctrl+Enter( As only MAC OS has return button)

But in the other patch which is already present does not check for the OS it
simply displays ctrl+enter

Comment 25

[(not reading, please use [e-mail] instead)]

sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.

Comment 26

[Wayne Mery (:wsmwk)]

patch attachment 184295 [details] [diff] [review] never checked in after approvals.

Comment 27

[Phil Ringnalda (:philor)]

Um, no. A two year old, self-reviewed, self-approved-l10n patch does not constitute checkin-needed.

Comment 28

[Wayne Mery (:wsmwk)]

OK. clearing approvals

Comment 29

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

Should be no functional change, as per previous discussion VK_RETURN and VK_ENTER are mapped to do the same thing. This patch just makes it read Enter on windows/linux and return on Mac (barring bug 282091 on mac though...)

Comment 30

[Magnus Melin [:mkmelin]]

Hm, I wonder if i should have had to bump the entity names for the keycode entities here...

Comment 31

[[e-mail]]

Comment on attachment 344521 [details] [diff] [review]
proposed fix

VK_ENTER is just a curious anomaly. The Enter key really is called VK_RETURN (just check a copy of WinUser.h if you don't believe me).

Comment 32

[Magnus Melin [:mkmelin]]

Comment on attachment 344521 [details] [diff] [review]
proposed fix

Ah yes...

Comment 33

[Magnus Melin [:mkmelin]]

After reading a bit more about this, i wonder if this isn't wontfix? At least according to http://en.wikipedia.org/wiki/Enter_key return and enter are different keys, enter being the one by the numpad.

Comment 34

[Wayne Mery (:wsmwk)]

assignee=nobody, so >status=new

Comment 35

[:aceman]

So is the wanted solution to set VK_ENTER=Return and VK_RETURN=Return on Mac, and VK_ENTER=Enter and VK_RETURN=Enter on other platforms in keys.properties, and make the file preprocessed?
With a comment that the keys are the same at this time.
(For the record, there are 114 uses of VK_ENTER and 313 of VK_RETURN in comm-central. Probably people use them interchangebly.)

Comment 36

[[e-mail]]

(In reply to aceman from comment #35)
> So is the wanted solution to set VK_ENTER=Return and VK_RETURN=Return on
> Mac, and VK_ENTER=Enter and VK_RETURN=Enter on other platforms in
> keys.properties, and make the file preprocessed?
I don't think langpacks like it if you preprocess a properties file, but I'm not actually sure that Macs use keys.properties anyway.

> With a comment that the keys are the same at this time.
> (For the record, there are 114 uses of VK_ENTER and 313 of VK_RETURN in
> comm-central. Probably people use them interchangebly.)
Or perhaps people don't know which one to use.

Comment 37

[:aceman]

(In reply to [e-mail] from comment #36)
> I don't think langpacks like it if you preprocess a properties file, but I'm
> not actually sure that Macs use keys.properties anyway.

Looks like this code constructs the menu strings
http://mxr.mozilla.org/comm-central/source/mozilla/layout/xul/base/src/nsMenuFrame.cpp#987
I do not see why it would not run on Mac (there are even Mac comments).
But what about moving the VK_ENTER and VK_RETURN to global-platform/*/platformKeys.properties ? But yes, that one only contains modifiers so far.

Comment 38

[[e-mail]]

(In reply to aceman from comment #37)
> (In reply to comment #36)
> > I don't think langpacks like it if you preprocess a properties file, but I'm
> > not actually sure that Macs use keys.properties anyway.
> Looks like this code constructs the menu strings
> http://mxr.mozilla.org/comm-central/source/mozilla/layout/xul/base/src/
> nsMenuFrame.cpp#987
> I do not see why it would not run on Mac (there are even Mac comments).
Because Macs use nsMenuItemX.mm most of the time instead. (They use nsMenuFrame.cpp for context menus but those don't normally show shortcuts.)

> But what about moving the VK_ENTER and VK_RETURN to
> global-platform/*/platformKeys.properties ? But yes, that one only contains
> modifiers so far.

Comment 39

[:aceman]

So what do you actually propose?
Just rename VK_ENTER adn VK_RETURN to Enter and be done with it?

Comment 40

[[e-mail]]

Bug 105990 is another request suggesting that they're the same.

I'm sure I asked jst on this once and he didn't want it changed, but my Bugzilla search fu fails me.

Perhaps a post on m.d.platform might trigger some discussion?

Comment 41

[Thomas D. (:thomas8)]

Right, can we please get this entirely hopeless bug out of the way?
We can't be serious to take more than 12 years to fix a broken label for one of the most used keys, no matter how confusing the internals might be. Btw this is a nice example of UX-implementation-level:

> User experience principle: interfaces should not be organized around the
> underlying implementation and technology in ways that are illogical,
> or require the user to have access to additional information
> that is not found in the interface itself.

OK, so here's the fact-finding mission, trying to consolidate all of what has probably been said before, but somehow never came to action...

Needinfo from relevant folks on this bug:
*** Please correct me for each of the following statements if there's anything wrong. ***
If there's nothing wrong, then please lets get this out of the way.

This is the key we are talking about:
DOM Name of key:      VK_Return (13) (see Mozilla documentation, [1])
Location on keyboard: "Enter/Return" key on the *main* part of keyboard

For this key, labels found on the physical keyboard (esp. MAC), or generally agreed oral/written reference to the key ("how we call that key"), differs between MAC and Win/Linux:

Label/common name of physical key on Win/Linux [3]: "Enter" (usually carriage return symbol)
Label/common name of physical key on MAC [2]:       "Return" (often spelled out)

This bug started out from the shortcut key for composition's File > Send now.
Per Thunderbird keyboard shortcuts documentation (maintained by me), and according to general custom and physical keyboard layout as described above, these are the shortcuts [4]:

Win/Linux: Ctrl + Enter
MAC:       Command + Return

Problem reported in this bug:

Keyboard Shorcut displayed on File > Send Now menu:
Win/Linux: Ctrl+Return (this bug: wrong label)

To Windows/Linux users, that's wrong because they've never heard of a "Return" key on their keyboard. All they know about, from documentation and any other reference, is "Enter".
So this is a bug in terms of UX-implementation-level, because user doesn't know nor care about the internal DOM name of this key, which is VK_Return (and rightly so).
So this bug requests to correct the label used by TB when showing the name for VK_Return in the UI.

TB's label for VK_Return is currently defined here:

http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/locales/en-US/chrome/global/keys.properties#61

61 VK_RETURN=Return

That label is used for all OS, which is wrong (this bug).
It should be OS-specific (this bug).
So it needs to moved to the OS-specific properties:

All platforms: http://mxr.mozilla.org/comm-central/find?string=keys.properties&tree=comm-central&hint=
Win: http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/locales/en-US/chrome/global-platform/win/platformKeys.properties
Linux: http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/locales/en-US/chrome/global-platform/unix/platformKeys.properties
Mac: http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/locales/en-US/chrome/global-platform/mac/platformKeys.properties

So the minimal patch for this bug needs to do the following:

1) Remove line 61 from global/keys.properties (perhaps leave a comment that VK_Return is defined in global-platform files)

VK_RETURN=Return   <-- remove this line

2) Add new lines 24/25 in global-platform/win/platformKeys.properties
                   and in global-platform/unix/platformKeys.properties

#the enter key
VK_RETURN=Enter

http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/locales/en-US/chrome/global-platform/win/platformKeys.properties#24
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/locales/en-US/chrome/global-platform/unix/platformKeys.properties#24

CAVEAT1: Now since these are *toolkit* files, we might want to change this in the upstream toolkit files (too), rather than just the comm-central copy of toolkit. I'll leave that to others to figure out. I suppose the problem should not be any different on toolkit itself and hence requires exactly the same solution.

I have deliberately ignored...
DOM_VK_ENTER 	0x0E (14)
...because:
a) it's not relevant for this bug; this bug is only about VK_RETURN (which unfortunately is commonly called "Enter")
b) according to offical MDN documentation for KeyboardEvent [1], DOM_VK_ENTER is
> Reserved, but not used.

CAVEAT2: We probably want to move out VK_ENTER along with VK_RETURN so that they stay together, so as to avoid more confusion.

So please let me know what's still stopping us from fixing this as proposed here, either in TB's toolkit copy, or in the original upstream toolkit, or both.

[1] https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent?redirectlocale=en-US&redirectslug=DOM%2FKeyboardEvent#Key_names
[2] http://g-ecx.images-amazon.com/images/G/01/electronics/apple/apple-11q3-wirelesskeyboard-top-lg.jpg
[3] http://msdn.microsoft.com/en-ca/goglobal/bb964651.aspx
[4] https://support.mozillamessaging.com/en-US/kb/keyboard-shortcuts#w_writing-messages

Comment 42

[Magnus Melin [:mkmelin]]

There is no such thing as comm-centrals copy of the toolkit files. Or there is, but the "copy" is always just pulled in from mozilla-central - so there are no forks of this code (except on special release branches in special situations). 

I doubt any keys should move out to platform files. It's just about using different labels for different platforms in the front-end code.

Comment 43

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #42)
> There is no such thing as comm-centrals copy of the toolkit files. Or there
> is, but the "copy" is always just pulled in from mozilla-central - so there
> are no forks of this code (except on special release branches in special
> situations). 

Good, so we just have to fix it in comm-central, as I suspected in comment 41, Caveat2.
So we're looking at the same set of files in the original toolkit:

http://mxr.mozilla.org/mozilla-central/find?string=Keys.properties&tree=mozilla-central&hint=

> I doubt any keys should move out to platform files. It's just about using
> different labels for different platforms in the front-end code.

I don't understand that "doubt", at all. Can you pls be more specific? Why should we maintain a wrong label in comm-central, which will then force *all the front-end codes to fix the label* ? The label VK_RETURN=Return in comm-central toolkit files will *always* be wrong on windows and linux, so why not fix it at the source?

Iow, is there any scenario where the "Enter" key on *Win/Linux* should be labelled "Return"!?

As we speak, labels for important keys are already distributed between the following files:
global/keys.properties
global-platform/win/platformKeys.properties
global-platform/linux/platformKeys.properties
global-platform/mac/platformKeys.properties

For example, VK_SHIFT=Shift is defined in each platformKeys.properties file because someone figured MAC should have a different label for shift key: VK_SHIFT=\u21e7 (upwards white arrow, ⇧); on all other platforms, it's just plain vanilla "Shift".

And there's a comment in each of the platformKeys.properties files describing the purpose:
> #this file defines the on screen display names for the various modifier keys
> #these are used in XP menus to show keyboard shortcuts
Isn't that exactly what we want in this bug?

The fact that so far it's only modifier keys is rather accidental without any technical consequence (e.g. we also have MODIFIER_SEPARATOR=+ in there, which is not a modifier key, but just another related label in this context).  It's just that the label problem of VK_RETURN had not yet been realized at the time when the platformKeys.properties files were created. Note that the file name is not "platformModifierKeys.properties" but just platformKeys.properties.

Iow, all keys with platform-specific labels are defined in platformKeys.properties.
Per this bug, VK_RETURN is a key with platform-specific label, because on MAC, the label seems to be "Return", but on Win/Linux, the same key should be correctly labelled "Enter".

So why should VK_RETURN not be moved out the respective platformKeys.properties of Win/Linux/MAC (sorry I forgot MAC in comment 41)?

I assume (pls correct me) that this would not even change a thing for frontend consumers, because both the global/keys.properties and global-platform/#aPlatform#/platformKeys.properties will be loaded concurrently, so from an application point of view, it doesn't matter at all from which file the label originates, application code will just use VK_RETURN as before and get the corrected, now platform-specific label in return (so we don't have to change code in all the consumers when we move the label property into the other file).

I'm really failing to see the problem (without claiming I know all about these things, just trying to finid out where this is stuck). What I see is that :aceman has basically made the same suggestion as me, and did not get any satisfactory replies, and since then nothing happening - another sad instance of stalled progress in TB.

Comment 44

[Thomas D. (:thomas8)]

If the hesitation is about labeling VK_RETURN=Enter, which is a bit weird given that we also have VK_ENTER=Enter, well, that weirdness just reflects reality on the ground, where what used to be "Return" key on typewriters has morphed over time to become "Enter" key on computers, and MAC is the only exception still using the old label (and, for added confusion, MAC keyboards often have both "Enter" and "Return" written on that key, with Return being the main label). So we'll keep VK_RETURN under the hood (since that's the official DOM name of the Enter key on main part of keyboards, and we can't change that), but we need to correct the labels to make them catch up with reality.

Comment 45

[Thomas D. (:thomas8)]

^^ comment 41 to comment 44

Comment 46

[Magnus Melin [:mkmelin]]

I think I confused this with another bug. The suggested approach sounds like it might work.

Comment 47

[Thomas D. (:thomas8)]

I'll try this.

As explained above, this bug is about VK_RETURN.
However, we also need to move VK_ENTER along into platformKeys.properties so as to keep the two together (to avoid more confusion). Normally, each key has a localization comment describing which key is meant.

However, I'm not sure about which key is VK_ENTER, looking at the documentation[1], Bug 105990 etc...
> DOM_VK_ENTER 	0x0E (14) 	Reserved, but not used.

[1] https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent?redirectlocale=en-US&redirectslug=DOM%2FKeyboardEvent#Constants

So is VK_ENTER the numpad Enter key?
And on MAC, is VK_ENTER the "Enter" key which on some MAC keyboards, you get with fn+return?

Comment 48

[Thomas D. (:thomas8)]

^^

Comment 49

[Masatoshi Kimura [:emk]]

Nakano-san, do you know about VK_RETURN vs. VK_ENTER, PC vs. Mac, and main key vs. numpad madness?

Comment 50

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

VK_ENTER must not be used forever because mapping different keyCode values to Enter key on standard position and numpad makes web apps broken. For example, if numpad's Enter key is mapped to VK_ENTER, it cannot be handled by event handlers which only handle VK_RETURN even though they should work as same key. If web apps need to know if the pressed key's position, they can/should use KeyboardEvent.location. So, DOM Events spec is designed as that same function keys must be same information except .location. This really does make sense for making key event handlers simple.

So, I believe that this bug's right solution is, key label in UI should be defined in somewhere for each platforms. I guess that Backspace and Delete key also have same issue.

I'm not sure whether "Return" or "Enter" is expected label for Mac users, though. We should look for the UI in some applications developed by Apple and use same label.

Comment 51

[Thomas D. (:thomas8)]

Attachment: patch v1: make label for VK_RETURN platform-specific, "Enter" for Win/Unix

So this patch makes the label for VK_RETURN platform-specific:
"Enter" for Win/Unix, and
"Return" for MAC.

Aceman, could you please complete the HG patch header for me (Date, Node ID, Parent)? Tia.

Comment 52

[Thomas D. (:thomas8)]

Comment on attachment 8350295 [details] [diff] [review]
patch v1: make label for VK_RETURN platform-specific, "Enter" for Win/Unix

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #50)
> So, I believe that this bug's right solution is, key label in UI should be
> defined in somewhere for each platforms.

That's what this patch does (as generally agreed), see Comment 51.

Pls somebody request any other reviews that might be required to land this.
Couldn't find "Locales" component on Toolkit, pls verify if "General" is the correct component.

Comment 53

[:aceman]

Interesting. Does this work by itself? If the key names are not found in keys.properties, are they looked up in platformKeys.properties ?

Comment 54

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #53)
> Interesting. Does this work by itself? If the key names are not found in
> keys.properties, are they looked up in platformKeys.properties ?

That's what I assume, but I'm not sure, and I can't test.

They seem to load into separate variables:
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/ShortcutUtils.jsm#13
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/ShortcutUtils.jsm#18

This looks like it could remap from platformKeys to keys, but again, not sure:
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/ShortcutUtils.jsm#88

Comment 55

[Benjamin Smedberg]

Comment on attachment 8350295 [details] [diff] [review]
patch v1: make label for VK_RETURN platform-specific, "Enter" for Win/Unix

I really don't know this at all, so I'm going to bounce this to somebody who might according to hg logs.

Comment 56

[:Gijs (he/him)]

Comment on attachment 8350295 [details] [diff] [review]
patch v1: make label for VK_RETURN platform-specific, "Enter" for Win/Unix

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

So while this adds the strings in platform-specific places, you need a code change as well, because right now stuff just won't work. You'd need to change all the consumers (yes, there are several) to look in the right places. See:

http://mxr.mozilla.org/mozilla-central/search?string=keys.properties

http://mxr.mozilla.org/mozilla-central/search?string=platformkeys.properties

::: toolkit/locales/en-US/chrome/global-platform/mac/platformKeys.properties
@@ +2,5 @@
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  #mac
> +#this file defines the on screen display names for the various modifier keys and enter/return keys

Nit: please wrap this comment (and the other ones in like files) at 80 characters

::: toolkit/locales/en-US/chrome/global-platform/unix/platformKeys.properties
@@ +20,5 @@
>  
>  #the control key
>  VK_CONTROL=Ctrl
> +
> +#the return/enter key on the main keyboard ("Enter" on Windows/Unix, "Return" on MAC)

Nit: don't all-caps "Mac". Same in windows version of this file.

Comment 57

[Thomas D. (:thomas8)]

(In reply to :Gijs Kruitbosch (Unavailable Dec 19 - Jan 2) from comment #56)
> Comment on attachment 8350295 [details] [diff] [review]
> patch v1: make label for VK_RETURN platform-specific, "Enter" for Win/Unix
> Review of attachment 8350295 [details] [diff] [review]:
> -----------------------------------------------------------------
> So while this adds the strings in platform-specific places, you need a code
> change as well, because right now stuff just won't work. You'd need to
> change all the consumers (yes, there are several) to look in the right
> places. See:
> 
> http://mxr.mozilla.org/mozilla-central/search?string=keys.properties

Well, then unfortunately I'm out of the game, mostly because I don't know C++, but also because this is nested in very confusing ways with several layers in the internals. Well, it was worth a try, and at least we've clarified direction and there's a starting point in code now from my draft patch.

How hard is this for someone who knows C++ and more of the internals?
Can we CC someone who is able to fix this? Perhaps after 12 years, even a trivial bug has a right to be fixed. And to see "Return" instead of "Enter" in shortcut keys on Windows is pretty confusing because windows users don't know anything about a "Return" key.

As a sidenote, I might be wrong, but I strongly suspect that this should have been designed differently. To begin with, both cross-platform keys and platform keys should at some stage be bundled together. Either move all cross-platform keys into the platform files, or read both files into a single bundle somehow. There's no structural difference between VK_RETURN and, say, VK_SHIFT, so it's weird that apparently they are called with two entirely different procedures, and moving them requires recoding all the consumers.

Comment 59

[Thomas D. (:thomas8)]

(In reply to :Gijs Kruitbosch from comment #56)
> Comment on attachment 8350295 [details] [diff] [review]
> patch v1: make label for VK_RETURN platform-specific, "Enter" for Win/Unix
> 
> Review of attachment 8350295 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So while this adds the strings in platform-specific places, you need a code
> change as well, because right now stuff just won't work. You'd need to
> change all the consumers (yes, there are several) to look in the right
> places. See:
> 
> http://mxr.mozilla.org/mozilla-central/search?string=keys.properties
> 
> http://mxr.mozilla.org/mozilla-central/search?string=platformkeys.properties

:Gijs, thank you for your review. Unfortunately, I'm failing to understand which consumers need changing per your review. Can you please provide one specific example demonstrating the change you have in mind?

This is what I understand:

My patch is moving vk_return and vk_enter from 
a/toolkit/locales/en-US/chrome/global/keys.properties
to
b/toolkit/locales/en-US/chrome/global-platform/mac/platformKeys.properties etc.

So I think the only consumers that can be affected are the consumers of keys.properties (from which we remove the keys), right?
That limits the number of consumers in moz-central to just 3:

http://mxr.mozilla.org/mozilla-central/search?string=/keys.properties

The result files are:

/layout/xul/nsMenuFrame.cpp (View Hg log or Hg annotations)
    line 1074 -- rv = bundleService->CreateBundle("chrome://global/locale/keys.properties",

/toolkit/locales/jar.mn (View Hg log or Hg annotations)
    line 51 -- locale/@AB_CD@/global/keys.properties (%chrome/global/keys.properties)

/toolkit/modules/ShortcutUtils.jsm (View Hg log or Hg annotations)
    line 20 -- "chrome://global/locale/keys.properties");

So the first question is, do these files load the platformKeys.properties file, too (into which we moved vk_return and vk_enter).

Both jar.mn and ShortcutUtils.jsm already reference platformKeys.properties file, so the platform files are already accessible.

Next question, do we have any consumers in the code which load string bundles from just the global keys.properties file into a variable/object and then use vk_enter or vk_return from the wrong set (because it's now in the other string bundle retrieved from platformKeys.properties file).

That's where I'm not very sure how to search for that; I tried this:

http://mxr.mozilla.org/mozilla-central/search?string=keys.getstringfromname

I see nothing relevant there, and the only candidate, http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/ShortcutUtils.jsm#88, works the other way round (find keys not found in platform in the global file).

So afasics, the only place where we might have to change something is this:

/layout/xul/nsMenuFrame.cpp (View Hg log or Hg annotations)
    line 1074 -- rv = bundleService->CreateBundle("chrome://global/locale/keys.properties",

Unfortunately, I wouldn't know if we need to change that, or how, or what. Can you assist?

Even brute force search for vk_enter or vk_return, I don't see any consumers which we'd have to change (but I'm not sure):

http://mxr.mozilla.org/mozilla-central/search?string=vk_enter
http://mxr.mozilla.org/mozilla-central/search?string=vk_return

Do we need to check any other repository roots like comm-central?

Comment 60

[Thomas D. (:thomas8)]

^^ cc neil

Comment 61

[[e-mail]]

The problem here is that both the places you mentioned assume that only modifiers are platform-specific, and so they get retrieved from platformKeys.properties while the regular keys get retrieved from keys.properties.

Unfortunately there isn't a straightforward way to override properties in the same way that you can override DTD entities (and even that isn't straightforward due to our l10n story) so someone would just have to write the appropriate code in both places.

Comment 62

[Thomas D. (:thomas8)]

(In reply to [e-mail] from comment #61)
> The problem here is that both the places you mentioned assume that only
> modifiers are platform-specific, and so they get retrieved from
> platformKeys.properties while the regular keys get retrieved from
> keys.properties.

Could you pls spell out which are the "two places that I mentioned"?
Could you provide any specific line of code where consumption of vk_return or vk_enter will fail after the change proposed by my patch attachment 8350295 [details] [diff] [review]?

> Unfortunately there isn't a straightforward way to override properties in
> the same way that you can override DTD entities (and even that isn't
> straightforward due to our l10n story) so someone would just have to write
> the appropriate code in both places.

Can anyone spell out any example or direction for such "appropriate code" so that it becomes actionable? Tia

Comment 63

[[e-mail]]

(In reply to Thomas D. from comment #62)
> Could you pls spell out which are the "two places that I mentioned"?
> Could you provide any specific line of code where consumption of vk_return
> or vk_enter will fail after the change proposed by my patch

(In reply to Thomas D. from comment #59)
> /layout/xul/nsMenuFrame.cpp (View Hg log or Hg annotations)
>     line 1074 -- rv = bundleService->CreateBundle("chrome://global/locale/keys.properties",
> 
> /toolkit/locales/jar.mn (View Hg log or Hg annotations)
>     line 51 -- locale/@AB_CD@/global/keys.properties (%chrome/global/keys.properties)
> 
> /toolkit/modules/ShortcutUtils.jsm (View Hg log or Hg annotations)
>     line 20 -- "chrome://global/locale/keys.properties");

As you mentioned, jar.mn contains both, so there's no problem there.
ShortcutUtils.jsm contains both, but it only uses platformKeys for modifiers.
nsMenuFrame.cpp doesn't use platformKeys at all at the moment.

Comment 64

[Thomas D. (:thomas8)]

(In reply to [e-mail] from comment #63)
> As you mentioned, jar.mn contains both, so there's no problem there.
> ShortcutUtils.jsm contains both, but it only uses platformKeys for modifiers.
> nsMenuFrame.cpp doesn't use platformKeys at all at the moment.

Thanks Neil. So for shortcututils.jsm, would something along these lines work to make prettifyShortcut function aware of Return and Enter now found in platformKeys?


83     let key;
84     let keyCode = aElemKey.getAttribute("keycode");
85     if (keyCode) {

         if (keyCode=="VK_RETURN" || keyCode=="VK_ENTER") { // these special creatures live in platformKeys, so get them from there

           try {
             // Some keys might not exist in the locale file, which will throw:
             key = PlatformKeys.GetStringFromName(keyCode.toUpperCase());       // here's where we get our special friends from PlatformKeys
           } catch (ex) {
             Cu.reportError("Error finding " + keyCode + ": " + ex);
             key = keyCode.replace(/^VK_/, '');
           }


         } else { // try getting keyCode from global Keys

86         try {
87           // Some keys might not exist in the locale file, which will throw:
88           key = Keys.GetStringFromName(keyCode.toUpperCase());
89         } catch (ex) {
90           Cu.reportError("Error finding " + keyCode + ": " + ex);
91           key = keyCode.replace(/^VK_/, '');
92         }

         } // end if for special creatures

93     } else {
94       key = aElemKey.getAttribute("key");
95       key = key.toUpperCase();
96     }
97     return elemString + key;

Comment 65

[[e-mail]]

(In reply to Thomas D. from comment #64)
> So for shortcututils.jsm, would something along these lines
> work to make prettifyShortcut function aware of Return and Enter now found
> in platformKeys?

I'm unfamiliar with SpecialKey.jsm but I guess that would work, although you could simplify it slightly:

if (keyCode) {
  try {
    // Some keys might not exist in the locale file, which will throw:
    key = PlatformKeys.GetStringFromName(keyCode.toUpperCase());   
  } catch (ex) { // try getting keyCode from global Keys
    try {
      key = Keys.GetStringFromName(keyCode.toUpperCase());
    } catch (ex) {
      Cu.reportError("Error finding " + keyCode + ": " + ex);
      key = keyCode.replace(/^VK_/, '');
    }
  }

[Side note: VK_ENTER no longer exists.]

Comment 66

[Thomas D. (:thomas8)]

Updated query for m-c consumers of keys.properties file which has the offending entry:
> VK_RETURN=Return

https://dxr.mozilla.org/mozilla-central/search?q=keys.properties

(In reply to [e-mail] from comment #63)
> (In reply to Thomas D. from comment #59)

https://dxr.mozilla.org/mozilla-central/source/layout/xul/nsMenuFrame.cpp
line 1108: rv = bundleService->CreateBundle("chrome://global/locale/keys.properties",

https://dxr.mozilla.org/mozilla-central/source/toolkit/locales/jar.mn
line 63: locale/@AB_CD@/global/keys.properties                 (%chrome/global/keys.properties)

https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/ShortcutUtils.jsm (View Hg log or Hg annotations)
line 20: "chrome://global/locale/keys.properties");

> As you mentioned, jar.mn contains both, so there's no problem there.
--> nothing to do.
> ShortcutUtils.jsm contains both, but it only uses platformKeys for modifiers.
--> needs fix.
> nsMenuFrame.cpp doesn't use platformKeys at all at the moment.
--> needs fix.

Comment 67

[Thomas D. (:thomas8)]

Sorry for bugspam, but let's have better links for everyone's convenience...

(In reply to Thomas D. (currently busy elsewhere) from comment #66)
Updated query for m-c consumers of keys.properties file which has the
offending entry:
> VK_RETURN=Return

https://dxr.mozilla.org/mozilla-central/search?q=keys.properties

(In reply to [e-mail] from comment #63)
> (In reply to Thomas D. from comment #59)

https://dxr.mozilla.org/mozilla-central/source/layout/xul/nsMenuFrame.cpp#1108
line 1108: rv = bundleService->CreateBundle("chrome://global/locale/keys.properties",

https://dxr.mozilla.org/mozilla-central/source/toolkit/locales/jar.mn#63
line 63: locale/@AB_CD@/global/keys.properties                 (%chrome/global/keys.properties)

https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/ShortcutUtils.jsm#20
line 20: "chrome://global/locale/keys.properties");

> As you mentioned, jar.mn contains both, so there's no problem there.
--> nothing to do.
> ShortcutUtils.jsm contains both, but it only uses platformKeys for modifiers.
--> needs fix.
> nsMenuFrame.cpp doesn't use platformKeys at all at the moment.
--> needs fix.

Comment 68

[Thomas D. (:thomas8)]

Attachment: Patch V.2: Make label for VK_RETURN keycode platform-specific, "Enter" for Win/Unix (also update m-c consumers)

Let's get this out of the way. 16 years after being filed, this is still wrong, ugly, and confusing to end users.

Gijs, here's an updated patch according to our old discussion which also fixes the two (out of 3) consumers where this makes a difference.

We're removing a string from cross-OS global keys.properties, so consumers of platformKeys.properties don't matter. See my comment 67 for the dxr query and current list of consumers, and why they are relevant or not.

From comment 65, I used Neil's streamlined version of my new code in shortcututils.jsm.

Comment 69

[:Gijs (he/him)]

Comment on attachment 8939856 [details] [diff] [review]
Patch V.2: Make label for VK_RETURN keycode platform-specific, "Enter" for Win/Unix (also update m-c consumers)

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

r=me on the JS + properties changes with the change below for the ShortcutUtils.jsm file.

I think someone else who knows layout/xul should review that change, so I've requested an additional review from Neil for that.

::: toolkit/modules/ShortcutUtils.jsm
@@ +94,2 @@
>          key = Keys.GetStringFromName(keyCode.toUpperCase());
> +        key = PlatformKeys.GetStringFromName(keyCode.toUpperCase());

This added line is incorrect. If the first call succeeds, we shouldn't try PlatformKeys and overwrite the result. If it doesn't, we will never hit this line. 

Here is what I consider a neater approach:

...
if (keyCode) {
  keyCode = keyCode.toUpperCase();
  try {
    let bundle = keyCode == "VK_RETURN" ? PlatformKeys : Keys;
    // Some keys might not exist in the locale file, which will throw.
    bundle.GetStringFromName(keyCode);
  } catch (ex) {
    Cu.reportError("Error finding " + keyCode + ": " + ex);
    key = keyCode.replace(/^VK_/, "");
  }
}
...

This avoids the nested try...catch, the repeated uppercasing of the keycode, and repetitive calls to GetStringFromName, and is fewer lines of code overall.

Comment 70

[Neil Deakin]

I would probably use a similar approach as described in comment 69 for nsMenuFrame.cpp as well -- that is compare the string to VK_RETURN and then select the right stringbundle rather than checking both.

Comment 71

[Thomas D. (:thomas8)]

Attachment: Patch V.3: Make label for VK_RETURN keycode platform-specific, "Enter" for Win/Unix (streamlined code)

(In reply to :Gijs from comment #69)
> Comment on attachment 8939856 [details] [diff] [review]

Thanks for rapid review! :)
Gijs, as I also changed nsMenuFrame.cpp, I'd appreciate another review.

> I think someone else who knows layout/xul should review that change, so I've
> requested an additional review from Neil for that.

Great, he had that idea of improving nsMenuFrame.cpp in the same way as you suggested for ShortcutUtils.jsm.

> ::: toolkit/modules/ShortcutUtils.jsm
> @@ +94,2 @@
> >          key = Keys.GetStringFromName(keyCode.toUpperCase());
> > +        key = PlatformKeys.GetStringFromName(keyCode.toUpperCase());
> 
> This added line is incorrect.

Obviously incorrect! That was unintentional, I forgot to delete this line.

> Here is what I consider a neater approach:
>     let bundle = keyCode == "VK_RETURN" ? PlatformKeys : Keys;

Nifty! Thanks. Done.

>     // Some keys might not exist in the locale file, which will throw.
>     bundle.GetStringFromName(keyCode);

Nit: |key = | was accidentally omitted here, which would have made this fail.

(In reply to Neil Deakin from comment #70)
> I would probably use a similar approach as described in comment 69 for
> nsMenuFrame.cpp as well -- that is compare the string to VK_RETURN and then
> select the right stringbundle rather than checking both.

Nifty again! Thanks! Done.

Comment 72

[Jorg K (CEST = GMT+2)]

Comment on attachment 8939923 [details] [diff] [review]
Patch V.3: Make label for VK_RETURN keycode platform-specific, "Enter" for Win/Unix (streamlined code)

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

Drive-by comment ;-)

::: layout/xul/nsMenuFrame.cpp
@@ +1105,5 @@
>          mozilla::services::GetStringBundleService();
>        if (bundleService) {
>          nsCOMPtr<nsIStringBundle> bundle;
> +        rv = bundleService->CreateBundle(
> +               ((keyCode == "VK_RETURN") ?

Nit: Parenthesis around the condition are not required.

Comment 73

[Thomas D. (:thomas8)]

Attachment: Patch V.3.1: Make label for VK_RETURN keycode platform-specific, "Enter" for Win/Unix (nitfix parentheses)

(In reply to Jorg K (GMT+1) from comment #72)
> Comment on attachment 8939923 [details] [diff] [review]
> Drive-by comment ;-)

Welcome :)
 
> > +               ((keyCode == "VK_RETURN") ?
> 
> Nit: Parenthesis around the condition are not required.

That's the last nit I was looking for! Thank you, Mr. Perfect! :)

Comment 74

[Thomas D. (:thomas8)]

Comment on attachment 8939942 [details] [diff] [review]
Patch V.3.1: Make label for VK_RETURN keycode platform-specific, "Enter" for Win/Unix (nitfix  parentheses)

Carry over Neil Deakin's r+ before comment 73.

Comment 75

[:Gijs (he/him)]

Comment on attachment 8939942 [details] [diff] [review]
Patch V.3.1: Make label for VK_RETURN keycode platform-specific, "Enter" for Win/Unix (nitfix  parentheses)

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

LGTM, thanks!

Comment 76

[:Gijs (he/him)]

Comment on attachment 8939942 [details] [diff] [review]
Patch V.3.1: Make label for VK_RETURN keycode platform-specific, "Enter" for Win/Unix (nitfix  parentheses)

Err, oops, didn't mean to clear the extra r+

Comment 77

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a55b10a6974
Change shortcut label of VK_RETURN to "Enter" on Win/Unix. r=enndeakin, r=Gijs

Comment 78

[Andreea Pavel [:apavel]]

Backed out for build bustage on /builds/worker/workspace/build/src/layout/xul/nsMenuFrame.cpp r=backout a=backout on a CLOSED TREE

Push with bustages: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=3a55b10a6974a04d3af16535cd97027d8fccc4a7

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=154275202&repo=mozilla-inbound&lineNumber=20439

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/d760cf06ca40056077e2a354b8b69350a5765ae3

Comment 79

[Jorg K (CEST = GMT+2)]

I guess no one compiled this to notice that in C++ keyCode == "VK_RETURN" doesn't work :-(
+        rv = bundleService->CreateBundle(
+               (keyCode == "VK_RETURN" ?
+                 "chrome://global-platform/locale/platformKeys.properties"
+               : "chrome://global/locale/keys.properties"),
+               getter_AddRefs(bundle));

Should really be
+        rv = bundleService->CreateBundle(
+               keyCode.Equals("VK_RETURN")
+                 ? "chrome://global-platform/locale/platformKeys.properties"
+                 : "chrome://global/locale/keys.properties",
+               getter_AddRefs(bundle));

I also dropped some more unnecessary parenthesis and formatted it to be more legible.

Comment 80

[Thomas D. (:thomas8)]

Attachment: Patch V.3.2: Make label for VK_RETURN keycode platform-specific, "Enter" for Win/Unix (fix comparison)

(In reply to Jorg K (GMT+1) from comment #79)
> +               (keyCode == "VK_RETURN" ?
> Should really be
> +               keyCode.Equals("VK_RETURN")

Thanks a lot, Jörg. Unfortunately I'm not into C++, and I'm not compiling.

So here's new patch with Jörg's fix and improvements.
Essentially unchanged except that one line, so existing r+ are still applicable.

Comment 81

[Jorg K (CEST = GMT+2)]

Still doesn't compile, let me fix it.

Comment 82

[Jorg K (CEST = GMT+2)]

Attachment: Patch V.3.2: Make label for VK_RETURN keycode platform-specific, "Enter" for Win/Unix (fix comparison, take 2)

Gijs, sorry to trouble you again. Please give this another thorough look to avoid another backout.

Comment 83

[:Gijs (he/him)]

Comment on attachment 8940148 [details] [diff] [review]
Patch V.3.2: Make label for VK_RETURN keycode platform-specific, "Enter" for Win/Unix (fix comparison, take 2)

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

Gah, sorry for not catching the EqualsLiteral stuff in review yesterday. This compiles on my mac, and LGTM.

Comment 84

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/86275396a96b
Change shortcut label of VK_RETURN to "Enter" on Win/Unix. r=enndeakin, r=Gijs

Comment 85

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #81)
> Still doesn't compile, let me fix it.

Thanks! :)

Comment 86

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/86275396a96b

Comment 87

[Thomas D. (:thomas8)]

Thanks everyone! Finally... :)

Comment 88

[Andrew Thompson]

Lovely to see an old UX bug finally bite the dust.

Comment 89

[:aceman]

Thanks Thomas for being so persistent and finally driving this to a solution.
The display of a non-existent on Win/Linux was quite embarrassing.