Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The getEnvKeystrokeText utility breaks for certain combos starting in v26 #10439

Closed
jzempel opened this issue Aug 27, 2021 · 7 comments · Fixed by #10546
Closed

The getEnvKeystrokeText utility breaks for certain combos starting in v26 #10439

jzempel opened this issue Aug 27, 2021 · 7 comments · Fixed by #10546
Assignees
Labels
squad:core Issue to be handled by the Core team. support:2 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior. type:regression This issue reports a bug that was not present in the previous releases.

Comments

@jzempel
Copy link

jzempel commented Aug 27, 2021

📝 Provide detailed reproduction steps (if any)

import { getEnvKeystrokeText } from '@ckeditor/ckeditor5-utils/src/keyboard';

getEnvKeystrokeText('CTRL+[');
getEnvKeystrokeText('CTRL+]');

Prevents Zendesk Garden from updating the demo Editor.

✔️ Expected result

Successfully parse this common keystroke for indent/outdent. This used to work in v25.

❌ Actual result

Unexpected error while loading ./stories/index.stories.js: CKEditorError: keyboard-unknown-key {"key":"["}

❓ Possible solution

Debug changes made in v25.0.0...v26.0.0#diff-8a8feb9905511e353215fac103af1ff8dbf4ed40e7e24d3994c63e28f1a8f756.

📃 Other details

  • Browser: any
  • OS: any
  • First affected CKEditor version: v26
  • Installed CKEditor plugins: not relevant

Possibly related to #9412


If you'd like to see this fixed sooner, add a 👍 reaction to this post.

@Mgsy
Copy link
Member

Mgsy commented Aug 30, 2021

Hi! Thanks for the report. It seems that [ and ] characters are not supported, so the editor doesn't know how to translate them to key codes. 

For now, you can add it on your own by adding them manually to the generateKnownKeyCodes() function:

keyCodes[ '[' ] = 219;
keyCodes[ ']' ] = 221;

Successfully parse this common keystroke for indent/outdent. This used to work in v25.

I've checked it at versions < 26.0.0 and it seems to behave the same, so it doesn't seem to be a regression.

@Mgsy Mgsy added the pending:feedback This issue is blocked by necessary feedback. label Aug 30, 2021
@jzempel
Copy link
Author

jzempel commented Aug 30, 2021

@Mgsy that seems to patch the runtime parsing error. However EditingKeystrokeHandler still seems to be affected as setting the corresponding UI shortcuts results in undefined.

Screen Shot 2021-08-30 at 12 34 20 PM

@jzempel
Copy link
Author

jzempel commented Aug 30, 2021

I've checked it at versions < 26.0.0 and it seems to behave the same, so it doesn't seem to be a regression.

This history is documented in zendeskgarden/ckeditor#17. What am I missing?

@Reinmar Reinmar added intro Good first ticket. squad:core Issue to be handled by the Core team. labels Aug 31, 2021
@Mgsy
Copy link
Member

Mgsy commented Aug 31, 2021

Thanks for the details! I've checked it once again and indeed, getEnvKeystrokeText was working properly with [ and ] characters before version 26.0.0. It looks like this is a regression introduced by this commit - 5c2a69a.

@Mgsy Mgsy added type:regression This issue reports a bug that was not present in the previous releases. and removed pending:feedback This issue is blocked by necessary feedback. labels Aug 31, 2021
@Reinmar
Copy link
Member

Reinmar commented Sep 1, 2021

We can go in two directions:

  • Somehow revert the change that we made so keys that our tool does not recognize ([ and ] would not work when e.g. trying to set such a keystroke – they only worked in getEnvKeystrokeText()).
  • Add more keys to the list of known keys.

I'm for the latter to be consistent. If we went with the first option, getEnvKeystrokeText() would work, but parseKeystroke() e.g. wouldn't. It makes no sense.

So the characters to add, at least looking at my keyboard layout: `-=[];',./\ 

I simply went through all the special characters on my keyboard. We should also scan 2-3 other common keyboard layouts.

Question: How to support characters that require Shift? E.g. {} on my keyboard at least. Can they be used for keystrokes? What about other keyboard layouts?

@Reinmar Reinmar removed the intro Good first ticket. label Sep 1, 2021
@Reinmar
Copy link
Member

Reinmar commented Sep 1, 2021

BTW, @jzempel, are you sure that you want to override Cmd+[? This keystroke is used by some people to navigate back/forth in the browser history.

@Reinmar
Copy link
Member

Reinmar commented Sep 1, 2021

I simply went through all the special characters on my keyboard. We should also scan 2-3 other common keyboard layouts.

Question: How to support characters that require Shift? E.g. {} on my keyboard at least. Can they be used for keystrokes? What about other keyboard layouts?

Since this is the first issue in this category for a long time, let's ignore these doubts for now. Let's:

  •  add the aforementioned characters to the list
    • don't forget about updating export const keyCodes documentation,
  • make sure that the utils in keyboard.js throw a meaningful error when passed an unknown key

@Reinmar Reinmar added this to the iteration 47 milestone Sep 13, 2021
@arkflpc arkflpc self-assigned this Sep 16, 2021
niegowski added a commit that referenced this issue Sep 17, 2021
Fix(utils): Adds more known key codes. For instance, to allow `Ctrl+]` keystroke. Closes #10439.
@Reinmar Reinmar added the support:2 An issue reported by a commercially licensed client. label Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
squad:core Issue to be handled by the Core team. support:2 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior. type:regression This issue reports a bug that was not present in the previous releases.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants