Impossible to navigate to editable from toolbar by keyboard and vice versa.

[Artur Delura]

1. Open editor page perhaps ​http://sdk.ckeditor.com/samples/accessibility.html.
2. Focus on editable.
3. Press Alt + F10 to navigate to toolbar.
4. Press Alt + F11 to navigate to elements path.

Result: You can't navigate to elements path.

Expected Result: You can navigate to elements path.

It should be possible to navigate to elements path right froom toolbar and vice versa.

[Tade0]

This didn't work, because the keystroke handler was attached only to the editable body.

I attached it to the element's path bar and the toolbar.

Changes pushed to ​branch:t/13168.

[Piotrek Koszuliński]

So what happens now when you press e.g. CTRL+B while having a focus in the toolbar/element path?

[Tade0]

The same as if you'd simply press the button "Bold".

[Marek Lewandowski]

This behavior is reasonable. The code looks fine, but I'm missing manual test for it.

It make sense to put a test for it into tests/core/manual/keystrokehandler as a general purpose manual test (without tc tag).

[Piotrek Koszuliński]

The same as if you'd simply press the button "Bold".

What I meant was that it may happen that some features won't want to work with a focus in the toolbar, because it's not a usual situation. So +1 for a manual test.

[Tade0]

Added manual test, rebased and fixed a minor bug that appeared when testing.

Changes pushed to ​branch:t/13168.

[Marek Lewandowski]

• We're still having some integration issues - I've added two more failing manual test cases. I'm sure there are couple more.
• There are way to many plugins added in 1.md manual test.
• 1.md is to little specific as for TC.
• Still we need general-use manual test.

We need to take steps more cautiously, otherwise this change might backfire rather badly.

Design

• Keys from outside editable (toolbar, elements path) must not fire ​editor#key event. This event is commonly used for in-editable operations.
• Only keystrokes added with ​editor#setKeystroke with a special parameter should be handled in toolbar, elements path.
  • Currently editor#setKeystroke takse 2 parameters, we could an extra parameter based on following constants:
    • CKEDITOR.KEYSTROKE_TOOLBAR
    • CKEDITOR.KEYSTROKE_ELEMENTS_PATH
    • CKEDITOR.KEYSTROKE_EDITOR
    • CKEDITOR.KEYSTROKE_ALL

So that you could make a call like:

editor.setKeystroke( CKEDITOR.CTRL + 76 /*L*/, 'link', CKEDITOR.KEYSTROKE_ALL );
editor.setKeystroke( CKEDITOR.ALT + 122 /*F11*/, 'elementsPathFocus', CKEDITOR.KEYSTROKE_ALL );
editor.setKeystroke( CKEDITOR.CTRL + CKEDITOR.SHIFT + 76 /*L*/, 'myFancyCommand', CKEDITOR.KEYSTROKE_ELEMENTS_PATH | CKEDITOR.KEYSTROKE_TOOLBAR );

Because of increased risk, let's move it to major release.

[Tade0]

Added context to keystrokes. The default context is KEYSTROKE_EDITOR, since this is how it worked before these changes.

Rebased with major, pushed to ​branch:t/13168.

[Marek Lewandowski]

Moving to 4.6.2 minor release, as 4.6.1 is mostly about polishing 4.6.0.

[kkrzton]

As target milestone changed from major to master, I rebased the branch on current master and pushed to ​branch:t/13168. The original branch was moved to ​branch:t/13168b for possible future references.

On the rebased branch I also removed ​9c9776 commit which fixed jslint errors in core/editable.js. While fixing this file to remove jslint errors seems tempting I will omit it as an addition to any fix as it makes the diff unreadable and the whole fix much harder to understand... I know it should be done, I also encounter this issue few times, but I think the good idea will be to create separate ticket for this (which can then cherry-pick fixing commit from the original issue branch).

[kkrzton]

There are few unit tests failing. You can see travis build: ​https://travis-ci.org/cksource/ckeditor-dev/builds/188515783. I run tests manually to recheck and the result was the same. Failing TCs:

tests/core/config/inline
tests/core/config/inline2
tests/core/editor/keystrokehandler
tests/core/selection/fake
tests/plugins/undo/change
tests/plugins/undo/keyboard
tests/plugins/widget/nestededitables
tests/plugins/widget/widgetsintegration

5669 passed / 23 failed in 10m 29s 581ms - 100%

To make sure it is not the rebase fault, I also checked the original branch (based on previous major). The tests results were the same.

I am not sure what is the exact reason of failing tests, maybe context is missing in some places or some events are canceled instead of propagating, but it have to be investigated in more detail.

[Tade0]

Reason for the tests failing was that at least one method was not updated in accordance to the changes made in CKEDITOR.editor.setKeystroke. Some tests also suffered from the same problem.

Changes pushed to ​branch:t/13168.

[Tomasz Jakut]

Case 1:

1. Open ​http://tests.ckeditor.dev:1030/tests/tickets/13168/2.
2. Click widget.
3. Press Alt+F10.
4. Press Right Arrow.
5. Move mouse.

Expected result: border on image is still there.

Actual result: border on image is gone.

Case 2:

Test ​http://tests.ckeditor.dev:1030/tests/tickets/13168/3 is failing: only spaces are inserted.

I've rebased ticket to the newest major and updated tags in tests. Pushed to ​branch:t/13168.

[Tade0]

It took a while but I finally noticed the obvious:

To determine if a command should be executed the appropriate event is fired and the result of that operation is taken into consideration.

[Tade0]

Change the keystroke handler behaviour so that in the editor context keystrokes are blacklisted, while in other context they are whitelisted.

Updated one manual test so that the changes are reflected.

Changes pushed to ​branch:t/13168.

[kkrzton]

Added some docs improvements. Overall the solution looks pretty solid, however unit tests for new context parameter are missing, it will be good to add those.