Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed: Holding and releasing mouse button is not inserting autocomplete item #2173

Merged
merged 12 commits into from
Jul 6, 2018

Conversation

engineering-this
Copy link
Contributor

@engineering-this engineering-this commented Jun 26, 2018

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 registered autocomplete items in focusManager, so focusing them wont trigger editor blur event which was cause for hiding dropdown. Note tabindex is required for items to be focusable.

Closes #2107.

@engineering-this
Copy link
Contributor Author

I'm thinking if we should treat this as feature instead of just fix.

@engineering-this engineering-this changed the base branch from major to master June 26, 2018 11:45
@engineering-this engineering-this changed the base branch from master to major June 26, 2018 11:45
@engineering-this
Copy link
Contributor Author

I will rebase it once master is merged with major.

@engineering-this engineering-this changed the base branch from major to next June 27, 2018 15:59
Copy link
Member

@Comandeer Comandeer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using [tabindex] introduces several quirks.

The most visible of them is default styling for focusable elements, which appears after any mouse related action (e.g. right click and closing native context menu):

Blue focus indicator around right-clicked element

The second quirk is connected with the fact that focusing any element with mouse breaks keyboard behaviour. Tab no longer inserts selected item into the editor, but focus the next element in tab order (in the manual test case – sidebar close button), arrow keys scroll the page and Esc does nothing.

@@ -586,7 +587,7 @@
* @readonly
* @property {CKEDITOR.template}
*/
this.itemTemplate = new CKEDITOR.template( '<li data-id="{id}">{name}</li>' );
this.itemTemplate = new CKEDITOR.template( '<li data-id="{id}" tabindex="-1">{name}</li>' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather add [tabindex] programatically, via element.$.tabIndex = -1 in addItem method. Requiring user to remember to add this attribute to itemTemplate seems unnecessary.

@engineering-this
Copy link
Contributor Author

After reconsidering this issue I decided to drop changes. I didn't delete commits, so it can be reverted.

For proper usage of autocomplete editor should remain focused all the time, so we can continue typing. Hence it shouldn't register anything in focusManager. Instead solution is to
event.preventDefault(); on mousedown, which solves the problem.

Now fix contain less code, quirks described above don't happen, and also this change solves #2126.

Copy link
Member

@Comandeer Comandeer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Comandeer Comandeer merged commit 746f71d into master Jul 6, 2018
@Comandeer Comandeer deleted the t/2107 branch July 6, 2018 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants