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)

RESOLVED FIXED in Firefox 59

Status

()

--
trivial
RESOLVED FIXED
18 years ago
a year ago

People

(Reporter: dean_tessman, Assigned: bugzilla2007)

Tracking

({ux-implementation-level})

Trunk
mozilla59
ux-implementation-level
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

18 years ago
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.
-->varada
Assignee: ducarroz → varada

Updated

18 years ago
Keywords: nsbeta1
(Reporter)

Comment 2

18 years ago
Changing VK_RETURN to VK_ENTER in messengercompose.dtd should do it.

Updated

18 years ago
Keywords: nsbeta1 → nsbeta1+

Comment 3

18 years ago
changing VK_RETURN to VK_ENTER changed shortcut text in 'File' menu, anyway the 
shortcut is not working.
(Reporter)

Comment 4

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

Comment 5

18 years ago
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

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

Updated

18 years ago
Priority: -- → P3

Updated

17 years ago
Status: NEW → ASSIGNED
Priority: P3 → P2

Updated

17 years ago
Blocks: 122274

Updated

17 years ago
Target Milestone: --- → mozilla1.0

Comment 7

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

Comment 8

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

Comment 9

17 years ago
Discussed in Mail News bug meeting with QA, PjM, and Engineering.  Decision was
to minus this bug and move to 1.2.
Keywords: nsbeta1+ → nsbeta1-
Target Milestone: mozilla1.0 → mozilla1.2

Updated

17 years ago
Keywords: helpwanted

Comment 10

17 years ago
nominating for the next release
Keywords: nsbeta1- → nsbeta1
taking all of varada's bugs.
Assignee: varada → sspitzer
Status: ASSIGNED → NEW

Updated

17 years ago
QA Contact: sheelar → esther

Comment 12

16 years ago
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

16 years ago
Mail triage team: nsbeta1+/adt3
Keywords: nsbeta1 → nsbeta1+
Whiteboard: [adt3]
Target Milestone: mozilla1.2alpha → mozilla1.4beta

Comment 14

15 years ago
Posted patch patch 1 (obsolete) — Splinter Review
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?

Updated

15 years ago
Attachment #149320 - Flags: superreview?(bienvenu)
Attachment #149320 - Flags: review?(neil.parkwaycc.co.uk)
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.
Attachment #149320 - Flags: review?(neil.parkwaycc.co.uk) → review-
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

15 years ago
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.

Updated

15 years ago
Attachment #149320 - Flags: superreview?(bienvenu)
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?
(Reporter)

Comment 19

15 years ago
That's always confused me, too.
(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

15 years ago
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?
(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

15 years ago
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.
Product: MailNews → Core

Comment 24

14 years ago
Posted patch Better Patch (obsolete) — Splinter Review
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
Attachment #184295 - Flags: review+
Attachment #184295 - Flags: approval-l10n+
sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.
Assignee: sspitzer → nobody
patch attachment 184295 [details] [diff] [review] never checked in after approvals.
Keywords: helpwanted → checkin-needed
OS: Windows 2000 → All
QA Contact: esther → composition
Hardware: PC → All
Whiteboard: [adt3]
Um, no. A two year old, self-reviewed, self-approved-l10n patch does not constitute checkin-needed.
Keywords: checkin-needed
OK. clearing approvals
Severity: normal → trivial
Keywords: helpwanted

Updated

11 years ago
Attachment #184295 - Flags: review+
Product: Core → MailNews Core
Keywords: helpwanted
Priority: P2 → --
Summary: Ctrl+Return should be Ctrl+Enter → Ctrl+Return should be Ctrl+Enter on windows and linux
Target Milestone: mozilla1.4beta → ---
Posted patch proposed fix (obsolete) — Splinter Review
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...)
Assignee: nobody → mkmelin+mozilla
Attachment #184295 - Attachment is obsolete: true
Attachment #149320 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #344521 - Flags: superreview?(neil)
Attachment #344521 - Flags: review?(philringnalda)
Hm, I wonder if i should have had to bump the entity names for the keycode entities here...

Updated

11 years ago
Attachment #344521 - Flags: superreview?(neil) → superreview-
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 on attachment 344521 [details] [diff] [review]
proposed fix

Ah yes...
Attachment #344521 - Attachment is obsolete: true
Attachment #344521 - Flags: review?(philringnalda)
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.
Assignee: mkmelin+mozilla → nobody
assignee=nobody, so >status=new
Status: ASSIGNED → NEW

Comment 35

7 years ago
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.)
Flags: needinfo?(neil)
(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.
Flags: needinfo?(neil)

Comment 37

7 years ago
(In reply to neil@parkwaycc.co.uk 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.
Flags: needinfo?(neil)
(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.
Flags: needinfo?(neil)

Comment 39

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

Updated

7 years ago
Flags: needinfo?(neil)
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?
Flags: needinfo?(neil)
(Assignee)

Comment 41

5 years ago
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
Flags: needinfo?
Keywords: ux-implementation-level
Summary: Ctrl+Return should be Ctrl+Enter on windows and linux → 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)
Whiteboard: [good first bug]
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.
Flags: needinfo?
(Assignee)

Comment 43

5 years ago
(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.
(Assignee)

Comment 44

5 years ago
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.
(Assignee)

Comment 45

5 years ago
^^ comment 41 to comment 44
Flags: needinfo?
I think I confused this with another bug. The suggested approach sounds like it might work.
Flags: needinfo?
Whiteboard: [good first bug]
(Assignee)

Comment 47

5 years ago
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?
Assignee: nobody → bugzilla2007
(Assignee)

Comment 48

5 years ago
^^
Flags: needinfo?

Updated

5 years ago
Flags: needinfo?
Nakano-san, do you know about VK_RETURN vs. VK_ENTER, PC vs. Mac, and main key vs. numpad madness?
Flags: needinfo?(masayuki)
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.
Flags: needinfo?(masayuki)
(Assignee)

Comment 51

5 years ago
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.
Attachment #8350295 - Flags: review?
Flags: needinfo?(acelists)
(Assignee)

Updated

5 years ago
Component: Composition → General
Product: MailNews Core → Toolkit
(Assignee)

Comment 52

5 years ago
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.
Attachment #8350295 - Flags: review? → review?(benjamin)

Comment 53

5 years ago
Interesting. Does this work by itself? If the key names are not found in keys.properties, are they looked up in platformKeys.properties ?
(Assignee)

Comment 54

5 years ago
(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 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.
Attachment #8350295 - Flags: review?(benjamin) → review?(gijskruitbosch+bugs)

Comment 56

5 years ago
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.
Attachment #8350295 - Flags: review?(gijskruitbosch+bugs) → review-
(Assignee)

Comment 57

5 years ago
(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.
Assignee: bugzilla2007 → nobody
Flags: needinfo?(acelists)
(Assignee)

Updated

5 years ago
Whiteboard: [has wip patch][patchlove]
Duplicate of this bug: 864197
(Assignee)

Comment 59

5 years ago
(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?
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 60

5 years ago
^^ cc neil
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.
(Assignee)

Comment 62

5 years ago
(In reply to neil@parkwaycc.co.uk 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
(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.

Updated

5 years ago
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 64

5 years ago
(In reply to neil@parkwaycc.co.uk 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;
Flags: needinfo?(neil)
(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.]
Flags: needinfo?(neil)
(Assignee)

Updated

5 years ago
Depends on: 969247, 105990
(Assignee)

Comment 66

a year ago
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 neil@parkwaycc.co.uk 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.
(Assignee)

Comment 67

a year ago
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 neil@parkwaycc.co.uk 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.
(Assignee)

Comment 68

a year ago
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.
Attachment #8350295 - Attachment is obsolete: true
Attachment #8939856 - Flags: review?(gijskruitbosch+bugs)

Comment 69

a year ago
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.
Attachment #8939856 - Flags: review?(gijskruitbosch+bugs)
Attachment #8939856 - Flags: review?(enndeakin)
Attachment #8939856 - Flags: review+

Comment 70

a year ago
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.
(Assignee)

Comment 71

a year ago
(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.
Assignee: nobody → bugzilla2007
Attachment #8939856 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8939856 - Flags: review?(enndeakin)
Attachment #8939923 - Flags: review?(gijskruitbosch+bugs)
Attachment #8939923 - Flags: review?(enndeakin)

Comment 72

a year ago
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.

Updated

a year ago
Attachment #8939923 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 73

a year ago
(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! :)
Attachment #8939923 - Attachment is obsolete: true
Attachment #8939923 - Flags: review?(gijskruitbosch+bugs)
Attachment #8939942 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 74

a year ago
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.
Attachment #8939942 - Flags: review+

Comment 75

a year ago
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!
Attachment #8939942 - Flags: review?(gijskruitbosch+bugs)
Attachment #8939942 - Flags: review+

Updated

a year ago
Keywords: checkin-needed

Comment 76

a year ago
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+
Attachment #8939942 - Flags: review+

Comment 77

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a55b10a6974
Change shortcut label of VK_RETURN to "Enter" on Win/Unix. r=enndeakin, r=Gijs
Keywords: checkin-needed

Comment 79

a year ago
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.
(Assignee)

Comment 80

a year ago
(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.
Attachment #8939942 - Attachment is obsolete: true
Flags: needinfo?(bugzilla2007)
Attachment #8940101 - Flags: review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed
Whiteboard: [has wip patch][patchlove]

Comment 81

a year ago
Still doesn't compile, let me fix it.
Keywords: checkin-needed

Comment 82

a year ago
Gijs, sorry to trouble you again. Please give this another thorough look to avoid another backout.
Attachment #8940101 - Attachment is obsolete: true
Attachment #8940148 - Flags: review?(gijskruitbosch+bugs)

Comment 83

a year ago
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.
Attachment #8940148 - Flags: review?(gijskruitbosch+bugs) → review+

Updated

a year ago
Keywords: checkin-needed

Comment 84

a year ago
Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/86275396a96b
Change shortcut label of VK_RETURN to "Enter" on Win/Unix. r=enndeakin, r=Gijs
Keywords: checkin-needed
(Assignee)

Comment 85

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

Thanks! :)
https://hg.mozilla.org/mozilla-central/rev/86275396a96b
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
(Assignee)

Comment 87

a year ago
Thanks everyone! Finally... :)
Lovely to see an old UX bug finally bite the dust.

Comment 89

a year ago
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.

Updated

a year ago
Duplicate of this bug: 928291
You need to log in before you can comment on or make changes to this bug.