Make sure the ID is always inserted if entered in the advanced dialog #1914
Conversation
|
I have trouble understanding the test suite and how to write an appropriate test for this... |
| @@ -1662,7 +1662,8 @@ CKEDITOR.STYLE_OBJECT = 3; | |||
|
|
|||
| // Avoid ID duplication. | |||
| if ( targetDocument.getCustomData( 'doc_processing_style' ) && el.hasAttribute( 'id' ) ) | |||
| el.removeAttribute( 'id' ); | |||
| if ( targetDocument.getById(el.getAttribute('id')) != null ) | |||
There was a problem hiding this comment.
This is not a proper fix for this. Actually the problem originates from bad doc_processing_style custom data management.
If you trace the code changes, you'll notice that it was moved from setupElement function to applyStyleOnSelection - and this is exactly when the bug was introduced.
This attribute was used to, sort of, mark a single transaction - but in case of editing an existing link this function wasn't even called, thus it didn't clear this "switch".
All that is needed here is to restore it and it should work correctly.
There was a problem hiding this comment.
Also the code style is inconsistent, see spacing next to parenthesis.
|
Thanks for the contribution, since it has been a while since you pushed the PR we'll take it from here and add the missing parts in a follow-up pull request. |
mlewand
changed the title
…ialog" This reverts commit 3b91f1f. The propsed fixed is not needed, see #1914 (comment) for more details.
…ialog" This reverts commit 3b91f1f. The propsed fixed is not needed, see #1914 (comment) for more details.
What is the purpose of this pull request?
Fixes issue #1046
Does your PR contain necessary tests?
Not yet - will add later
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?
When creating a new element based on style it would remote the ID attribute to prevent duplicates.
This bit of code was added in 2011 to prevent an issue with copying links (according to the git blame and git log 235dc2a) - I did not retest this use case as I did not know where to find this issue
I modified it so it only removes ID's if the chosen ID already exists in the targetDocument