Fixed alt + a key combination for editor content selection on Windows
#1423
Conversation
There was a problem hiding this comment.
I'm missing a unit test. It should be possible to set editor with a widget at the beginning of the content, and then fake fire key combinations on it.
There should be at least two cases:
- ctrl+65
- ctrl+alt+65
As for your communication, it's not about Alt key but AltGr key which is a slightly different case.
Other than that looks OK.
|
|
||
| Test should be performed on a Windows operating system with polish keyboard layout. You can't validate this test case on UNIX based systems. | ||
|
|
||
| 1. Set system settings to use polish keyboard layout. |
There was a problem hiding this comment.
Polish keyboard layout (...)
|
|
||
| ---- | ||
|
|
||
| Test should be performed on a Windows operating system with polish keyboard layout. You can't validate this test case on UNIX based systems. |
There was a problem hiding this comment.
Polish keyboard layout (...)
| @@ -0,0 +1,13 @@ | |||
| <div id="editor01"> | |||
There was a problem hiding this comment.
Awkward name for the editor. We typically set it just to editor1. 🙂
There was a problem hiding this comment.
You have changed name in CKEDITOR.replace call, but you left this div without a change, thus manual test is breaking and doesn't work.
| <script> | ||
| var config = { | ||
| height: 200, | ||
| allowedContent: true |
There was a problem hiding this comment.
There's no reason for disabling ACF with allowedContent: true. Actually yhere's no need for custom config at all.
| 2. Focus editor instance. | ||
| 3. Press `Alt + a` to insert `ą` letter. | ||
|
|
||
| **Expected:** |
There was a problem hiding this comment.
Please, use headers for expected / unexpected sections.
|
@jacekbogdanski could you please change the name of editor too? See #1423 (comment) |
There was a problem hiding this comment.
There are still some issues.
I'll fix them in a follow up commits.
There was also an issue I fixed in 500ac4f - p[contenteditable] was actually removed, since it was not allowed, so the manual test was not checking the actual problem (as it was changed to a regular paragraph).
| ## Expected | ||
| `ą` letter has been inserted at the caret position. Editor content has not been selected. | ||
|
|
||
| ## Expected |
There was a problem hiding this comment.
This is an Unexpected result.
| @@ -0,0 +1,13 @@ | |||
| <div id="editor01"> | |||
There was a problem hiding this comment.
You have changed name in CKEDITOR.replace call, but you left this div without a change, thus manual test is breaking and doesn't work.
…acritics works as expected.
|
Pushed corrections, and rebased onto latest master branch. |
What is the purpose of this pull request?
Bug fix
Does your PR contain necessary tests?
All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.
This PR contains
What changes did you make?
Used CKEDITOR.dom.event.getKeystroke function to define content selection by
cmd/ctrl+akey combination.Closes #1419