Fix for ability to activate buttons using right click

[Comandeer]

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?

I've added additional check if left mouse button was used for click. I also add ability to get info about mouse button from native DOM event in CKEDITOR.tools.getMouseButton.

Closes #2565.

[engineering-this]

There is one issue with Edge.
Right clicking on toolbar items changes editor selection. While any other click outside of an iframe doesn't affect editor selection at all.

[engineering-this]

Please move function declaration to end of block.

[engineering-this]

Same as above.

[engineering-this]

Please update version tag to 4.11.3.

[Comandeer]

I'd rather keep it the way it is. After inlining we'd have two conditionals (ternary operator and &&) in one line.

[engineering-this]

Small simplification could be done. Ternary operator could be moved to generateEvent param, so changed lines would look like:

assert.areSame( input[ 0 ], CKEDITOR.tools.getMouseButton( generateEvent( input[ isIe8 ? 1 : 0 ] ) ) );
(...)
[ CKEDITOR.MOUSE_BUTTON_LEFT, 1 ],

Improvements for this test case could be also applied on previous one.

[engineering-this]

This function might be reused in some future tests.
Keys '0', '1', '2' doesn't mean a lot. I understand, we can't use computed key names in current es version. So instead we could define them like following:

ie8ButtonMap[ CKEDITOR.MOUSE_BUTTON_LEFT ] = 1;
ie8ButtonMap[ CKEDITOR.MOUSE_BUTTON_MIDDLE ] = 4;
ie8ButtonMap[ CKEDITOR.MOUSE_BUTTON_RIGHT ] = 2;

But thats ugly, and repetitive, so instead maybe just add comment in each line with button name, e.g.:

var ie8ButtonMap = {
	0: 1, // CKEDITOR.MOUSE_BUTTON_LEFT
	(...)

WDYT?

[Comandeer]

Comments sound like a good idea 👍

[Comandeer]

The issue with Edge is present also on current master branch, so it's a separate bug. What's more you must click toolbar button as the first thing after selecting text, clicking anywhere else first and then on button does not trigger the bug. However I don't think there is much sense in reporting it, as it's a real edge case (no pun intended).

[engineering-this]

Just two small things, everything else looks good to me.

[engineering-this]

I could resolve my own suggestion, but only the first one. For the second GH said 'You are not allowed to resolve suggestions'. So I just manually pushed commit 😂

[f1ames]

Looks good, just two small things (see comments). Also it seems added unit test are failing on IE9, IE10 🤔

[Comandeer]

Fixed failing tests. It was connected with too strict conditional checking if the event is instance of native DOM event. Now the function assumes that passed evt option is native DOM event if it doesn't have data property.

I've also refactored button's template according to the advice to be more readable.