Disallow creating anchors with space characters #5362
Conversation
plugins/link/dialogs/anchor.js
Outdated
| @@ -136,10 +136,20 @@ CKEDITOR.dialog.add( 'anchor', function( editor ) { | |||
| label: editor.lang.link.anchor.name, | |||
| required: true, | |||
| validate: function() { | |||
| if ( !this.getValue() ) { | |||
| var disallowedWhitespacesRegex = /[\u0020\u0009\u000c]/g, | |||
There was a problem hiding this comment.
I would add also new line and carriage return characters here – just to be sure. They aren't possible to but they can be pasted from somewhere. Better safe than sorry.
It'd be also nice to link to the spec to explain why these characters are not allowed.
There was a problem hiding this comment.
I've added a new line and carriage return character but I was not able to add a unit test to it as it was impossible to paste it to input. Are we okay with it? 🤔
I've also added also short information about disallowed characters from spec.
plugins/link/lang/en.js
Outdated
| @@ -13,6 +13,7 @@ CKEDITOR.plugins.setLang( 'link', 'en', { | |||
| title: 'Anchor Properties', | |||
| name: 'Anchor Name', | |||
| errorName: 'Please type the anchor name', | |||
| errorWhitespace: 'Anchor name cannot contain whitespaces', | |||
There was a problem hiding this comment.
| errorWhitespace: 'Anchor name cannot contain whitespaces', | |
| errorWhitespace: 'Anchor name cannot contain any whitespace', |
Whitespace is mostly used as an uncountable noun.
There was a problem hiding this comment.
I've changed it to a space character, not a whitespace.
|
Thanks, @Comandeer for the review - it's ready to another one. |
|
Rebased onto the latest |
plugins/link/dialogs/anchor.js
Outdated
| @@ -136,10 +136,23 @@ CKEDITOR.dialog.add( 'anchor', function( editor ) { | |||
| label: editor.lang.link.anchor.name, | |||
| required: true, | |||
| validate: function() { | |||
| if ( !this.getValue() ) { | |||
| // W3C HTML 5.2 Recommendation; §3.2.5: | |||
There was a problem hiding this comment.
The HTML 5.2 specification does not longer exist. Could you update the reference to point to https://html.spec.whatwg.org/multipage/dom.html#global-attributes ?
plugins/link/lang/en.js
Outdated
| @@ -13,6 +13,7 @@ CKEDITOR.plugins.setLang( 'link', 'en', { | |||
| title: 'Anchor Properties', | |||
| name: 'Anchor Name', | |||
| errorName: 'Please type the anchor name', | |||
| errorWhitespace: 'Anchor name cannot contain space character', | |||
There was a problem hiding this comment.
There is some small discrepancy between this message and the message in the manual test. IMO the version from the test ("space characters") sounds better than the version in language files ("space character").
Probably it's safer to consult it with our docs team to ensure that the grammatical form of the message is ok 🤔 It would be much harder to do that after the release.
There was a problem hiding this comment.
After talking with @godai78 we have 3 possible options:
Anchor name cannot contain space characters-> your versionAnchor name cannot contain any spaces-> this may be not obviousAnchor name cannot contain any space characters
For me, 1 and 3 look equally good to me, WDYT?
KarolDawidziuk
force-pushed
the
t/5305
branch
from
f16a458 to
429f112
Compare
|
Thanks, @Comandeer for the review. |
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?
I've updated a
validatemethod in anchor dialog by checking if the given name a space character.Which issues does your PR resolve?
Closes #5305.