Fix element id is not escaped properly in IE #4666
Conversation
|
This is a known issue, other ppl also reported similar issues before: #681. For us, the ID could have colon so we need to improve the I'm applying a similar fix for escaping colon. Thanks! |
|
Hi, @limingli0707 Since that is a part of the solution of a bigger issue, which you mentioned in your comment. Maybe you could work on it a little bit more and address the solution to solve #681? We can't work on partial solutions, because that will introduce 'use-cases' patching development and could lead to terrible code after a few months. That should solve your particular issue, but also helps others with the same source of the problem. I will be glad to hear from you about that. |
|
Thanks @sculpt0r for reviewing the PR. I will put a generic solution to fix all issues related to IE. |
|
Thank you :) |
|
@sculpt0r I have updated the PR with a general solution. Actually that is the solution that MDN suggested too https://developer.mozilla.org/en-US/docs/Web/API/CSS/escape |
There was a problem hiding this comment.
Hi @limingli0707
We are happy that you want to contribute to CKEditor4 and help to make it even better. Thanks for this update.
We should start formalities. They are also important for us, to make sure everything is in right place and has a purpose. Also, sorry for the long list, but we need to be sure everything is working as expected and it won't fail anymore. I think it is also important for you as a developer.
- Please update the reference ticket, so now closing section will point to the original issue: table elements (td, tr, th, ..) with an id that starts with dot (.) causes JavaScript runtime err #681
- Please update the proposed changelog, so it will match wit the resolution
- I see that you used polyfill copied from https://github.com/mathiasbynens/CSS.escape/blob/master/css.escape.js It is important because we need to remember to be fully transparent with licenses. This one is based on MIT so, I assume that will be no problem, but please @f1ames - could you say a word about this?
As for the code:
- I know you adjust the new unit test naming to the existing convention in
tools.js. However, as for the test naming, please stick to such convention:what is testing, in what circumstances, what is expected. It helps a lot to understand such a test whenever it fails. So for this particular case, it could be:test escapeCss escaped colon in the css selector. Feel free to propose anything that comes to your mind and will match with this convention. - Please add at least one unit test that verifies selector with the dot
.character, as it is described in the original issue. - Please add other unit tests that confirm, that the current flow is working properly with the specification (https://drafts.csswg.org/cssom/#serialize-an-identifier) even if this is still a draft.
- It will be nice to have a manual test with a test scenario like in the original issue. Could you also add one?
- I know it copied as it is from quite a reliable source, but since it could be integrated into our code base, please spend a while refactoring it, to match our code style. It is important to keep all code base consistent as much as possible.
|
Hi @sculpt0r Thanks for your input! I have addressed the comments, would you like to take a look at it again? Thanks! |
limingli0707
changed the title
|
Rebase on newest master |
|
Hi @sculpt0r, I just rebased the latest master. Thanks a lot! |
There was a problem hiding this comment.
I added a few comments.
Besides those, I'm glad that you add some extra unit tests for edge cases 👍
Please, don't commit anything new while you are not assigned to PR. It leads to the situation when I'm not sure if the code doesn't change after a while... Also, be sure that you are finished when you ask about another review. Be patient. It's better to take a break and return to PR after a day, than sending unpolished changes or adding changes while someone else is reviewing.
So, be sure you are assigned to PR until your work is done :)
Overall, very good job 👍 We just need to polish this solution.
|
Hi @sculpt0r Thank you very much for your input! I am trying to assign myself to the PR but I am not able to see the action/option. I am guessing I might miss some permission to do that. Do you have any thoughts? |
|
Hi @limingli0707 Let me know if you find this option, in the meantime I try to verify it on our side 👍 |
|
OK - seems that this option is permitted for people outside the team. So I will assign you manually each time I send a review, and assign myself after your send a review request and I started to work on it. |
There was a problem hiding this comment.
Hi, @limingli0707
Good job with all changes. I've added some polishing comments. Looks like, after those - it could be merged 👍
Beside that. Could you create another manual test that covers the case from #641? It appears that your PR also fix this issue 👍
Also, please make a pull because I rebased this PR on the newest master branch.
|
Thank you so much @sculpt0r . I have learned a lot from you!! |
|
Sorry about pushing some extra commits after requesting a review. I just realized that some commits didn't get pushed correctly. Hope you don't mind :) |
Tests won't run code which causes issue without tableselection plugin
There was a problem hiding this comment.
Hi @limingli0707
LGTM 🎉 👍
Everything looks good now. Please take a look at some final steps I've made:
- Licence update - because we are basing on someone else solution: a8399c3
- I added your nick to the changelog 😉 : 869ddbf
- but also added one entry that was lost after our rebases: 869ddbf
- Small correction for
ifstatement: f447cb4
However, I must say, that the manual tests weren't so good. Especially the second one added later. They don't verify the issues at all, because they didn't run. There were missed plugins, steps that didn't reproduce original issue scenarios, expected messages that weren't adequate to real observations. Please take a look at them now: 7ee7ebd and c257a73
Overall: very good job! 👍 Thank you for your contribution, time and involvement. Also, your responses were so fast that sometimes it was a pleasure to have such fluent cooperation.
|
Thank you so much for your help and guidance @sculpt0r ! It was a pleasure to work with you on this RP and I have learned a lot! |
What is the purpose of this pull request?
Bug fix.
issue here: #681
Basically, IE doesn't support CSS.escape, so we need to escape it manually.
In our case, id could have colon, so we need to escape it in IE.
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?
Minor change in the tool.js > escapeCss(). When CSS.escape is not available (in IE), we need to escape colon otherwise it will cause javascript err. I saw we already did very similar escape for leading numbers due to the incomplete implementation of CKEDITOR.tools.escapeCss(). I'm just following the same pattern to escape colon.
Which issues does your PR resolve?
Closes #681
Closes #641