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

Prevent of throwing an error when is no selection. #359

Merged
merged 9 commits into from
Jun 8, 2017
Merged

Prevent of throwing an error when is no selection. #359

merged 9 commits into from
Jun 8, 2017

Conversation

beatadelura
Copy link
Contributor

@beatadelura beatadelura commented May 11, 2017

@beatadelura beatadelura changed the title T/16874 Prevent of throwing an error when is no selection. May 11, 2017
@mlewand mlewand requested a review from f1ames May 26, 2017 09:31
@@ -0,0 +1,18 @@
@bender-tags: 4.7.1, 16874
Copy link
Contributor

Choose a reason for hiding this comment

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

The manual test only covers the issue case on Chrome (there is no selection when pressing enter) while in other browsers the selection exists and cannot be removed/dismissed by clicking inside of the editor.

Maybe it would be possible to add a second scenario for the test which will be covering button removing selection/ranges from the editor to check if enter does not produce an error then (similar as in unit test).

The exampe of test with more than one scenario: https://github.com/ckeditor/ckeditor-dev/blob/master/tests/plugins/clipboard/manual/13883.md.

Keep in mind that after pressing button outside of the editor, the editor loses focus - probably clicking should first focus the editor and then remove selection.

@@ -521,7 +521,8 @@
// Check path block specialities:
// 1. Cannot be a un-splittable element, e.g. table caption;
var path = editor.elementPath();
if ( !path.isContextFor( 'p' ) ) {

if ( path && !path.isContextFor( 'p' ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is similar issue #357 and @Comandeer mentioned a more generic approach there which may cover all those cases, which is generally a good approach. The cause of the issue is the fact that enter key press is handled while there is no selection (while editable area is focused). This is however a responsibility of the core to handle such situations and enterkey plugin by itself also should do some additional checkings as above.

Having said that, I think this check is sufficient for this case.

@beatadelura
Copy link
Contributor Author

@f1ames PTAL

@f1ames
Copy link
Contributor

f1ames commented Jun 8, 2017

LGTM! I just added some small changes in tests.

@f1ames f1ames self-requested a review June 8, 2017 06:27
@f1ames
Copy link
Contributor

f1ames commented Jun 8, 2017

Rebased with latest master.

@f1ames f1ames merged commit 40176d2 into ckeditor:master Jun 8, 2017
@f1ames f1ames deleted the t/16874 branch June 8, 2017 06:47
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