-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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 editor throwing an exception when destroyed shortly after it was created #2258
Conversation
…d (race condition)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we shouldn't add a manual test for this changes. As I understand this issue somehow touched users using CKEditor so IMO we should cover it with a manual test, especially that current unit test is false positive and may give invalid result in the future.
tests/plugins/wysiwygarea/destroy.js
Outdated
|
||
bender.test( { | ||
// (#2257) | ||
'test destroy editor on instance created': function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test always passes despite changes.
Fixed the unit test. I decided to remove As for manual test I related to that in:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closing this PR as there is already more general PR on the way #3128. |
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
Skipped the manual test as this is pretty much an internal change, and the case presented for manually reproduced the issue required refreshing the page.
What changes did you make?
This PR adds two early returns, making sure that delayed editor initialization doesn't take place in case it was removed before.
4 people in total worked on this one :D I had to adjust the unit tests, as they were not failing for previous CKEditor releases, thus it would not protect us from a regression.
This is a follow-up of a #273 PR.
Closes #2257, #718.