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.

Sign up for GitHub

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

Jump to bottom

Fixed: [tableselection] table contents can be removed in a readonly mode #1527

Merged
merged 10 commits into from
Sep 3, 2018
Merged

Fixed: [tableselection] table contents can be removed in a readonly mode #1527

merged 10 commits into from
Sep 3, 2018

Conversation

jacekbogdanski
Copy link
Member

@jacekbogdanski jacekbogdanski commented Jan 30, 2018 •
edited
Loading

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?

Disabled table selection delete on keypress and keydown events in a readonly mode.

NOTE: This pull request depends on #1525 fix. It was created from t/1525 branch.

Based on #1692

Closes #1489

Comandeer
Comandeer requested changes Apr 11, 2018
View reviewed changes
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.

This PR includes changes from #1525, which is already merged. Please rebase this PR to latest major and remove redundant changes.

@jacekbogdanski
Copy link
Member Author

I rebased to the latest major and refactored tests a little (like tag update, test fixture down etc. - more in the commit message). Could you elaborate what redundant changes would you like to see removed? The tests are very similar to the one from #1525 PR but they are validating different case scenarios.

@Comandeer
Copy link
Member

Could you elaborate what redundant changes would you like to see removed?

The ones removed by rebase ;)

Comandeer
Comandeer requested changes Apr 12, 2018
View reviewed changes
tests/plugins/tableselection/tableselection.js
},

// (#1489)
'test random keys are not removing readonly selection': function( editor ) {
Copy link
Member

Choose a reason for hiding this comment

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

Test is failing in IE11.

plugins/tableselection/plugin.js Outdated
@@ -1022,6 +1022,10 @@
firstCell,
i;

if ( editor.readOnly ) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add comment with reference to the issue.

plugins/tableselection/plugin.js
@@ -972,7 +972,7 @@
i;

// Handle only left/right/del/bspace keys.
Copy link
Member

Choose a reason for hiding this comment

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

Please adjust comment and add reference to the issue.

CHANGES.md Outdated
@@ -63,6 +63,7 @@ Fixed Issues:
* [#1516](https://github.com/ckeditor/ckeditor-dev/issues/1516): Fixed: Fake selection allows removing content in read-only mode using the <kbd>Backspace</kbd> and <kbd>Delete</kbd> keys.
* [#1570](https://github.com/ckeditor/ckeditor-dev/issues/1570): Fixed: Fake selection allows cutting content in read-only mode using the <kbd>Ctrl</kbd>/<kbd>Cmd</kbd> + <kbd>X</kbd> keys.
* [#1363](https://github.com/ckeditor/ckeditor-dev/issues/1363): Fixed: Paste notification is unclear and it might confuse users.
* [#1489](https://github.com/ckeditor/ckeditor-dev/issues/1489): Fixed: [tableselection](https://docs.ckeditor.com/ckeditor4/latest/api/CKEDITOR_plugins_tableselection.html) table contents can be removed in readonly mode.
Copy link
Member

Choose a reason for hiding this comment

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

Please change this reference to reference to plugin:

[Table Selection](https://ckeditor.com/cke4/addon/tableselection) plugin

@jacekbogdanski
Copy link
Member Author

I rebased this branch into t/1632 (#1692). The issue with IE is already fixed with this PR. I think it's better idea to rebase it into fixed branch than ignoring unit tests for IE.

@mlewand mlewand added the review:easy Pull requests that can be reviewed by a Junior Developer before being reviewed by the Reviewer. label Jul 10, 2018
@Comandeer Comandeer changed the base branch from major to master August 27, 2018 14:30
Comandeer
Comandeer requested changes Aug 27, 2018
View reviewed changes
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.

Please rebase onto latest master.

@jacekbogdanski jacekbogdanski self-assigned this Aug 31, 2018
@jacekbogdanski jacekbogdanski changed the base branch from master to next August 31, 2018 11:20
@Comandeer Comandeer self-assigned this Sep 3, 2018
Comandeer
Comandeer approved these changes Sep 3, 2018
View reviewed changes
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 237dbdf into next Sep 3, 2018
@CKEditorBot CKEditorBot deleted the t/1489 branch September 3, 2018 10:40
@Comandeer Comandeer mentioned this pull request Sep 3, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Comandeer Comandeer Comandeer approved these changes

Assignees
No one assigned
Labels
review:easy Pull requests that can be reviewed by a Junior Developer before being reviewed by the Reviewer.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jacekbogdanski @Comandeer @mlewand