Summarize the issue, if anything incorrect, please help to correct it. Thanks.
Uefi spec ambiguous description:
> When interpreting the data from this function, it should be noted that if a class
> of printable characters that are normally adjusted by shift modifiers (e.g. Shift
> Key + "f" key) would be presented solely as a KeyData.Key.UnicodeChar without the
> associated shift state. So in the previous example of a Shift Key + "f" key being
> pressed, the only pertinent data returned would be KeyData.Key.UnicodeChar with
> the value of "F". This of course would not typically be the case for non-printable
> characters such as the pressing of the Right Shift Key + F10 key since the
> corresponding returned data would be reflected both in the
> KeyData.KeyState.KeyShiftState and KeyData.Key.ScanCode values.
Some firmware vendor see the “if” as an option to implement the keyboard driver and they choose to implement in the other way.
Ideal: shift + UnicodeKey/UnScancodeKey == report as ==> “shifted” UnicodeKey/UnScancodeKey
Ctrl + UnicodeKey == report as ==> UnicodeKey – ‘A’/’a’ + 1
Other way: shift + UnicodeKey/UnScancodeKey == report as ==> shift state + “shifted” UnicodeKey/UnScancodeKey
Ctrl + UnicodeKey == report as ==> ctrl state + UnicodeKey – ‘A’/’a’ + 1
I.e. do not remove the shift state.
That would cause the shell edit/Hexedit missing handling the “shifted” key when run shell in such firmware.
Vitaly’s solution add the support the handle “other implementation”.
If we take Vitaly’s solution, we should fixed all the applications in edk2 not only in shell edit/Hexeidt function.
And when we catch another request same with this issue, we would follow the same progress.
Can you help to review this issue? Because of the uefi spec issue, some firmware implemented the different behavior.
Should the edk2 code to handle such issue? If yes, here maybe the situation:
1. Change
for all apps -> uefi spec update and accept such behavior with description -> Done.
2. Change
for all apps -> uefi spec update to remove the ambiguous and reject other behavior -> removal the change in first step.
I used to think it is an additional support for different implementation because of the spec. But if we approve this patch, all the app in edk2 using the combo key function should be update.
Using shell’s ctrl+’c’ as an example, it need to update at the same time. Same with SCT tool.
Requesting to merge this into edk2-stable202005 for the reasons explained in BZ[1]. I assume there is no real objection for this, only the approach we make such changes
in the future, but this can be postponed as we encounter more of such problems.
Sorry for my mistake. Then I have no other comments for this patch.
Thanks for your review. The comment is correct, as ShiftOnlyState means the state where only shift (and no other modifiers) can be pressed or not.
Hi Vitaly,
See the comment below:
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io>
On Behalf Of Vitaly
> Cheptsov via Groups.Io
> Sent: Monday, February 10, 2020 6:18 PM
> To: devel@edk2.groups.io
> Subject: [edk2-devel] [PATCH 1/1] ShellPkg: Add support for input with
> separately reported modifiers
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2510
>
> Some firmwares:
> - Report Shift modifier even when they report upper-case unicode letter.
> - Report Ctrl modifier with "shifted" UniChar (i.e. X - 'A' + 1).
>
> This change provides support for these firmwares preserving the compatibility
> with the previous input handling.
>
> Signed-off-by: Michael Belyaev <usrsse2@icloud.com>
> Reviewed-by: Vitaly Cheptsov <vit9696@protonmail.com>
> ---
> ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c | 37
> ++++++++++++++------
> ShellPkg/Library/UefiShellDebug1CommandsLib/EditInputBar.c | 28
> ++++++++++-----
> ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c | 6 ++++
> ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c | 11
> +++---
> 4 files changed, 58 insertions(+), 24 deletions(-)
>
> diff --git
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> index df530f1119..9615a4dfbd 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
> @@ -1381,8 +1381,8 @@ MainCommandDisplayHelp (
> continue;
> }
>
> - if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) == 0) ||
> - (KeyData.KeyState.KeyShiftState == EFI_SHIFT_STATE_VALID)) {
> + if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) == 0)
> + || (KeyData.KeyState.KeyShiftState == EFI_SHIFT_STATE_VALID)) {
> //
> // For consoles that don't support/report shift state,
> // CTRL+W is translated to L'W' - L'A' + 1.
> @@ -1390,14 +1390,17 @@ MainCommandDisplayHelp (
> if (KeyData.Key.UnicodeChar == L'W' - L'A' + 1) {
> break;
> }
> - } else if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0)
> &&
> - ((KeyData.KeyState.KeyShiftState & (EFI_LEFT_CONTROL_PRESSED |
> EFI_RIGHT_CONTROL_PRESSED)) != 0) &&
> - ((KeyData.KeyState.KeyShiftState & ~(EFI_SHIFT_STATE_VALID |
> EFI_LEFT_CONTROL_PRESSED | EFI_RIGHT_CONTROL_PRESSED)) == 0)) {
> + } else if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0)
> + && ((KeyData.KeyState.KeyShiftState & (EFI_LEFT_CONTROL_PRESSED |
> EFI_RIGHT_CONTROL_PRESSED)) != 0)
> + && ((KeyData.KeyState.KeyShiftState &
> + ~(EFI_SHIFT_STATE_VALID | EFI_LEFT_CONTROL_PRESSED |
> + EFI_RIGHT_CONTROL_PRESSED)) == 0)) {
> //
> // For consoles that supports/reports shift state,
> // make sure that only CONTROL shift key is pressed.
> + // For some consoles that report shift state,
> + // CTRL+W is still translated to L'W' - L'A' + 1.
> //
> - if ((KeyData.Key.UnicodeChar == 'w') || (KeyData.Key.UnicodeChar == 'W'))
> {
> + if ((KeyData.Key.UnicodeChar == L'w') || (KeyData.Key.UnicodeChar == L'W')
> + || (KeyData.Key.UnicodeChar == L'w' - L'a' + 1) ||
> + (KeyData.Key.UnicodeChar == L'W' - L'A' + 1)) {
> break;
> }
> }
> @@ -1834,7 +1837,8 @@ MainEditorKeyInput (
> EFI_KEY_DATA KeyData;
> EFI_STATUS Status;
> EFI_SIMPLE_POINTER_STATE MouseState;
> - BOOLEAN NoShiftState;
> + BOOLEAN NoModifierState;
> + BOOLEAN ShiftOnlyState;
>
> do {
>
> @@ -1886,17 +1890,28 @@ MainEditorKeyInput (
> //
> StatusBarSetRefresh();
> //
> - // NoShiftState: TRUE when no shift key is pressed.
> + // NoModifierState: TRUE when no modifier key is pressed.
> //
> - NoShiftState = ((KeyData.KeyState.KeyShiftState &
> EFI_SHIFT_STATE_VALID) == 0) || (KeyData.KeyState.KeyShiftState ==
> EFI_SHIFT_STATE_VALID);
> + NoModifierState = ((KeyData.KeyState.KeyShiftState &
> EFI_SHIFT_STATE_VALID) == 0)
> + || (KeyData.KeyState.KeyShiftState == EFI_SHIFT_STATE_VALID);
> + //
> + // ShiftOnlyState: TRUE when no modifier key except Shift is pressed.
> + //
> + ShiftOnlyState = ((KeyData.KeyState.KeyShiftState &
> EFI_SHIFT_STATE_VALID) == 0)
> + || ((KeyData.KeyState.KeyShiftState
> + & ~(EFI_SHIFT_STATE_VALID |
> + EFI_LEFT_SHIFT_PRESSED | EFI_RIGHT_SHIFT_PRESSED)) == 0);
> //
> // dispatch to different components' key handling function
> //
> if (EFI_NOT_FOUND != MenuBarDispatchControlHotKey(&KeyData)) {
> Status = EFI_SUCCESS;
> - } else if (NoShiftState && ((KeyData.Key.ScanCode == SCAN_NULL) ||
> ((KeyData.Key.ScanCode >= SCAN_UP) && (KeyData.Key.ScanCode <=
> SCAN_PAGE_DOWN)))) {
> + } else if ((ShiftOnlyState && (KeyData.Key.ScanCode == SCAN_NULL))
> + || (NoModifierState && (KeyData.Key.ScanCode >= SCAN_UP) &&
> (KeyData.Key.ScanCode <= SCAN_PAGE_DOWN))) {
> + //
> + // alphanumeric keys with or without shift, or arrow keys without shift
> + //
This is unmatched with the comments. It only handles the alphanumeric keys with shift.
Thanks,
Zhichao
> Status = FileBufferHandleInput (&KeyData.Key);
> - } else if (NoShiftState && (KeyData.Key.ScanCode >= SCAN_F1) &&
> (KeyData.Key.ScanCode <= SCAN_F12)) {
> + } else if (NoModifierState && (KeyData.Key.ScanCode >= SCAN_F1)
> + && (KeyData.Key.ScanCode <= SCAN_F12)) {
> Status = MenuBarDispatchFunctionKey (&KeyData.Key);
> } else {
> StatusBarSetStatusString (L"Unknown Command"); diff --git
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/EditInputBar.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/EditInputBar.c
> index 35b0b701e8..d053059220 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/EditInputBar.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/EditInputBar.c
> @@ -130,6 +130,8 @@ InputBarRefresh (
> UINTN EventIndex;
> UINTN CursorRow;
> UINTN CursorCol;
> + BOOLEAN ShiftPressed;
> + BOOLEAN ModifiersPressed;
>
> //
> // variable initialization
> @@ -180,17 +182,23 @@ InputBarRefresh (
> if (EFI_ERROR (Status)) {
> continue;
> }
> - if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0) &&
> - (KeyData.KeyState.KeyShiftState != EFI_SHIFT_STATE_VALID)) {
> - //
> - // Shift key pressed.
> - //
> + ModifiersPressed = ((KeyData.KeyState.KeyShiftState &
> EFI_SHIFT_STATE_VALID) != 0)
> + && (KeyData.KeyState.KeyShiftState !=
> + EFI_SHIFT_STATE_VALID);
> +
> + //
> + // TRUE if Shift is pressed and no other modifiers are pressed
> + //
> + ShiftPressed = ModifiersPressed &&
> + ((KeyData.KeyState.KeyShiftState &
> + ~(EFI_SHIFT_STATE_VALID | EFI_LEFT_SHIFT_PRESSED |
> + EFI_RIGHT_SHIFT_PRESSED)) == 0);
> +
> + if (ModifiersPressed && !ShiftPressed) {
> continue;
> }
> //
> // pressed ESC
> //
> - if (KeyData.Key.ScanCode == SCAN_ESC) {
> + if (!ModifiersPressed && KeyData.Key.ScanCode == SCAN_ESC) {
> Size = 0;
> Status = EFI_NOT_READY;
> break;
> @@ -198,9 +206,10 @@ InputBarRefresh (
> //
> // return pressed
> //
> - if (KeyData.Key.UnicodeChar == CHAR_LINEFEED ||
> KeyData.Key.UnicodeChar == CHAR_CARRIAGE_RETURN) {
> + if (!ModifiersPressed
> + && (KeyData.Key.UnicodeChar == CHAR_LINEFEED ||
> + KeyData.Key.UnicodeChar == CHAR_CARRIAGE_RETURN)) {
> break;
> - } else if (KeyData.Key.UnicodeChar == CHAR_BACKSPACE) {
> + } else if (!ModifiersPressed && KeyData.Key.UnicodeChar ==
> + CHAR_BACKSPACE) {
> //
> // backspace
> //
> @@ -213,7 +222,8 @@ InputBarRefresh (
>
> }
> }
> - } else if (KeyData.Key.UnicodeChar <= 127 && KeyData.Key.UnicodeChar >=
> 32) {
> + } else if ((!ModifiersPressed || ShiftPressed)
> + && KeyData.Key.UnicodeChar <= 127 &&
> + KeyData.Key.UnicodeChar >= 32) {
> //
> // VALID ASCII char pressed
> //
> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c
> index ca8bc506d9..eabbf3c571 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/EditMenuBar.c
> @@ -190,11 +190,17 @@ MenuBarDispatchControlHotKey (
> //
> // For consoles that supports/reports shift state,
> // make sure only CONTROL is pressed.
> + // For some consoles that report shift state,
> + // Ctrl+A is still translated to 1 (UnicodeChar).
> //
> if ((KeyData->Key.UnicodeChar >= L'A') && (KeyData->Key.UnicodeChar <=
> L'Z')) {
> ControlIndex = KeyData->Key.UnicodeChar - L'A' + 1;
> } else if ((KeyData->Key.UnicodeChar >= L'a') && (KeyData->Key.UnicodeChar
> <= L'z')) {
> ControlIndex = KeyData->Key.UnicodeChar - L'a' + 1;
> + } else if ((KeyData->Key.UnicodeChar >= L'A' - L'A' + 1) && (KeyData-
> >Key.UnicodeChar <= L'Z' - L'A' + 1)) {
> + ControlIndex = KeyData->Key.UnicodeChar;
> + } else if ((KeyData->Key.UnicodeChar >= L'a' - L'a' + 1) && (KeyData-
> >Key.UnicodeChar <= L'z' - L'z' + 1)) {
> + ControlIndex = KeyData->Key.UnicodeChar;
> }
> }
> if ((SCAN_CONTROL_Z < ControlIndex)
> diff --git
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c
> index a00df49d38..394e531c16 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/MainHexEditor.
> +++ c
> @@ -137,14 +137,17 @@ HMainCommandDisplayHelp (
> if (KeyData.Key.UnicodeChar == L'W' - L'A' + 1) {
> break;
> }
> - } else if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0)
> &&
> - ((KeyData.KeyState.KeyShiftState & (EFI_LEFT_CONTROL_PRESSED |
> EFI_RIGHT_CONTROL_PRESSED)) != 0) &&
> - ((KeyData.KeyState.KeyShiftState & ~(EFI_SHIFT_STATE_VALID |
> EFI_LEFT_CONTROL_PRESSED | EFI_RIGHT_CONTROL_PRESSED)) == 0)) {
> + } else if (((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) != 0)
> + && ((KeyData.KeyState.KeyShiftState & (EFI_LEFT_CONTROL_PRESSED |
> EFI_RIGHT_CONTROL_PRESSED)) != 0)
> + && ((KeyData.KeyState.KeyShiftState &
> + ~(EFI_SHIFT_STATE_VALID | EFI_LEFT_CONTROL_PRESSED |
> + EFI_RIGHT_CONTROL_PRESSED)) == 0)) {
> //
> // For consoles that supports/reports shift state,
> // make sure that only CONTROL shift key is pressed.
> + // For some consoles that report shift state,
> + // CTRL+W is still translated to L'W' - L'A' + 1.
> //
> - if ((KeyData.Key.UnicodeChar == 'w') || (KeyData.Key.UnicodeChar == 'W'))
> {
> + if ((KeyData.Key.UnicodeChar == 'w') || (KeyData.Key.UnicodeChar == 'W')
> + || (KeyData.Key.UnicodeChar == L'w' - L'a' + 1) ||
> + (KeyData.Key.UnicodeChar == L'W' - L'A' + 1)) {
> break;
> }
> }
> --
> 2.21.1 (Apple Git-122.3)
>
>
> -=-=-=-=-=-=
> Groups.io Links:
You receive all messages sent to this group.
>
> View/Reply Online (#54122): https://edk2.groups.io/g/devel/message/54122
> Mute This Topic: https://groups.io/mt/71133729/1768756
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [zhichao.gao@intel.com]
> -=-=-=-=-=-=