Text part language support #9074
Conversation
| { | ||
| "name": "@ckeditor/ckeditor5-language", | ||
| "version": "25.0.0", | ||
| "description": "Block quote feature for CKEditor 5.", |
| @@ -0,0 +1,16 @@ | |||
| CKEditor 5 language feature | |||
There was a problem hiding this comment.
Let's be consistent here – "Language feature".
Also, cc @AnnaTomanek, do you think it makes sense to call it like this?
There was a problem hiding this comment.
"Language" looks fine (as it's consistent with CKE4). Another option would be "text language" as it then would be clear that it's not about the UI language. In WCAG the success criterion is named "Language of Parts", the lang attribute itself defines the language of text/element. To make it short, we can go with "language" as the feature name and then use other keywords (language of text, language of content, language of text parts) in the docs.
There was a problem hiding this comment.
I'm for "text language" and renaming this plugin. I'd prefer to avoid confusion as much as we can.
There was a problem hiding this comment.
text language would also improve options config. For now, I had to go with languageList to avoid duplication with language. Not sure if CKE5 follow plugin name == config name semantic in the whole codebase, but in most cases, I saw that they are the same.
There was a problem hiding this comment.
I didn't know we actually used config.language already. So thats a must to rename this feature. The name of the property should match the name of the plugin.
| } | ||
|
|
||
| /** | ||
| * The configuration of the language feature. Introduced by the {@link module:language/languageediting~LanguageEditing} feature. |
There was a problem hiding this comment.
We need to be super clear here that it's not about the editor interface language. Ideally, let's link to https://ckeditor.com/docs/ckeditor5/latest/features/ui-language.html in a couple most critical places.
| * @module language/utils | ||
| */ | ||
|
|
||
| import { getLanguageDirection } from '@ckeditor/ckeditor5-utils/src/language'; |
There was a problem hiding this comment.
This import is incorrect and I wonder why it wasn't caught by the linter. It should go through ckeditor5/....
| * @module utils/language | ||
| */ | ||
|
|
||
| const RTL_LANGUAGE_CODES = [ |
There was a problem hiding this comment.
Are the 3-letter versions in the standard? In the UI we support the 2-letter ones only, AFAIK.
There was a problem hiding this comment.
3-letter version is supported by CKE4.
| import { getLanguageDirection } from '../src/language'; | ||
|
|
||
| describe( 'language', () => { | ||
| describe( 'getLanguageDirection', () => { |
There was a problem hiding this comment.
| describe( 'getLanguageDirection', () => { | |
| describe( 'getLanguageDirection()', () => { |
|
|
||
| describe( 'language', () => { | ||
| describe( 'getLanguageDirection', () => { | ||
| it( 'determines the language direction', () => { |
There was a problem hiding this comment.
Avoid one huge tests. Better to have multiple smaller ones so you know which fails.
| describe( 'language', () => { | ||
| describe( 'getLanguageDirection', () => { | ||
| it( 'determines the language direction', () => { | ||
| expect( getLanguageDirection( 'en' ) ).to.eql( 'ltr' ); |
There was a problem hiding this comment.
| expect( getLanguageDirection( 'en' ) ).to.eql( 'ltr' ); | |
| expect( getLanguageDirection( 'en' ) ).to.equal( 'ltr' ); |
No idea what's the difference, but we use the longer form everywhere else.
| * | ||
| * const languageConfig = { | ||
| * options: [ | ||
| * { title: 'Arabic', class: 'ck-language_ar', languageCode: 'ar' }, |
There was a problem hiding this comment.
Where is that class used? I'm not sure the ck- prefix makes sense, but it depends on the context.
There was a problem hiding this comment.
I should've read further:
* The `title` and `class` properties will be used by the `language` dropdown to render available options.
I don't think anyone will be styling these options in the dropdown so I don't think we need to have specific classes for them. If anything, we can have these classes added automatically.
In the heading feature, the classes are configurable as you're supposed to style the headings in the dropdown to look like headings in the content. And for that you need these classes.
| */ | ||
|
|
||
| /** | ||
| * @module block-quote/languageediting |
There was a problem hiding this comment.
Is this block-quote correct here?
| * | ||
| * The `title` and `class` properties will be used by the `language` dropdown to render available options. | ||
| * | ||
| * The `languageCode` property is used for the lang attribute in ISO 639 format. Language codes can be found |
There was a problem hiding this comment.
As I mentioned in the other place – so far I think we've been supporting two-letter versions. But if CKE4 allowed using 3-letter ones, then it makes sense to have them covered this way here.
| import LanguageUI from './languageui'; | ||
|
|
||
| /** | ||
| * The language plugin. |
There was a problem hiding this comment.
This is also a good place to clarify that this is not about UI language but an ability to set the language of parts of the content.
| /** | ||
| * The available language options. | ||
| * | ||
| * The default value is: |
There was a problem hiding this comment.
Is the default from CKE4? It's quite useless, but I suppose it's on purpose – it ensures that the feature works by default, but requires to be reconfigured when integrated for real.
Thus, we don't need to have translations for these language titles. Note: in many other features such strings are covered by language files and we have automatic translations for them. But here, it makes no sense.
This is something that may not be obvious to others (that we don't translate them on purpose), so it'd be good to have a comment about that in the code (next to the default value?). It may be hard to spot if you don't know that you're looking for it, but it's better than nothing.
There was a problem hiding this comment.
Is the default from CKE4? It's quite useless, but I suppose it's on purpose – it ensures that the feature works by default, but requires to be reconfigured when integrated for real.
Yes, and yes :) that's the purpose of this default configuration in CKE4.
| * @param {module:core/editor/editor~Editor} editor | ||
| * @param {String} attributeKey Attribute that will be set by the command. | ||
| */ | ||
| constructor( editor, attributeKey ) { |
There was a problem hiding this comment.
I'm not sure if we need this param for the attributeKey. Let's make it inline as in the Link command.
There was a problem hiding this comment.
My comment depends on the decision from this comment: #9074 (comment)
| const doc = model.document; | ||
| const selection = doc.selection; | ||
|
|
||
| const value = languageCode ? parseLanguageToString( languageCode, textDirection ) : false; |
There was a problem hiding this comment.
This is more like stringifyLanguageAttribute, not parseLanguageToString.
| * @param {String|Boolean} [options.languageCode] Language code to be applied to the model. | ||
| * @param {String} [options.textDirection] Language text direction. | ||
| */ | ||
| execute( { languageCode, textDirection } = {} ) { |
There was a problem hiding this comment.
This class looks like a copy-paste from basic-styles AttributeCommand, maybe a better way would be to expose AttributeCommand class from some utils to be reused in basic-styles and in here?
There was a problem hiding this comment.
Maybe just expose _getValueFromFirstAllowedNode() and a part of execute logic as utils helpers, used in both language and AttributeCommand?
There was a problem hiding this comment.
Or maybe we could provide those helpers as the model methods similar to insertContent and deleteContent?
There was a problem hiding this comment.
Also, we have findAttributeRange exported from typing package and it's used in some places, maybe typing should provide more such helpers?
There was a problem hiding this comment.
TBH, IDK what findAttributeRange() does in the typing package as it's not very related... I'm afraid whatever's needed in the LanguageCommand doesn't make much sense there too.
Do you have an idea for a better place?
Also, a little copy-paste here is not a big deal. So, if we can reduce the current ~100% duplication by creating 1-2 reasonable utils and putting them in the core or engine packages, then I'd go for that.
There was a problem hiding this comment.
I think that for now we could go with the copy-paste, but someday we will refactor it to extract some common code fragments to the model and selection methods: #9107
| return; | ||
| } | ||
|
|
||
| const { languageCode, textDirection } = parseLanguageFromString( attributeValue ); |
There was a problem hiding this comment.
Maybe a better name would be parseLanguageAttribute?
| const t = editor.t; | ||
|
|
||
| return editor.config.get( 'languageList.options' ).map( option => { | ||
| option.title = t( option.title ); |
There was a problem hiding this comment.
I'm not sure if this would work as expected.
There was a problem hiding this comment.
It will not. t() requires a string in order to replace it with a correct translation.
There was a problem hiding this comment.
Also, I explained it in another comment that perhaps we don't need this translated: https://github.com/ckeditor/ckeditor5/pull/9074#discussion_r580850365.
| /** | ||
| * Allows to use different language for the editor UI. | ||
| * | ||
| * The language codes are defined in the [ISO 639-1](https://en.wikipedia.org/wiki/ISO_639-1) standard. | ||
| * | ||
| * @member {String} module:core/editor/editorconfig~LanguageConfig#ui | ||
| */ | ||
|
|
||
| /** | ||
| * Allows to use different language of the editor content. | ||
| * | ||
| * The language codes are defined in the [ISO 639-1](https://en.wikipedia.org/wiki/ISO_639-1) standard. | ||
| * | ||
| * @member {String} module:core/editor/editorconfig~LanguageConfig#content |
There was a problem hiding this comment.
textFragmentLanguage is also supporting ISO 639-2 (3 letters) codes. I'm wondering if we should also extend language.content and language.ui with ISO 639-2 or rather remove this information from textFragmentLanguage and treat ISO 639-2 only as deprecated, compatibility support? I have a feeling that it would be nicer to limit supported codes only to ISO 639-1 as they are more common.
There was a problem hiding this comment.
I've simplified it to ISO 639-1. It's still possible to use other ISO formats since it's just a string that won't conflict with the current implementation, but using a single format makes our life a bit easier.
| } | ||
|
|
||
| /** | ||
| * The available {@link module:language/textfragmentlanguage} options allowing setting language of parts of the content. |
There was a problem hiding this comment.
I went with language.textFragmentLanguage option name to match the plugin name, but it would be probably nicer to simply name it language.textFragment to avoid duplication. WDYT?
There was a problem hiding this comment.
| * The available {@link module:language/textfragmentlanguage} options allowing setting language of parts of the content. | |
| * The available {@link module:language/textfragmentlanguage~TextFragmentLanguage | |
| } options allowing setting language of parts of the content. |
There was a problem hiding this comment.
After a second thought, it probably makes a bit more sense to keep a longer format, so the plugin name. Otherwise, it will be hard to tell what feature is actually introducing this option which may affect discoverability.
| @@ -0,0 +1,3 @@ | |||
| { | |||
| "Language": "Toolbar button tooltip for the Text Fragment Language feature." | |||
There was a problem hiding this comment.
We should describe contexts for all translatable strings.
| * Refer to [WCAG 3.1.2 Language of Parts](https://www.w3.org/TR/UNDERSTANDING-WCAG20/meaning-other-lang-id.html) specification | ||
| * to learn more. | ||
| * | ||
| * To change UI editor language, refer to [Setting the UI language](https://ckeditor.com/docs/ckeditor5/latest/features/ui-language.html) |
There was a problem hiding this comment.
I think this should use glink here
| } | ||
|
|
||
| /** | ||
| * The available {@link module:language/textfragmentlanguage} options allowing setting language of parts of the content. |
There was a problem hiding this comment.
| * The available {@link module:language/textfragmentlanguage} options allowing setting language of parts of the content. | |
| * The available {@link module:language/textfragmentlanguage~TextFragmentLanguage | |
| } options allowing setting language of parts of the content. |
| * Refer to [WCAG 3.1.2 Language of Parts](https://www.w3.org/TR/UNDERSTANDING-WCAG20/meaning-other-lang-id.html) specification | ||
| * to learn more. | ||
| * | ||
| * To change UI editor languge, refer to [Setting the UI language](https://ckeditor.com/docs/ckeditor5/latest/features/ui-language.html) |
There was a problem hiding this comment.
We should use glink here.
| /** | ||
| * The text fragment language UI plugin. | ||
| * | ||
| * It introduces the `'language'` button. |
There was a problem hiding this comment.
It's introducing a dropdown, not a button.
| import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; | ||
| import TextFragmentLanguageEditing from '../src/textfragmentlanguageediting'; | ||
| import TextFragmentLanguageCommand from '../src/textfragmentlanguagecommand'; | ||
| import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor'; | ||
| import { getData as getModelData, setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; | ||
| import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view'; |
There was a problem hiding this comment.
Please don't mix relative imports with package imports.
packages/ckeditor5-language/tests/textfragmentlanguageediting.js
Outdated
Show resolved
Hide resolved
packages/ckeditor5-language/tests/textfragmentlanguageediting.js
Outdated
Show resolved
Hide resolved
| function createCustomConfigEditor( languageConfig ) { | ||
| return VirtualTestEditor | ||
| .create( { | ||
| plugins: [ TextFragmentLanguageEditing, Paragraph ], | ||
| language: languageConfig | ||
| } ) | ||
| .then( newEditor => { | ||
| editor = newEditor; | ||
| model = editor.model; | ||
| } ); | ||
| } | ||
| } ); |
There was a problem hiding this comment.
This would override the editor created before and both the editors would not be destroyed properly.
| import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor'; | ||
| import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; | ||
| import TextFragmentLanguageEditing from '../src/textfragmentlanguageediting'; | ||
| import TextFragmentLanguageUI from '../src/textfragmentlanguageui'; | ||
| import DropdownView from '@ckeditor/ckeditor5-ui/src/dropdown/dropdownview'; | ||
| import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; | ||
| import { setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; |
There was a problem hiding this comment.
Please don't mix imports.
|
CI fails. |
| /** | ||
| * @extends module:core/plugin~Plugin | ||
| */ | ||
| export default class TextFragmentLanguage extends Plugin { |
| * @member {Array.<module:language/textfragmentlanguage~TextFragmentLanguageOption>} | ||
| * module:core/editor/editorconfig~LanguageConfig#textFragmentLanguage | ||
| */ |
There was a problem hiding this comment.
We should note here that this option is not available by default and the TextFragmentLanguage plugin must be added.
| let spy; | ||
|
|
||
| beforeEach( () => { | ||
| spy = sinon.spy(); |
There was a problem hiding this comment.
We should init sinon sandbox before using it (testUtils.createSinonSandbox();)
| function getListViewItems( listView ) { | ||
| // Let's drop separator. | ||
| return listView.items.filter( item => item.children ); | ||
| } |
There was a problem hiding this comment.
We should define this helper inside a describe block.
|
The PR is ready for testing, @FilipTokarski @LukaszGudel, could you please take a look at it? |
Co-authored-by: Kuba Niegowski <[email protected]>
|
I've renamed the feature to "text part language". It shouldn't break anything (especially that tests passed), but worth pulling the latest changes @FilipTokarski @LukaszGudel |
|
There's a little mismatch between content from CKE4 and CKE5. If you create a "nested" text, for example like <p><span dir="rtl" lang="he">hebrew <span dir="ltr" lang="fr">french</span> hebrew again</span></p>CKE5 creates three separate spans: <p><span lang="he" dir="rtl">hebrew </span><span lang="fr" dir="ltr">french</span><span lang="he" dir="rtl"> hebrew again</span></p>If you now want to get content from 4 and put it inside 5, the inner span will be removed and the whole text remains in Hebrew: <p><span lang="he" dir="rtl">hebrew french hebrew again</span></p> |
|
After testing this PR it's looking good to me. Other than the issue reported by Filip I could not find anything buggy related to those changes. BTW, could it be also related to #8921? There are few issues related to RTL languages or mixing LTR with RTL in one paragraph but same issues can be reproduced in the browser without CKE5, so I guess those are not related to the editor itself. |
|
@FilipTokarski as @LukaszGudel already wrote the issue with this HTML is caused by #8921. I've already checked it (using some dirty fix) and it should be fixed with #8921. Basically, element nodes in ckeditor5/packages/ckeditor5-engine/src/conversion/upcasthelpers.js Lines 894 to 904 in d63ec7c lang attribute as there is no check if the attribute has already been set.
|
|
I didn't find any other issues, looks ok 👍 |
|
Just for future reference, the main changelog entry should not be about adding the dropdown for the feature but about adding the feature itself. Now it sounds like the feature has already existed and we've just added a dropdown that was missing. |
Suggested merge commit message (convention)
Feature (language): Added language dropdown button to support text part language. Closes #8989.
Other (utils): Added
language.getLanguageDirectionhelper function allowing to determine text direction based on language code.Internal (locale): Language text direction detection moved to the
languageutils module.