-
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
Fix For: Pasting individual list elements from Firefox on Windows fails #3416
Conversation
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.
Thx for the contribution!
The fix seems to work correctly. I can't think of situation, in which we have to preserve whitespace before and after comments delimiting clipboard content.
However I'm missing some tests. You added unit test to check if the fix works correctly, but there aren't any tests checking if the reported issue is really fixed. The manual test should be easier to create – just prepare the list and editor with instructions what to copy and paste. You can look at other tests in tests/plugins/clipboard/manual
to get an idea how to construct such test. Unit test is probably more troublesome to write, but there are still some existing examples (tests/plugins/clipboard/paste.js
). Unit test should be basically the automated version of the manual one.
Can I ask you to add the tests? If you encounter any problems with it, I'll be glad to help.
Please also rebase the branch and point to major
, instead of master
, as the upcoming version of CKEditor 4 is based on major
branch.
@Comandeer I have made the requested changes |
Sorry for the delay. We're preparing to launch 4.13.0 version of CKEditor 4 and code is basically frozen. I'll come back to your PR as soon as we'll be accepting new PRs. |
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.
LGTM! I've just added one additional test and slightly enhanced the manual one.
Thanks for the contribution!
What is the purpose of this pull request?
Bug fix
Does your PR contain necessary tests?
Yes
This PR contains
What is the proposed changelog entry for this pull request?
Fix an issue that caused pasting part of a list to fail on Firefox
What changes did you make?
Trim the whitespace before
<!--StartFragment-->
and after<!--EndFragment-->
by updating the regular expression. I am not 100% certain that this is the correct approach, but it does fix the issue in Firefox on Windows and all of the tests continue to pass. I can't think of any good reason why we would want to keep that whitespace, although I'm not familiar with the browser paste formats.Closes #3415.