Fix for: Can't open context menu with keyboard shortcut over widget

[Dumluregn]

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

• Unit tests
• Manual tests

What changes did you make?

The issue occured only for SHIFT + F10 shortcut (CTRL + SHIFT + F10 was working fine). Therefore I allowed SHIFT keystrokes to be passed to other listeners the same way as CTRL and ALT are which solved the problem.

Closes #1901 .

[jacekbogdanski]

Seems to work, however, such change requires unit tests coverage. You can see some examples here:
https://github.com/ckeditor/ckeditor-dev/blob/15c4181d1e6cab08a11d59220a820ef605ef29f7/tests/plugins/widget/undo.js#L287-L312

You wanna know if keystrokes with shift are not canceled, so it should be enough to fire key event with the correct keystroke and check if it didn't return false.

[jacekbogdanski]

Both unit tests you created are fine. They are checking if SHIFT key has not been canceled so the event can be propagated further. However, such test should be probably placed in widgetsintegration.js file because it's more like a generic test case. Although, we will still be missing integration test for context menu. Issue request concerns this plugin, so it makes sense to add an additional test to make sure if the context menu is opened for a widget.

In case of creating such integration test, you will need to find out the whole keyboard handling flow, thus context menu listens on keydown event, but widget issue is placed inside artificial key event handler. In shortcut, you wanna use keydown event on editable, which will be propagated to the context menu and widget by keystrokehandler:

editor.editable().fire( 'keydown', new CKEDITOR.dom.event( { keyCode: CKEDITOR.SHIFT + 121 } ) );

and check if contextMenu event has been fired for widget instance.

[jacekbogdanski]

Only minor refactoring left. Make sure to correct all similar issues in PR pointed out in this R-.

[jacekbogdanski]

LGTM, a couple of minor issues which I resolve in the upcoming commits. Although, make sure that you agree with my changes.

[f1ames]

The problem with this solution is the fact that it enables all Shift + some key shortcuts when widget is focused. So apart from Shift + F10 all other Shift shortcuts will start to work which may lead to some unexpected results (the list of shortcuts is on our docs page - here and here). For example Shift + Left / Right moves selection to the content end and logs an error:

Shift + Enter inserts new paragraph before widget, which upon undoing logs an error:

Shift + Tab moves selection out of the editor, while before it was simply disabled not changing selection.

While I agree that there might be some shortcuts which could be enabled, most of them were disabled on focused widget for a reason and won't work or will break something. My suggestion is to simply enable Shift + F10 combination.

---

When it comes to manual test we usually try to keep things simple not loading too much plugins. However, in this case, shortcuts are provided by few plugins and without them we cannot check if something is broken (cases mentioned above will not fail on provided manual test). My suggestion here is to add at least one plugin which has its own shortcuts and extend test by the additional case checking if this shortcut still does not have any effect while widget is focused.

Also I would add both block and inline widgets there (now there is only inline one).

[f1ames]

Added unit tests are failing on Safari:

See also review comments (some minor polishing).

[Dumluregn]

I double-checked the tests on Safari and with clear cache they pass in my browser (Version 12.0.3 (14606.4.5)).

[f1ames]

I double-checked the tests on Safari and with clear cache they pass in my browser.

Seems to be also a cache issue on my side.

---

There is one problem with positioning context menu in IE8 but it is not related to this fix so I will report a follow-up.