Add ability to indicate the language of styles by KarolDawidziuk · Pull Request #5474 · ckeditor/ckeditor4

KarolDawidziuk · 2023-05-04T06:38:18Z

What is the purpose of this pull request?

New feature

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

Unit tests
Manual tests

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.

PR is consistent with the code style guide

What is the proposed changelog entry for this pull request?

* [#5410](https://github.com/ckeditor/ckeditor4/issues/5410): Added the ability to indicate the language of styles via [config.styleSet config variable](https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_config.html#cfg-stylesSet).

What changes did you make?

PR adds the ability to set the language of the Styles dropdown elements. When the language is not explicitly set, the same language as the editor will be added.

Which issues does your PR resolve?

Closes #5410.

Comandeer

The overall logic looks good, there is only a small issue with outputting the [lang] attribute – see my inline comments for more details.

Comandeer · 2023-05-08T10:03:14Z

tests/plugins/stylescombo/langattribute.js

+		} );
+	},
+
+	'test adds the default editor lang code to the Styles combo box item if it is not defined': function() {


This test fails in my browser:

Values should be the same. Expected: en (string) Actual: en-gb (string)

It would probably be better to set the language to one with a low probability of usage in our team and one different than config.defaultLanguage – that's how we will have the certainty that the fallback really works.

Comandeer · 2023-05-08T10:03:24Z

tests/plugins/stylescombo/langattribute.js

+		bender.editorBot.create( {
+			name: 'cb_2',
+			config: {
+				lang: 'en',


Suggested change

lang: 'en',

language: 'en',

Comandeer · 2023-05-08T10:17:18Z

tests/plugins/stylescombo/manual/langattribute.html

+				resultElement.setHtml( '' );
+
+				CKEDITOR.tools.array.forEach( panelItems, function( element, index ) {
+					resultElement.appendHtml( 'Item ' + index + ': ' + JSON.stringify( element.getAttributes() ) + '</br></br>' );


The dump of all attributes in the JSON format is quite unreadable. Maybe just display the value of the [lang] attribute?

Sure. Changed.

Comandeer · 2023-05-08T10:26:45Z

plugins/listblock/plugin.js

 					' draggable="false"' +
 					' ondragstart="return false;"' + // Draggable attribute is buggy on Firefox.
 					' href="javascript:void(\'{val}\')" ' +
+					' lang="{language}"' +


Adding the language directly in the listblock template results in outputting this attribute in each place that uses the listblock – so if the language is not set, the [lang="{language}"] will be generated (you can see it e.g. in Font dropdown).

Comandeer · 2023-05-08T10:27:48Z

plugins/listblock/plugin.js

+					if ( language ) {
+						data.language = language;
+					}


Probably the logic that fallbacks to the editor's language should be moved here to always output the correct [lang] attribute in various toolbar buttons.

KarolDawidziuk

Thank you @Comandeer for the review!

I've applied the changes mentioned in your inline comment and PR is ready for another review.

KarolDawidziuk · 2023-05-15T07:34:35Z

tests/plugins/stylescombo/manual/langattribute.html

+				resultElement.setHtml( '' );
+
+				CKEDITOR.tools.array.forEach( panelItems, function( element, index ) {
+					resultElement.appendHtml( 'Item ' + index + ': ' + JSON.stringify( element.getAttributes() ) + '</br></br>' );


Sure. Changed.

Comandeer

@KarolDawidziuk, thx for your changes! However, I'm wondering if we couldn't simplify our approach (see my inline comments for more details).

Comandeer · 2023-05-16T15:25:17Z

plugins/listblock/plugin.js

 			proto: {
-				add: function( value, html, title ) {
-					var id = CKEDITOR.tools.getNextId();
+				add: function( value, html, title, language, editor ) {


Five parameters are probably too many.

I'm wondering if we could replace the added ones with the options object pattern:

Suggested change

add: function( value, html, title, language, editor ) {

add: function( value, html, title, options ) {

options = CKEDITOR.object.merge( someDefaultOptions, options );

The second idea I had: maybe we could pass the editor to the list block constructor and save it as this._.editor? However, I'm not sure if a list block instance can't be reused in another editor 🤔

I see even two more solutions, probably even easier ones:

pass only possible language fallbacks (so basically editor.langCode) instead of the whole editor,

add the [lang] attribute only if it's known; it's absence should make the screen reader use the nearest available value – so the editor's container [lang] attribute.

Thanks for the ideas 👍
Let's go with applying the lang attribute when it's defined.

Comandeer · 2023-05-16T15:34:11Z

plugins/listblock/plugin.js

+						editorLanguage = editor.config.language ||
+							editor.config.defaultLanguage ||
+							editor.langCode;


Do we need so many options? Couldn't we use just editor.langCode? It should contain the actual language of the editor.

Removed as we changed our approach.

Comandeer · 2023-05-16T15:35:49Z

plugins/listblock/plugin.js

 						title: CKEDITOR.tools.htmlEncodeAttr( title || value ),
-						text: html || value
+						text: html || value,
+						language: language || editorLanguage


If we go with the "add the [lang] attribute only if it's known" approach, then we could add the whole markup here (similar to what we do in the button plugin).

Comandeer · 2023-05-16T15:43:37Z

tests/plugins/stylescombo/manual/langattribute.md

+* Item 0 should have the lang attribute set to `pl`.
+* Item 1 should have the `lang` attribute set to the editor language value.


Suggested change

* Item 0 should have the lang attribute set to `pl`.

* Item 1 should have the `lang` attribute set to the editor language value.

* Item 1 should have the lang attribute set to `pl`.

* Item 2 should have the `lang` attribute set to the editor language value.

Additionally, in my case the editor is en-gb but the second item is en 🤔 I don't know why as the code suggests they should be the same.

Fixed. I added lang instead of language in the editor config.

…nd update tests

KarolDawidziuk · 2023-05-26T11:20:10Z

Thanks @Comandeer for the review! It's ready for another one.

Comandeer

LGTM! I've just updated the manual test description as it became outdated.

Comandeer self-assigned this May 8, 2023

Comandeer requested changes May 8, 2023

View reviewed changes

KarolDawidziuk added 5 commits May 15, 2023 09:17

Add ability to indicate the language of styles

2c68449

Add manual test

c80beaa

Add unit tests

f35b09e

Handle applying default language to listblock items

d4c0d1c

Update tests

9f3bcb9

KarolDawidziuk force-pushed the t/5410 branch from 604c1e4 to 9f3bcb9 Compare May 15, 2023 10:11

KarolDawidziuk commented May 15, 2023

View reviewed changes

KarolDawidziuk requested a review from Comandeer May 15, 2023 10:13

Comandeer requested changes May 16, 2023

View reviewed changes

Apply lang attribute to the Style combo box items when it's defined a…

1d54480

…nd update tests

KarolDawidziuk requested a review from Comandeer May 26, 2023 11:20

Comandeer added 2 commits May 30, 2023 17:45

Update manual test description.

8d89c11

Add changelog entry.

c0ce7a0

Comandeer approved these changes May 30, 2023

View reviewed changes

Comandeer merged commit 4ec334e into master May 30, 2023

Comandeer deleted the t/5410 branch May 30, 2023 15:49

-				add: function( value, html, title, language, editor ) {
+				add: function( value, html, title, options ) {
+					options = CKEDITOR.object.merge( someDefaultOptions, options );

		* Item 0 should have the lang attribute set to `pl`.
		* Item 1 should have the `lang` attribute set to the editor language value.

Conversation

KarolDawidziuk commented May 4, 2023

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

Did you follow the CKEditor 4 code style guide?

What is the proposed changelog entry for this pull request?

What changes did you make?

Which issues does your PR resolve?

Uh oh!

Comandeer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KarolDawidziuk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Comandeer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KarolDawidziuk commented May 26, 2023

Uh oh!

Comandeer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants