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

Fix change event not fired on type after paste #308

Closed

Conversation

millerdev
Copy link
Contributor

@millerdev millerdev commented Dec 8, 2016

Steps to reproduce the bug:

  • Setup change event listener to record change events in the console. Something like this:
    var i = 0;
    editor.on("change", function () {
        console.log("change", i++);
    });
  • Paste some text into the editor and note change event number in console.
  • Type a single character.

Expected:
Change event should fire after typing (should see new change event number since paste).

Observed:
Change event is not fired.

@f1ames
Copy link
Contributor

f1ames commented Dec 9, 2016

Hello @millerdev,
thanks for the PR. We will try to look into it soon. Just as a notice, here is a related bug ticket from our official bug tracker: http://dev.ckeditor.com/ticket/13763.

@aik099
Copy link

aik099 commented Dec 9, 2016

@f1ames , could you also look at #297?

@millerdev
Copy link
Contributor Author

@f1ames is there anything else needed to get this approved/merged? I'd love to see this bug fixed on the main line.

@f1ames
Copy link
Contributor

f1ames commented Jan 4, 2017

Hello @millerdev,
we will look into it when fixing #13763. However, the fix is not scheduled for any upcoming release at the moment so it is hard to say when exactly we can take care of this one.

In the meantime you could make two improvements which will help us with this fix:

Thank you!

@millerdev
Copy link
Contributor Author

Thanks @f1ames, I'll work on those things. Out of curiosity, why are manual tests needed if I have a fully automated test for this bug? The contributing guidelines imply that manual tests are necessary only if it is not possible to test automatically.

@mlewand
Copy link
Contributor

mlewand commented Jan 5, 2017

I've explained it a little in #297 (comment). It's true that docs needs to be updated here, we'll update them shortly.

@millerdev
Copy link
Contributor Author

@f1ames @mlewand rebased, all tests passing on travis, manual test added.

@djdenv
Copy link

djdenv commented Jun 21, 2017

👍 Can we get this targeted for an upcoming release soon?

@mlewand mlewand added this to the Backlog milestone Jun 21, 2017
@mlewand
Copy link
Contributor

mlewand commented Jun 21, 2017

Sorry, we're in the middle of 4.7.1 release testing phase, thus we're unable to fit it in. But I'll put it in backlog to review it for 4.7.2, thanks for the feedback!

Also @djdenv you add 👍 reaction to the pull request description to show your support 😉.

@djdenv
Copy link

djdenv commented Jun 21, 2017

Thank you. Yes, I'm using the thumbs up to show support for merging this PR. 😉

@Comandeer
Copy link
Member

This PR was used as a base for #666. After adding tests and fixing some code style issues, the solution initially proposed by @millerdev was merged into our master and it'll become part of 4.7.3 release. Thanks for contributing!

@Comandeer Comandeer closed this Aug 18, 2017
@mlewand mlewand removed this from the Backlog milestone Aug 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants