Fix forceSimpleAmpersand config implementation #2416
Conversation
Run ampersand replacement after the htmlEncodeAttr, otherwise the results are overwritten (ckeditor#965)
There was a problem hiding this comment.
There are a few code style issues, but the fix and unit test looks to be good 👌. Thanks for the contribution!
I have noted most of the code style issues, but I'll correct them in a follow-up to this PR.
@alexmaris please, make sure that you're using latest master or major branch (depending whether you're making a bugfix or a feature) in the future. As your current branch is based on 4.8.0 version and contains conflicts that we need to resolve before we can accept your PR.
| // Browsers don't always escape special character in attribute values. (http://dev.ckeditor.com/ticket/4683, http://dev.ckeditor.com/ticket/4719). | ||
| attValue = CKEDITOR.tools.htmlEncodeAttr( attValue ); | ||
|
|
||
| // Run ampersand replacement after the htmlEncodeAttr, otherwise the results are overwritten (https://github.com/ckeditor/ckeditor-dev/issues/965) |
There was a problem hiding this comment.
We refer to GH ticket in a shorter notation, just (#965) is enough. We only use links for other resources, e.g. our old Trac bug tracker.
| @@ -39,5 +39,39 @@ bender.test( { | |||
| assert.areSame( afterFormat, bot.getData( false, false ) ); | |||
| } ); | |||
| } ); | |||
| }, | |||
| 'test editor config.forceSimpleAmpersand in html element attributes': function () { | |||
There was a problem hiding this comment.
It would be nice to mention the issue related to this test case too. Also other tests have a one line of spacing between the test cases.
| }, | ||
| 'test editor config.forceSimpleAmpersand in html element attributes': function () { | ||
| var data = '<p><a href="http://www.blah.com?foo=1&bar=2">Test link</a></p>'; | ||
| bender.editorBot.create({ |
There was a problem hiding this comment.
We use spacing between ({ I can see that you didn't run the linting task. Please make sure to use it in the future, that will show all the code style issues.
| bot.setData( data, function () { | ||
| var afterFormat = bot.getData( false, false); | ||
|
|
||
| // Trigger getData a second time to reveal bug. |
There was a problem hiding this comment.
Actually you're not calling the getData method for a second time, you called setData first, so this comment is misleading.
|
Changed the target to |
|
@mlewand my apologies for the sloppy PR, thank you for the help fixing it up and merging the fix. |
Run ampersand replacement after the htmlEncodeAttr, otherwise the results are overwritten (#965)
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
What changes did you make?
Run ampersand replacement after CKEDITOR.tools.htmlEncodeAttr() executes against the given attribute
Closes #965.