Fix for activating various UI items via right click #2859

Comandeer · 2019-02-24T21:50:45Z

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 is the proposed changelog entry for this pull request?

Fixed Issues:

* [IE, Edge] Fixed: Various editor's UI elements react to right mouse button click:
	* [#2845](https://github.com/ckeditor/ckeditor-dev/issues/2845): [Rich Combo](https://ckeditor.com/cke4/addon/richcombo).
	* [#2857](https://github.com/ckeditor/ckeditor-dev/issues/2857): [List Block](https://ckeditor.com/cke4/addon/listblock).
	* [#2858](https://github.com/ckeditor/ckeditor-dev/issues/2858): [Menu](https://ckeditor.com/cke4/addon/menu).

API Changes:

* [#2845](https://github.com/ckeditor/ckeditor-dev/issues/2845): The [`CKEDITOR.tools.normalizeMouseButton`](https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_tools.html#method-normalizeMouseButton) was added.
* [#2975](https://github.com/ckeditor/ckeditor-dev/issues/2975): The [`CKEDITOR.dom.element#fireEventHandler`](https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_dom_element.html#method-fireEventHandler) was added.

What changes did you make?

I've fixed ability to use rich combos, listblocks and menus with right mouse button.

To do so I had to add CKEDITOR.tools.fireElementEventHandler to be able to emulate mouseup events with correct mouse button info in keystrokes handler in panel.

Closes #2845.
Closes #2857.
Closes #2858.
Closes #2975.

jacekbogdanski

I don't like magic numbers for IE8 passed as a mouse button keystroke. I see that IE8 returns different mouse button code than other browsers, however, the mapping should be done explicitly to avoid guessing what's going on. I'm wondering if we could just make reversed getMouseButton mapping public?

jacekbogdanski · 2019-03-06T15:00:32Z

tests/plugins/menu/manual/rightclick.md

+## Procedure
+
+1. Open console.
+2. Open editor's context menu.


Maybe Open editor's context menu by right-clicking its content. to make it more brainless?

tests/plugins/menu/manual/rightclick.md

jacekbogdanski · 2019-03-06T15:02:38Z

tests/plugins/menu/manual/rightclick.md

+
+	### Expected result:
+
+	Focus is moved to the clicked item.


Not valid for a single context menu item which is already preselected.

tests/plugins/menu/manual/rightclick.html

jacekbogdanski · 2019-03-06T15:04:57Z

tests/plugins/menu/menu.js

+
+		// (#2858)
+		'test right-clicking menu items': function() {
+			if ( !CKEDITOR.env.ie ) {


I would also enable this test for other browsers for regression protection.

@Comandeer can you refer to this comment? Is this similar situation as with other tests that you find the regressions very unlikely to happen?

Yes, it's de facto impossible. We use click event in every other browser, so right click will never occur in the first place. In IE it occurred as we use mouseup event there due to some nasty bug in IE's native image controls.

jacekbogdanski · 2019-03-06T15:49:47Z

tests/core/tools.js

+				'<a href="#" onmouseup="this.setAttribute(\'data-button\',event.button);return false;">Link</a>' );
+
+			CKEDITOR.tools.fireElementEventHandler( link, 'onmouseup', {
+				button: 2


Please, use enum values for readability (CKEDITOR.MOUSE_BUTTON_RIGHT). Update the rest of the proposed tests with enum values.

jacekbogdanski · 2019-03-06T15:53:47Z

tests/plugins/panel/panel.js

+
+			block._.markFirstDisplayed();
+
+			block.onKeyDown( 13 );


Add info about pressed keystroke e.g. block.onKeyDown( 13 ); // ENTER.

tests/plugins/richcombo/manual/rightclick.html

jacekbogdanski · 2019-03-06T15:55:17Z

tests/plugins/richcombo/richcombo.js

+
+	// (#2565)
+	'test right-clicking combo': function() {
+		if ( !CKEDITOR.env.ie ) {


Again, I think it makes sense to enable test for all browsers.

@Comandeer same as in https://github.com/ckeditor/ckeditor-dev/pull/2859/files?file-filters%5B%5D=.js&file-filters%5B%5D=.md#r262980964.

As I stated there, I do not think such regression is even possible.

jacekbogdanski · 2019-03-06T16:06:50Z

plugins/panel/plugin.js

+							// We must pass info about clicked button (#2857).
+							CKEDITOR.tools.fireElementEventHandler( focusable, 'on' + keyAction, {
+								button: ( CKEDITOR.env.ie && ( CKEDITOR.env.version < 9 || CKEDITOR.env.ie6Compat ) ) ?
+									1 : CKEDITOR.MOUSE_BUTTON_LEFT


This IE8 magic number which additionally can be mistaken with a middle button click (thus 1 == CKEDITOR.MOUSE_BUTTON_MIDDLE should be replaced with correct mapping.

Comandeer · 2019-03-23T17:59:17Z

I've replaced most of magic numbers with bilateral mouse button normalization via newly added CKEDITOR.tools.normalizeMouseButton method. Additionally I've added CKEDITOR.dom.element#fireEventHandler method and fixed unit tests (as mouse event were dispatched from wrong document object, causing tests to always pass).

I didn't enable tests in any other browsers, as I don't see any reason to do it. The whole issue originates from the fact that we use mouseup event in IE, which is fired for every mouse click, not only ones made with primary button. In contrary, all other browsers use click event, which is restricted only to primary button clicks (bold's mine):

The click event type MUST be dispatched on the topmost event target indicated by the pointer, when the user presses down and releases the primary pointer button, or otherwise activates the pointer in a manner that simulates such an action.

There is no point of switching click to mouseup in any other browser, so IMO it's safe to ignore these tests in other browsers – as regression is very unlike to ever occur there.

Comandeer · 2019-03-25T13:12:40Z

I see that introduced changes caused CI to fail due to keyboard handling in emoji. I'll fix it.

Comandeer · 2019-03-25T14:20:42Z

I've fixed incorrect handling of keyboard in emoji plugin, using the same technique that was already used in our old approach: if there isn't element[ 'on' + eventName ] property, use element[ eventName ] one. Thanks to that clicks can be fired using element.click.

jacekbogdanski

Tests checking if right click focuses item fails e.g.:
tests/plugins/listblock/manual/rightclick
tests/plugins/menu/manual/rightclick
Right click doesn't focus clicked item, you can verify it by right-clicking item and pressing enter. In that case, the default option will be applied instead of the selected one. Also, there is no visible focus for context menu.

jacekbogdanski · 2019-03-26T10:29:38Z

core/tools.js

@@ -1466,26 +1466,72 @@
 		 * @since 4.7.3
 		 * @param {CKEDITOR.dom.event/Event} evt DOM event. Since 4.11.3 a native `MouseEvent` instance can be passed.
 		 * @returns {Number|Boolean} Returns a number indicating the mouse button or `false`
-		 * if the mouse button cannot be determined.
+		 * if the mouse button cannot be determined. The mouse button is normalized using {@link CKEDITOR.tools#normalizeMouseButton}.


Do we need this info revealing implementation details?

jacekbogdanski · 2019-03-26T10:42:39Z

core/tools.js

+		 *
+		 * ```
+		 * +--------------+------+----------------+
+		 * | Mouse button |  IE  | Other browsers |


Maybe IE -> IE8 / IE9 QM / IE9 CM so you need quick glimpse to find out different mappings for these browsers?

Comandeer · 2019-05-18T16:39:40Z

Tests checking if right click focuses item fails e.g.:
tests/plugins/listblock/manual/rightclick
tests/plugins/menu/manual/rightclick
Right click doesn't focus clicked item, you can verify it by right-clicking item and pressing enter. In that case, the default option will be applied instead of the selected one. Also, there is no visible focus for context menu.

This is the mistake in test description, caused by the fact that visible focus indicator (outline) is in fact moved to the clicked item. I fixed tests descriptions.

I've also rebased the whole PR onto latest major.

jacekbogdanski

When right-clicking any toolbar button on Edge using manual tests, a selection from editor disappears:

The issue also sometimes occurs when the right-clicking button of language button (IE10) or right-clicking dropdown item (IE10 and below).

Seems to work fine on Chrome, Firefox.

jacekbogdanski · 2019-05-23T12:37:18Z

core/dom/element.js

+		 * } );
+		 * ```
+		 *
+		 * @since 4.11.4


Please, update version tags across all changes.

core/tools.js

jacekbogdanski · 2019-05-23T12:47:39Z

core/tools.js

+
+				if ( mapping[ 0 ] === button && reverse ) {
+					return mapping[ 1 ];
+				} else if ( !reverse && mapping[ 1 ] === button ) {


else if could be replaced with simpler if condition thus return statement.

jacekbogdanski · 2019-05-23T12:53:35Z

tests/_benderjs/ckeditor/static/tools.js

+		},
+
+		/*
+		 * Fires specified mouse event on given element


Missing dot, block comment should start with two *. Also grammar:
Fires specified mouse event on the given element.

jacekbogdanski · 2019-05-23T13:09:32Z

core/dom/element.js

+		 */
+		fireEventHandler: function( eventName, evt ) {
+			var handlerName = 'on' + eventName,
+				element = this.$;


What do you think about renaming native element with $ prefix? I.e. $element. I know it's not something we are doing in our code base but makes code much more readable for me.

I'm not a big fan of it. IMO the name of the variable is readable enough on its own. TBH I'm more eager to change to something like nativeElement than add $ to the beginning – as it would be readable also for users that do not know the convention.

nativeElement also works for me 👍

tests/plugins/menu/manual/rightclick.md

jacekbogdanski · 2019-05-23T14:25:46Z

CHANGES.md

@@ -12,6 +12,9 @@ Fixed Issues:

 * [#2672](https://github.com/ckeditor/ckeditor-dev/issues/2672): Fixed: When resizing [Enhanced Image](https://ckeditor.com/cke4/addon/image2) to minimum size with a resizer the image dialog doesn't show actual values.
 * [#1478](https://github.com/ckeditor/ckeditor-dev/issues/1478): Fixed: Custom colors added to [Color Button](https://ckeditor.com/cke4/addon/colorbutton) via [`config.colorButton_colors`](https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_config.html#cfg-colorButton_colors) in form label/code don't work correctly.
+* [#2845](https://github.com/ckeditor/ckeditor-dev/issues/2845): [Rich Combo](https://ckeditor.com/cke4/addon/richcombo) reacts to right mouse button click.
+* [#2857](https://github.com/ckeditor/ckeditor-dev/issues/2857): [List Block](https://ckeditor.com/cke4/addon/listblock) items react to right mouse button click.
+* [#2858](https://github.com/ckeditor/ckeditor-dev/issues/2858): [Menu](https://ckeditor.com/cke4/addon/menu) items react to right mouse button click.


I'm wondering about some entry change log merge here as they have mostly the same message. WDYT?

I was also thinking about it, but I looked through the whole changelog and didn't spot anything like that. Because of that I decided to create separate entries.

Actually I got an idea, how to do it. bed3e35 shows the proposed solution.

[ci skip] Co-Authored-By: Krzysztof Krztoń <k.krzton@cksource.com>

f1ames · 2019-07-31T14:14:17Z

Rebased onto latest major.

f1ames

LGTM 👍 Well done @Comandeer @jacekbogdanski 🎉

Comandeer closed this Feb 26, 2019

Comandeer changed the base branch from next to master February 26, 2019 13:16

Comandeer reopened this Feb 26, 2019

Comandeer force-pushed the t/2845 branch from 45606d4 to 1e23471 Compare February 26, 2019 13:19

mlewand added the review:easy Pull requests that can be reviewed by a Junior Developer before being reviewed by the Reviewer. label Mar 3, 2019

jacekbogdanski self-assigned this Mar 6, 2019

jacekbogdanski requested changes Mar 6, 2019

View reviewed changes

CKEditorBot unassigned jacekbogdanski Mar 6, 2019

Comandeer self-assigned this Mar 23, 2019

Comandeer force-pushed the t/2845 branch from 1e23471 to db57bbf Compare March 23, 2019 17:48

Comandeer requested a review from jacekbogdanski March 23, 2019 18:00

CKEditorBot unassigned Comandeer Mar 23, 2019

jacekbogdanski self-assigned this Mar 25, 2019

Comandeer assigned Comandeer and unassigned jacekbogdanski Mar 25, 2019

Comandeer removed their assignment Mar 25, 2019

Comandeer force-pushed the t/2845 branch from a6e896e to e665b28 Compare March 25, 2019 14:41

jacekbogdanski self-assigned this Mar 25, 2019

jacekbogdanski requested changes Mar 26, 2019

View reviewed changes

CKEditorBot unassigned jacekbogdanski Mar 26, 2019

Comandeer force-pushed the t/2845 branch from e665b28 to ac92633 Compare May 18, 2019 16:39

Comandeer changed the base branch from master to major May 18, 2019 16:40

Comandeer requested a review from jacekbogdanski May 18, 2019 16:42

jacekbogdanski self-assigned this May 23, 2019

jacekbogdanski requested changes May 23, 2019

View reviewed changes

CKEditorBot unassigned jacekbogdanski May 23, 2019

Comandeer and others added 21 commits July 31, 2019 16:13

Introduce normalization in tests.

415ba5e

Force dispatchMouseEvent to use appropriate document.

155d4c9

Improve API docs.

8a0b2fb

Simplify normalizeMouseButton implementation.

20e2338

Use event name if there is no handlerName property.

4222fe1

Fix incorrect test description.

45a7899

Small fixes for API docs.

96b63c6

Update version tag.

5ab8760

Update API docs.

fce3190

Refactoring conditions inside normalizeMouseButton.

c00a700

Update variable name.

b2bd157

Add manual test for keyboard integration in panel.

c7a00bd

Update manual test for menu plugin.

a21c757

Update version tags.

35dd0d9

Removed rebase leftover.

21955d9

Reword result in manual test.

12c5a2a

[ci skip] Co-Authored-By: Krzysztof Krztoń <k.krzton@cksource.com>

Add info about bug in Firefox.

fdc901b

Add sourcearea to menu manual test.

e439b3e

Update API docs for mouse button normalization.

74f814f

Declare mappings only in IE.

92cc8b7

Enhance spacing in onclick definitions.

56701c8

f1ames force-pushed the t/2845 branch from 06c6a8e to 56701c8 Compare July 31, 2019 14:14

f1ames added 2 commits July 31, 2019 16:33

Minor docs rewording. [skip ci]

d80e9d1

Added changelog entry. [skip ci]

428360d

f1ames approved these changes Jul 31, 2019

View reviewed changes

CKEditorBot unassigned f1ames Jul 31, 2019

f1ames merged commit 00c4c02 into major Jul 31, 2019

CKEditorBot deleted the t/2845 branch July 31, 2019 14:53

f1ames removed this from the 4.13.0 milestone Jul 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for activating various UI items via right click #2859

Fix for activating various UI items via right click #2859

Comandeer commented Feb 24, 2019 •

edited

Loading

jacekbogdanski left a comment

jacekbogdanski Mar 6, 2019

jacekbogdanski Mar 6, 2019

jacekbogdanski Mar 6, 2019

f1ames Jul 16, 2019

Comandeer Jul 16, 2019

jacekbogdanski Mar 6, 2019

jacekbogdanski Mar 6, 2019

jacekbogdanski Mar 6, 2019

f1ames Jul 16, 2019

Comandeer Jul 16, 2019

jacekbogdanski Mar 6, 2019

Comandeer commented Mar 23, 2019

Comandeer commented Mar 25, 2019

Comandeer commented Mar 25, 2019 •

edited

Loading

jacekbogdanski left a comment •

edited

Loading

jacekbogdanski Mar 26, 2019

jacekbogdanski Mar 26, 2019

Comandeer commented May 18, 2019

jacekbogdanski left a comment •

edited

Loading

jacekbogdanski May 23, 2019

jacekbogdanski May 23, 2019

jacekbogdanski May 23, 2019

jacekbogdanski May 23, 2019

Comandeer Jun 17, 2019

jacekbogdanski Jun 17, 2019

jacekbogdanski May 23, 2019

Comandeer Jun 17, 2019

Comandeer Jun 17, 2019

f1ames commented Jul 31, 2019

f1ames left a comment

Fix for activating various UI items via right click #2859

Fix for activating various UI items via right click #2859

Conversation

Comandeer commented Feb 24, 2019 • edited Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What is the proposed changelog entry for this pull request?

What changes did you make?

jacekbogdanski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Comandeer commented Mar 23, 2019

Comandeer commented Mar 25, 2019

Comandeer commented Mar 25, 2019 • edited Loading

jacekbogdanski left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Comandeer commented May 18, 2019

jacekbogdanski left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f1ames commented Jul 31, 2019

f1ames left a comment

Choose a reason for hiding this comment

Comandeer commented Feb 24, 2019 •

edited

Loading

Comandeer commented Mar 25, 2019 •

edited

Loading

jacekbogdanski left a comment •

edited

Loading

jacekbogdanski left a comment •

edited

Loading