Added additional keystroke handlings for elementspath and toolbar plugins #2412
Conversation
mlewand
added
the
review:easy
|
I've rebased branch onto latest |
There was a problem hiding this comment.
Unit tests are failing in IE8.
Additionally please look at https://dev.ckeditor.com/ticket/13168#comment:13 as it provides info about suggested solution.
| }, | ||
|
|
||
| // (#438) | ||
| 'test focusing editor': function() { |
There was a problem hiding this comment.
It seems that these tests have common logic, that can be extracted into helper function.
There was a problem hiding this comment.
I don't know if it's worth it. We will remove very small duplication here because almost half of the test will have to be still configured by arguments (assertion, keystroke, and spy).
Of course, we could extract test code into 2 helper functions shared between toolbar and elementspath test suites, but is it worth effort for such small duplication inside unit tests?
During IE8 bug fix I find out that it requires some custom event handler attribute logic. Extracting it now makes sense.
| }, | ||
|
|
||
| // (#438) | ||
| 'test focusing editor': function() { |
There was a problem hiding this comment.
The same here: common logic can be extracted.
plugins/elementspath/plugin.js
Outdated
| case CKEDITOR.ALT + 121: // ALT + F10 (#438). | ||
| editor.execCommand( 'toolbarFocus' ); | ||
| return false; | ||
| case CKEDITOR.ALT + 122: // ALT + F11 (#438). |
There was a problem hiding this comment.
There is already case for focusing editor. Additionally I'm not sure if we really need it, as Esc already handles it.
plugins/toolbar/plugin.js
Outdated
| case CKEDITOR.ALT + 122: // ALT + F11 (#438). | ||
| editor.execCommand( 'elementsPathFocus' ); | ||
| return false; | ||
| case CKEDITOR.ALT + 121: // ALT + F10 (#438). |
There was a problem hiding this comment.
The same here: there is already case for focusing editor.
| @@ -0,0 +1,31 @@ | |||
| @bender-ui: collapsed | |||
| @bender-tags: 4.11.0, 438, bug | |||
| @@ -0,0 +1,31 @@ | |||
| @bender-ui: collapsed | |||
| @bender-tags: 4.11.0, 438, bug | |||
| @@ -0,0 +1,31 @@ | |||
| @bender-ui: collapsed | |||
There was a problem hiding this comment.
Actually is this test needed? It seems the same as tests/plugins/elementspath/manual/focus.md.
There was a problem hiding this comment.
Looks like rebase artifact, I will remove it right away.
| @@ -0,0 +1,31 @@ | |||
| @bender-ui: collapsed | |||
| @bender-tags: 4.11.0, 438, bug | |||
|
Unfortunately, IE8 is unable to resolve event object when element event attribute handler is executed directly on the element (which is required to test this case). I find out a workaround for this issue, although it requires some small but tricky code. I moved it into a separate helper function. |
| * @param {String} eventName Event handler attribute name. | ||
| * @param {Object} evt Event payload. | ||
| */ | ||
| function fireElementEventHandler( element, eventName, evt ) { |
There was a problem hiding this comment.
Helper copied and moved to bender.tools.fireElementEventHandler. More details: #2828
|
I reuse helper ( |
jacekbogdanski
added
the
pr:frozen ❄
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?
Added additional keystroke handling for elementspath and toolbar. I had some hard time with unit tests for this changes - decided to mock everything instead of more integration testing. The reason is that it's almost impossible to easily test focus change between elementspath, toolbar, and editor.
I also extended the issue with toggling between elementspath and toolbar.
Closes #438