-
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 encoding entities #5306
Fix encoding entities #5306
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.
@KarolDawidziuk - I need to ask you to add some tests. I didn't look at the code much more (but we discussed it ;) ).
We missed tests (since the beginning, not only for this PR) for force
option 🤔 It is mentioned in docs but our tests only use true
value. So before we touch the code even more - let's establish the current logic with tests 🤞
@sculpt0r I've added more unit tests with |
Rebased on the newest master. |
Looks good - thank you for that :) |
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.
@KarolDawidziuk - FF, Safari, Chrome looks good 👍 IE also is not blown up, so another 👍
Please take a look at my final inline comments. Sorry for dragging you through writing tests not explicitly related to the issue itself, but we never know when we come back here. So if we have already deep dive into this plugin - we might add tests that will cover this feature for future changes - hope you understand 😓
Thanks, @sculpt0r for the review. I've applied changes mentioned in your comments except a change with bender tags in the manual test and PR is ready for another check. |
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.
Thanks for all new test cases 👍
LGTM 🎉
What is the purpose of this pull request?
Bug fix
Does your PR contain necessary tests?
All patches that 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
Did you follow the CKEditor 4 code style guide?
Your code should follow the guidelines from the CKEditor 4 code style guide which helps keep the entire codebase consistent.
What is the proposed changelog entry for this pull request?
What changes did you make?
There was an issue with using
String#charCodeAt()
in theentities
plugin that returns an integer from 0 to 65535 representing UTF-16 code unit at the given index. In the case when index was greater than 65535 method returns a surrogate pair. To fix this I usedcodePointAt()
which returns a Unicode code point value at the given index.Another issue was with the regex which was unable to match and replace given unicodes. To fix this I updated existing entities regex by adding a
u
- unicode option.Unfortunately, this is not working on IE because
codePointAt()
and Unicode option are not supported there. Although it is possible to correctly convert HTMLEntity based on the values from the surrogate pair(see d148812) I was unable to find a solution to support the Unicode option on IE.Which issues does your PR resolve?
Closes #4941.