Language of parts: Introduce a language toolbar button

This is a very good idea. We should be able to map it into the internationalization settings such that it is only visible if Drupal knows it is a multi-lingual site.

Generally, all languages from the standard_language_list() should be available from the dropdown. Selection shouldn't be restricted to enabled languages nor to a small subset defined by the admin.
We could supply a shortlist though that is listed on top and this should be configurable by the admin.
The button would add a span around the selected text.
Not sure what it should do when no text is selected - add the language tag to the enclosing container? Or should it be disabled then?
In any case, the button should display the language the selected text or enclosing container is currently set to.

Besides the separate dropdown, we should find a special solution for the blockquote button, which should be the most common situation of a different language of parts. Not sure how we can implement that. A dialog box would inacceptably complicate the use. If a combined button/dropdown widget exists, that would be a possibility. But we should evaluate other options.

Assigning to Gábor Hojtsy to gather feedback on how important this is.

My initial reaction is: yes, it should be possible to enable this on a Drupal core installation. But not by default — we don't want to complicate the default experience.

Agreed with Wim.

.

This is truly excellent. :)

As to the language settings, i.e., which languages are shown as markup options, I think there should be a user interface for setting these languages either to a more encompassing list (default?) or a shorter list of languages likely to be needed.
We want to make creating accessible content as easy as possible, and wading through a bunch of languages that will never be used counters that.

Apart from that, the draft plugin will need rewriting in this aspect of defining the language dropdown, which currently is created in a loop within the plugin.js that the drag and drop interface of the CK module in D7 cannot interpret. This was touched upon in another issue concerning display & use of buttons in the drag and drop interface.

Also, I really like the way typo3 defines the use cases, adding the language attribute to the next enclosing element from cursor position (no highlighting) or multiple elements if so highlighted.
However, even the more rudimentary adding in of a span as the plugin is written now is enormously beneficial, since in my experience the most common things to mark up languages on are single words and phrases.

duplicate post

This seems independent from actual language settings, given that one can add any language, including your own. It seems like this could best live at the settings level of CKEditor (formats/basic_html).

#11: I think you misunderstood. :) This is about specifying language of parts of content. It affects the CKEditor UI visible to the end user.

@WimLeers I did understood it as that. It was my understanding, this was an option - not provided by default but selectable in CKEditor configuration.

Oh, yes! It would be optional. It was I who understood you wrong, then :)

@ Hanno
I like the idea of using the list of available languages in Drupal (shown in admin/config/regional/language/add) as the set of default values the dropdown features.

However, what kind of foreign language elements typically appear in a website's texts has nothing to do with internationalization or multilinguality of the site itself, and it would thus be overstretching that infrastructure to try and use it to set details of the Editor's appearance.

Instead, I'd say it is rather much more dependent on the kind of topic/subject matter of the site.

Some examples:
A very interesting topic just cropped up on the WebAim accessibility list on "exotic" languages that have no written form (in this case Kambaata, a language spoken in Ethiopia) and for which there is no text-to-speech engine. Kind of a parallel situation to your example of old latin and greek, here a website-specific customization must be available.
Similarly, an English language website discussing France is likely to have more French expressions, while a German language website dealing with inclusion in education will, by virtue of the fact that the most research on this has been undertaken in the English language, have more English language terms in it, which ideally for ease of use should be topmost in the respective dropdowns.
Another website however may want the whole list of languages since it covers a very wide geographical range of subject matter (a site focusing on international comparisons of something springs to mind in example).

Thus, IMHO the ideal solution for this would be to make the complete Drupal list available by default, and add an input field for a linebreak separated custom list of languages in the CKE configuration interface when the addon is enabled. I can imagine the best way to go about defining languages would be in pairs, parallel to the method for creating select lists in general, i.e. with entries in the format: en|English.
The only problem I see remaining here is that this list also should be internationalizable on multilingual sites. Could it work if the option definition went like so: en|t('English')??

This small issue apart, a link to an index of of language abbreviation codes for reference would also be useful in the field's description text.

Also, a check box to enable/disable the inclusion of the complete language list below the custom list or not (enabled by default) would allow further flexibility in adapting the dropdown to the content producers' real specific needs.

@Bojhan
I think this suggested setup is independent of whether D8 comes with the language dropdown enabled or disabled by default.
That said, personally I would tend towards enabling it by default, if only to set a sign of how Drupal prioritizes accessibility concerns. It is then the site builder's responsibility and conscious decision to remove this feature from the CKEditor frontend interface depending on the individual site's needs.

Edit:
@ Hanno
haha you sneaky, posting while I was writing ;)

Actually you have a point there, especially regarding fields... language lists needed to fill out content in that way indeed could be identical to the one needed in the CKE.

I also agree that end users shouldn't have access to customization, but that mustn't be the case anyways as far as CKE is concerned?

End users would not have acces to this. Its in configuration. It is still my understanding, that this is somewhat but not directly related to actual multilingual settings. I have asked Gábor to chime in as it seems like we are reinventing the wheel here.

@calliandra I do not think that exposing this by default will make any sense, its a very particular piece of functionality. You need to know exactly what to use it for, if we enable it by default I quickly see people confusing it with actual multilingual settings. I do not see the need to introduce something this complex, by default for everyone. If one needs it, we have a quick and easy way to set it up. I think, our continuous efforts on the accessibility front of Drupal should be enough to promote that we care.

This looks *very* similar to #1314250: Allow filtering/configuration of which languages apply to what (UI, nodes, files, etc) where we were discussing making a matrix of "situations" and languages to map to each other. It is not just this case where certain languages may only apply to certain cases on the site I think.

#18 looks great, but the capability for an author to add a tag from any language should be possible as well, as you may not always be able to predict what languages an author will include in the future.

Having read this again, I think this belongs in contrib. We have field-level translation (and hence language indicators), this is about adding intra-field language indicators. It's definitely not something the majority of content creators need.
If this is an inaccurate assessment, let me know.

Hanno, calliandra, Mark123, Pancho: please start developing a Drupal 8 module called "ckeditor_lang" or something like that to tightly integrate this with both CKEditor and Drupal 8's improved i18n functionality. That way, you also help with validating the APIs that editor.module & ckeditor.module provide.

Moodle added

Linkfix WCAG docs

More helpful link

latest version of an even more helpful document :)

Having read this again, I think this belongs in contrib. We have field-level translation (and hence language indicators), this is about adding intra-field language indicators. It's definitely not something the majority of content creators need.
If this is an inaccurate assessment, let me know.

I both agree, and disagree. Certainly, this isn't something most content creators need. But it's like most accessibility aspects: most people don't need it, but some need it very much. And Drupal needs to pick any low-hanging fruit that make Drupal core rise a level higher on WCAG 2.0 and ATAG 2.0 accessibility levels.

About optional or by default: if we want to comply for ATAG2.0 level AA, we should include this button by default, as ATAG requires to have accessiblity helpers available by default. But I realise this makes it more complex. Most important for Drupal 8 is to have this button included, optionally or not. We could always add this button by default in Drupal 9.

Confirm that for AA level of conformity, ATAG 2.0 B.2.1/2.2 require WCAG 2.0 conforming content to be made possible to the individual author, not the admin. And WCAG 2.0 3.1.2 requires that languages of parts may be accordingly tagged.

Now the big question is: is this the last remaining prerequisite that we need for D8 in order to achieve AA level? If yes, I would say that this is an important, major task. If not, it depends.
As long as there is a realistic chance for D8 to pass WCAG/ATAG with an AA level by D8 release, then we should keep working in a core issue. If that's unrealistic, we should create a contrib module instead, that might be included in D8 core at a later point, if that allows us to get Drupal 8 certified by WCAG/ATAG level AA.

Until this research has been done, I'm tentatively reopening this issue. We shouldn't start work, though.

Emm, no patch, of course.

Also note that we have stated AA level on WCAG 2.0 and ATAG 2.0 as our goal.
However, that isn't identical with the goal being realistic to meet in D8 by the release date, or at a later point.
We're having a few tickets for specific subsystems or aspects. But what we'd need is a meta issue that systematically tracks the remaining subsystems and aspects. This is going to be a quite huge work, but seems necessary in order to evaluate how much we want to push forward this and other aspects.

I created #2034915: [meta] Track progress in WCAG 2.0 conformance and #2034909: [meta] Track progress in ATAG 2.0 conformance.

This is a huge task but not quite as bad as it may look like.
Finally many criteria are AAA-only or don't apply to us, or are already met by D8 core.
We need to check every single criterium and figure out what's missing or what needs to be discussed.

Some help is very appreciated, but please don't just mechanically add sub-tasks which would just clutter our issue queue and end up being unmanageable. (Note that I'm mentioning this for less experienced Drupalistas...)

For CKEditor, this feature has moved from Milestone CKEditor 4.2 to CKEditor 4.3.

@Wim Leers is there anything that would make this difficult to do in Contrib?

In D8, I assume that we'll still be needing to enable other Contrib i18n modules to make a site effectively multi-lingual. I'm wondering if this is something that could be brought into a sub-module which could be optionally enabled.

I'm definitely eager to have better support for this part of WCAG in Core, but we're relying on CKEditor & internationalization has always required some Contrib modules.

@Hanno:
Yes, our vendored libraries are strictly versioned.
But given that the current CKEditor releases to a great part adapt to the needs and wishes of Drupal, I'm quite certain that we will release with the latest version of CKEDitor.
For CKEditor 4.2, which is due in a week, there's already a ticket: #2036253: Update CKEditor library to 4.2
And CKEditor 4.3 is scheduled to follow in 6 weeks already, see CKEditor Roadmap. So this shouldn't be a problem, IMHO. We just should make them know that we want it, and help testing & improving.

I think that CKEditor should be responsible of providing the button, as well as the language list. However, our current APIs allow us to get immediately a list of all languages, but I'm not sure how hard it could be passing it to CKEditor, if needed. Anyway, as soon as CKEditor provides this feature, it should be included in core and it should be enabled by default.

In any case, I think that the "Let's see how far from WCAG/ATAG compliance we are, then decide if we should work on this in core or in contrib" makes no sense to me. This is something that can help us with WCAG/ATAG compliance, so core should be shipped with it. It is important to know how far we are from our stated goals in terms of accessibility, but this is not a reason for not working on something in core (IMO).

We can't enable it by default. I still stand by my initial response in #5 of two months ago:

My initial reaction is: yes, it should be possible to enable this on a Drupal core installation. But not by default — we don't want to complicate the default experience.

Most sites are only in one language. Unless usability testing proves that this does not confuse users, we should not enable this by default.

Most sites are only in one language.

Remember this is not just about multilingual sites. Even in monolingual sites, citations of foreign-language content are used or may be used by authors. No admin will be able to tell that this won't be the case for their website, as it's entirely up to the content authors.

But let's first see how it's implemented upstream and then add some UI improvements, and let's then decide if it's still confusing or not. It agree it might end up being still confusing to regular users.

I agree with @Pancho.

But (this is only a curiosity of mine) how could exposing an extra button (or any other widget to implement this feature) in the toolbar be confusing for content authors?

In any case, I think that we should work closely with the CKEditor guys to ensure that this feature gets implemented in an "usable" way...

@falcon03 It's a gross generalization, but I think from a UX perspective the fewer elements there are on a page for a user to sift through, the more usable it is.

We could simply check if i18n is enabled, as an English site might well want to include a small phrase in french as part of a quote.

What about if the button were enabled by default it could just be disabled by administering CKeditor here admin/config/content/formats/manage/full_html

At the very least the new CKEditor button should be available there though.

I think it would be great to focus on getting the feature in at all instead of getting stuck arguing about the default enabledness.

Agree that 'getting it in' somehow should be our focus, and it seems we pretty much agree on that.

As I said on #2034909-2: [meta] Track progress in ATAG 2.0 conformance, WCAG compliance seems possible, even if much remains to be done.
However, even A-level ATAG compliance seems to be absolutely out of reach, at least by the D8 release date.
By that I'm not saying that we shouldn't try to be as much ATAG-compliant as possible. I'm just saying that ATAG-compliance at the moment isn't a 'no matter what' criterion, just a major one among others.

Think the slight over-discussion is due to the plugin yet to be finished upstream.
We should probably postpone this issue on #2039163: Update CKEditor library to 4.4, and let our JS/CKEditor cracks help the CKEditor guys with Introduce Language toolbar button, while all others (including me) would find lots more things to do in the issue queue, including #1314250: Allow filtering/configuration of which languages apply to what (UI, nodes, files, etc) which would be of great help for us here.

Hello all,

I think the problem here is that we are actually talking about 2 things within the context of Drupal, and though the original ticket description focuses on the editor button, the integration part would mean a much more encompassing approach to the matter.

1. foregrounded of course is including the language button in the CKE Editor by default.
   I agree this is something that we can wait to be done upstream as far as the basic inclusion is concerned.
   The question of current content author needs within this context is not, to my mind, an argument:
   • Today, most content authors are not even aware of the confusion (or even inability to understand content at all because the screen reader mispronounces "foreign" words so badly) that can be created for screen reader users if the language of parts is not marked up appropriately. This consciousness is something that still needs to evolve, just like teaching content authors that images don't need to be put in a table to be aligned right... So, similarly to making "left" "center" "right" positioning of images (via CSS in the background) easier to achieve than former table misuse for positioning, this is something that needs to evolve multidimensionally: on the one hand, developers must make the appropriate mechanisms easily available, while on the other, end user consultancy must educate the public as to their usage.
   • Also, if I look at the buttons enabled by default in the full HTML version of the Editor today, there are so many buttons that most will never "need" and it is up to the site builder to enable only those relevant to each project.
     And yes, along with others here I would like to stress that this button is NOT for multilingual sites but rather caters to the internationalization of language in general, as foreign terms have become part of everyday language! Thus, where there is a consciousness for this aspect, this is definitely a must-have!
2. The other side of this issue is integrating a Drupalwide mechanism by which the language of single elements of a node can be defined. Think node fields.
   In example: if I have a node type of "books" which I am using to display a bibliography type of information set, the nodes themselves could be in the site language (i.e., descriptions, reviews etc.), but the book title could be in a different language.
   AFAIK, to now we can only mark the whole node's language (though in D7 the database tables already include a table field for the language of each data item). The dropdown that is displayed for this includes the different languages of a multilingual site.
   What I understood Hanno to be visualizing in #18 is that for these instances we not (only) enable the (optional/default) pulling in of Drupal's complete languages list but that we also have the possibility to define further languages (or a reduced language set) that could possibly be used within a project's context due to subject matter or specific jargon (=increased usability by reducing the amount of options). This then could be used for things specifically Drupal (i.e. defining single field content language) as well as for the language button in the editor.

Thus, I am not at all sure the issue name change is appropriate, since the work to be done in Drupal IMO mainly concerns the second point!

p.s. @ Wim: I'd love to be able to help develop this, but while hope is the last to die I am as yet still very much learning Drupal, so feel a bit useless on the hands-on part of this for now. :'(

Re #36:
ad 1a) Agree with all of this and have always agreed, see #23 #31, #1314250-27. So whether an admin would deem this important or not, should not be our guideline.
However, while there are simple and completely intuitive buttons, this one might be less so, and will certainly be more complex. So for our default setup we need to take into consideration how much the language button regredes simplicity at first glance.

ad 2) Yes, there's more to do for language of parts than just this button, and there is #1323338: [meta] Conform to WCAG Success Criterion 3.1.2: Language of Parts to keep track of all aspects. This discussion however has been focussed to the language button from the very beginning and should remain so.
Feel free to open a new issue for every aspect that hasn't been covered yet! Otherwise you will certainly find a sub-issue of that meta-issue where we can push things forward right now, without relying on upstream.

Re #37:
I'm fine with reopening this issue, if you prefer. I just wanted to limit further discussion to the necessary, until the button really is available from upstream.

#2039163: Update CKEditor library to 4.4 landed, so this is unblocked.

Can you please update the issue summary with your exact goals? It's unclear to me right now what exactly you want to get into D8 core. Thanks :)

@Wim I think it's just a matter of adding in this plugin http://ckeditor.com/addon/language

I'd like to see it appear on the list of available buttons here /admin/config/content/formats/manage/basic_html

However, it's a big enough change I'm not sure we want it to appear elsewhere. Maybe in /admin/config/content/formats/manage/full_html

I haven't added plugins to ckeditor for years. It is easy to drop the module in core/modules/ckeditor/js/plugins/language

But I wasn't sure where to adjust the CKEditor config. Between CKEditor's documentation for adding plugins and Drupal 8's documentation, something seems missing.

#43.4: this is my biggest concern. How could you ever know what the limited set of languages is that users will use? We don't even have a complete list of languages in Drupal core.

#43.5: Are you asking the Languages dropdown to be enabled by default for some text formats?

Adding D8MI topic tags.

#48.5: Agreed, also as Gábor said earlier: let's not waste time over discussing whether this should be enabled by default or not.

#48.4: thanks, that helps

Finally: note that this is a simple feature addition that doesn't break anything, so we could easily add this in later Drupal 8 releases, e.g. 8.1, if we don't get it done before 8.0.

At the sprints today, I'll have a look if I can roll-up a patch.

Here's a first stab. It brings in the language button in the interface. The javascript (ckeditor.js) needs to be updated to make it actual work, so we need a new build of Ckeditor. Wim's about to help me with this when he arrived at the sprint. Regarding the language dropdown; I'd say add the complete language list that Drupal outputs at admin/config/regional/language/add.

If this is in, we can improve it by adding a separator between the enabled and disabled languages.

Here's a working patch, with a rebuilded Ckeditor. If #2345961: Update CKEditor library to 4.4.5 is in, this patch will be much smaller.
This patch does the following:

- Adds the language button to the toolbar
- Adds the language list of all languages returned by LanguageManager::getStandardLanguageList()
- Adds RTL support if needed

Not done yet, or to change/improve later: make the list configurable.
I've split the patch up in two different ones: one that includes the updated ckeditor library, and one with only the plugin.

Now that #2345961: Update CKEditor library to 4.4.5 is in, here's patch for purely the new button. Thanks @ Wim Leers for the help!

And now with free updated tests too!

After adding the button to the toolbar, this works great! I just loaded it up on SimplyTest.me and it worked as expected.

Ah, forgot one test. Try again, test bot.

Looks great. Looked over the code. Got an instance up on SimplyTest.me. This is a great addition for both internationalization & accessibility.

Just added a little improvement, to show the language too. See screenshot.

This is not enabled by default, still. Correct?

@Bojhan: that's correct. It's available to be dragged into the default button bar.

So, can we get it back to RTBC?

I like the improvement. I've got the patch applied on SimplyTest.me for the next few hours.

Sorry, but this one is not entirely ready yet.

1. +++ b/core/modules/ckeditor/css/plugins/language/ckeditor.language.css
   @@ -0,0 +1,20 @@
   +  content: " ("attr(lang)")";
   

   Extraneous space?

2. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Language.php
   @@ -0,0 +1,88 @@
   + *   label = @Translation("Set language")
   

   "Set language" is a strange name. Let's change this to just "Language".

3. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Language.php
   @@ -0,0 +1,88 @@
   +  public function getConfig(Editor $editor) {
   +    $language_list = array();
   +    $predefined_languages = LanguageManager::getStandardLanguageList();
   +    foreach ($predefined_languages as $langcode => $language) {
   +      $english_name = $language[0];
   +      $direction = empty($language[2]) ? NULL : $language[2];
   +      if ($direction === LanguageInterface::DIRECTION_RTL) {
   +        $language_list[$english_name] = $langcode . ':' . $english_name . ':rtl';
   +      }
   +      else {
   +        $language_list[$english_name] = $langcode . ':' . $english_name;
   +      }
   +    }
   +    // Sort on full language name.
   +    ksort($language_list);
   +    $config = array(
   +      'language_list' => array_values($language_list),
   +    );
   +    return $config;
   +  }
   

   We want test coverage for this.

4. +++ b/core/modules/ckeditor/src/Tests/CKEditorTest.php
   @@ -430,7 +432,7 @@ protected function getDefaultContentsCssConfig() {
   diff --git a/core/modules/user/interdiff-59-62.txt b/core/modules/user/interdiff-59-62.txt
   
   diff --git a/core/modules/user/interdiff-59-62.txt b/core/modules/user/interdiff-59-62.txt
   new file mode 100644
   
   new file mode 100644
   index 0000000..e69de29
   

   We don't want to commit an interdiff :)

1. The space is deliberate. We need it to separate the language code from the string.
2. Changed!
3. I have no experience with writing tests. Can you explain which tests you'd like to have so I can write them this week?
4. Fixed

With some amazing mentoring from Xano I managed to write my first UnitTest. So here goes (once more, Testbot)!

Try again testbot.

@Hanno; we don't need to test that. The AdminTest is to test if the admin interface works (CKeditor settings). We don't need to test adding of the button, we only need to test the piece of code that Wim highlighted in #72.

Wonderful work, Baris! And thank you very much, Xano! :)

This is now very close — down to the nitpicks! :)

1. +++ b/core/modules/ckeditor/css/plugins/language/ckeditor.language.css
   @@ -0,0 +1,20 @@
   +html[lang] {
   ...
   +html[lang]:after {
   

   It would be great to explicitly document why these are necessary. It's documented in this issue, but we should document it in the code, for future reference.

2. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Language.php
   @@ -0,0 +1,88 @@
   +use Drupal\ckeditor\CKEditorPluginConfigurableInterface;
   +use Drupal\Component\Utility\NestedArray;
   +use Drupal\Core\Form\FormStateInterface;
   

   These 3 are never used. Should be removed.

3. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Language.php
   @@ -0,0 +1,88 @@
   +        'label' => t('Language button'),
   

   We don't say "Bold button", but "Bold". Similarly, this should just say "Language".

Thanks for the review. Fixed these 3 things too.

Applied fine locally, but I got a warning:
warning: 2 lines add whitespace errors.

It s a binary file so not sure where that comes from.

Hmm, weird. When I use git apply filename I get these same whitespace errors, but also on the other patches (who succeeded on the test bot).
When I use patch -p1 < filename I don't get any errors.

Let's await the testbot.

#83: As Baris indicated, that's nothing to worry about in this case; it's normal for patches that modify CKEditor's JS.

So this is what it looks like:

I'm afraid we have to be honest here… and this is really a feature request, not a task. Unless people can convince core maintainers that it isn't a feature request.
A mitigating factor is that this does not and cannot affect other things unexpectedly.

I think there are two usability problems:

1. the list of languages is overwhelmingly long
2. the way CKEditor deals with <span>s is … apparently painful

@Wim Leers - I don't want to debate whether this is a feature or a task. I'm fine with it either way.

The patch provides an optional button which allows those who want to have a language selection button to easily do so.

It's not perfect, but it's an optional button. If it is a function that an organization wants because it will make it easier for their staff to produce WCAG compliant content it is easy for them to add in. It's definitely not perfect, but for an optional button that isn't enabled by default, it will make it easier for sites to meet WCAG 2.0 3.1.2.

I do think this will help with future versions of CKEditor too, given that having this functionality in a WYSIWYG is an accessibility best practice.

This patch has come a long ways. I don't see any reason not to RTBC it again (either as a feature or a task).

Actually feature requests that go into core now, shouldn't really have important follow ups. Is it possible for us to just solve these issues, and get this in? There is no rush, right?

I discussed this with alex.pott and catch yesterday night. This is fine to go in, because it doesn't affect anything else. So: no worries about that part :)

Of course the <span> thing doesn't need to be fixed in this issue. But we do want to make the language list have better usability, in that it should be approved by Bojhan or yoroy before it gets committed. Precisely for the reasons Bojhan provided in #90: any feature added now should not introduce any technical debt, it should be "done", and therefore follow-ups are not acceptable.

(The <span> thing is a rare exception because it's something that needs to be fixed upstream — I opened https://dev.ckeditor.com/ticket/12520 for that and updated #2350173: [upstream] Improvement of the CKEditor language plugin's <span> handling behavior.)

Great, thanks for the input and opening the CKEditor issue. I've added a config form to select either the the six official languages of the United Nations (english, french, spanish, chinese, russian and arabic) or all languages. I added the list of UN languages to the LanguageManager class, as they might come in handy on other places too.

Also, I expanded the tests with help of Wim Leers.

Wow, this is great! This solution should be fine,

I am just wondering if we need to specifically reference the United Nations here, or if we can just call it a short list. I am always a bit hesitant about referencing specific organisations in Drupal strings. We can just mention, that we based it off the UN list in the code.

@Bojhan; I tend to agree. But what about the name of the function in the LanguageManager? Should I change getUnitedNationsLanguageList() to something like getShortLanguageList()? And would this be appropriate on such a high level in core or should this subset only live in the CKEditor module?

Baris: you want to use the .txt extension for interdiffs, otherwise testbot will try to run them. But regardless of that, there are also actual test failures. So testbot was right in marking this as "needs work".

Nice! Just have to get the tests right then?

EDIT: Oh ya, also need to address specific reference the United Nations. I think the UN would be fine. It's like an ISO standard. Useful to know why those languages are included. But still, maybe there are other better ways to describe this.

@BarisW I think that Wim is better equipped to answer that. It probably make sense for it solely to be a comment reference to the UN.

Updated tests. Bring back that green light!

I've been staring at my tests for hours, couldn't spot the bug. Then Wim Leers helped, and he found it in a few minutes. Argh, frustrating ;) Anyway, here's the fix.

For easier review; here's a patch without the CKEditor changes.

I need some help here, I'm afraid. I have no idea why the test bot fails. When I run the test on my local machine (PHP 5.4 too), the test runs fine with the patch in #106.

Can anyone else have a look what's happening?

Anyone who can jump in to fix the tests? See screenshot; my test runs fine on my machine, so I have no idea on how to debug/fix this.

No longer applies.

Here we go again. The problem stays the same: the tests run fine locally, but the testbot fails. I need some help here.

Hmm, I believe I've found the problem. Let's try again.

Going back to #93 (with screenshots) & #102.

Looks fine in

For the next few hours, you can see it here:

http://s1bd3d36932d061a.s3.simplytest.me/node/1/edit
http://s1bd3d36932d061a.s3.simplytest.me/admin/config/content/formats/ma...

I don't think there's a way to make the whole list of 94 be more user friendly at this stage. Maybe when Chosen or Select2 get in. I'm attaching a screenshot of that too.

The bot likes it and I think it's good to be RTBC at this point.

It was green.... Not sure what would have changed..

I don't get it either. It was definitely green. It's green on my machine. Will have a look during DrupalCamp Gent this weekend.

Cool. Just tagging it.

That's very odd indeed; especially because CKEditorAdminTest hasn't changed since August!

So, the fails are caused by an array comparison of $expected_settings and $editor->getSettings(), where the order of the keys in the array differs. I cannot explain why this doesn't fail on my local machine. However, we can simply overcome this issue by replacing $this->assertIdentical with $this->assertEqual, as suggested by Xano.

Try again, testbot.

In #86, I voiced two concerns:

I think there are two usability problems:

1. the list of languages is overwhelmingly long
2. the way CKEditor deals with <span>s is … apparently painful

• Point 1 was fixed by BarisW in #93. It was approved by Bojhan in #96
• Point 2 is out of scope for this issue, since it's a bug in CKEditor, that will get fixed. I opened the upstream https://dev.ckeditor.com/ticket/12520 ticket for that and created #2350173: [upstream] Improvement of the CKEditor language plugin's <span> handling behavior.

This is looking great now! I manually tested it and can't find anything else to complain about :)

One final code review before this can be RTBC!

1. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
   @@ -353,6 +353,28 @@ public static function getStandardLanguageList() {
   +   * The official languages used at the United Nations with
   +   * their English and native names.
   ...
   +   *   An array of language code to language name information.
   +   *   Language name information itself is an array of English and native names.
   

   80 cols rule.

2. +++ b/core/modules/ckeditor/config/schema/ckeditor.schema.yml
   @@ -41,3 +41,4 @@ ckeditor.plugin.stylescombo:
   +
   

   Unrelated change; please remove!

3. +++ b/core/modules/ckeditor/js/ckeditor.language.admin.js
   @@ -0,0 +1,16 @@
   +        return $('#edit-editor-settings-plugins-language-list-type option:selected').text();
   

   Oh, I didn't know about the :selected pseudo selector! Nice!

4. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Language.php
   @@ -0,0 +1,130 @@
   +    foreach ($predefined_languages as $langcode => $language) {
   

   Add a comment: "Generate the language_list setting as expected by the CKEditor Language plugin, but key the values by the full language name so that we can sort them later on."

5. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Language.php
   @@ -0,0 +1,130 @@
   +        'image_alternative' => '<a href="#" class="cke-icon-only cke_ltr" role="button" title="' . t('Language') . '" aria-label="' . t('Language') . '"><span class="cke_button_icon cke_button__language_icon">' . t('Language') . '<span class="ckeditor-button-arrow"></span></span></a>',
   ...
   +      '#title' => t('Language list type'),
   ...
   +        'un' => t('United Nations subset'),
   +        'all' => t('All @count languages', array('@count' => sizeof($predefined_languages))),
   

   Use $this->t() rather than t(). It's already available because it subclasses CKEditorPluginBase :)

6. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Language.php
   @@ -0,0 +1,130 @@
   +    $config = array('list_type' => 'un');
   

   Add a comment "Defaults." above this, just like in StylesCombo::settingsForm(), where you got this code block from :)

7. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Language.php
   @@ -0,0 +1,130 @@
   +      '#description' => $this->t('The list of languages to show in the language dropdown. The basic list will only show the <a href="@url">six official languages of the UN</a>. The extended list will show all @count languages that are available in Drupal.', array(
   

   Here you did it correctly!

Thanks @Wim for this thorough review. I've implemented all your suggestions, see interdiff.

Only two super silly nitpicks remaining.

BarisW: A reroll to fix them would be nice, but if needed, I think a core committer could fix them on commit. Great work here! :)

Dear core committer, some notes for you that should help you while reviewing this:

1. there is one @todo in this patch, which documents a pre-existing flaw, that is out of scope for this patch to fix. So please don't hold this patch back on that.
2. while this is a 2.62 MB patch, that's only because ckeditor.js is minified and diffs for big minified JS files are always big
3. this increases the size of ckeditor.js from 513530 bytes to 516412 bytes: an increase with 2882 bytes, or 0.5%. That's not going to make a noticeable difference.
4. this language plugin is not being enabled by default, it is only being made available by default. The reason: it'd unnecessarily complicate the 90%. But at the same time, this makes Drupal 8 compliant with WCAG 2.0, because one only has to add this button in the CKEditor toolbar configuration — no messing with contrib modules required.

1. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
   @@ -353,15 +353,15 @@
   +   * This list is bases on
   

   s/bases/based/

2. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Language.php
   @@ -64,6 +64,9 @@
   +    // plugin, but key the values by the full language name so that we can
   +    // sort them later on.
   

   80 cols rule.

Thanks a lot for summing up the notes for the core committer. Here's a patch with both points fixed too.

Looks good. Thank so much @Wim Leers for the great review & @BarisW for all the work to get this patch to this state!

Thanks @Wim Leers, @BarisW, and @mgifford. While this is a good feature and has accessibility value, I agree with @Wim Leers' assessment in #86 that this is a feature request. I see that alexpott discussed this with Wim Leers in #91 but the deadline for work allowed during Amsterdam has passed. Following the beta changes policy (https://www.drupal.org/core/beta-changes), this feature request will need to be postponed to 8.1.x. Would it be possible to provide this functionality in contrib for 8.0.0, and then we could merge it into 8.1.x potentially?

Drat! So close...

Why is this marked Postponed? I understand it moving to 8.1.x but Postponed items get forgotten much of the time.

When could we merge it into the 8.1.x branch? Any reason for it not to be RTBC'd for 8.1.x? Guess it might need to be Needs Review as you'd want to review it again...

@mgifford and I discussed #134 in person at BADCamp. The 8.1.x branch isn't open for development yet, so we're marking all issues against it postponed to make that explicit. Thanks @mgifford!

Nice to see that this language button is officially in CKEditor now (and has been for a while):
http://dev.ckeditor.com/ticket/7987#comment:30
http://ckeditor.com/demo#full

Here is a re-roll to keep up with HEAD.

Failing due to config schema validation errors:

Uncaught PHP Exception Drupal\Core\Config\Schema\SchemaIncompleteException: "Schema errors for editor.editor.filtered_html with the following errors: editor.editor.filtered_html:settings.plugins.language missing schema"

Here you go: the necessary config schema.

1. +++ b/core/modules/ckeditor/ckeditor.libraries.yml
   @@ -77,3 +83,14 @@ drupal.ckeditor.stylescombo.admin:
   +    - core/jquery.once
   ...
   +    - core/drupalSettings
   

   AFAICT the JS uses neither of these.

2. +++ b/core/modules/ckeditor/ckeditor.module
   @@ -69,6 +69,12 @@ function ckeditor_ckeditor_css_alter(array &$css, Editor $editor) {
   +  // @todo: This should only be loaded when the Language button is enabled;
   +  //    the CKEditor text editor plugin should load this automatically,
   +  //    CKEditor plugins that want to load additional CSS *IN* CKEditor
   +  //    shouldn't have to implement hook_ckeditor_css_alter().
   +  $css[] = drupal_get_path('module', 'ckeditor') . '/css/plugins/language/ckeditor.language.css';
   

   We should investigate this more closely now.

3. +++ b/core/modules/ckeditor/config/schema/ckeditor.schema.yml
   @@ -33,6 +33,15 @@ editor.settings.ckeditor:
   +    list_type:
   ...
   +      label: 'Language list ID
   

   Having now written the config schema, it seems quite clear to me that list_type is not very meaningful. language_list would be a better name I think?

4. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Language.php
   @@ -0,0 +1,134 @@
   +    return array(
   ...
   +    $language_list = array();
   +    $config = array('list_type' => 'un');
   ...
   +    $config = array(
   ...
   +    return array(
   +      'Language' => array(
   ...
   +    $config = array('list_type' => 'un');
   ...
   +    $form['list_type'] = array(
   

   array() -> []

5. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Language.php
   @@ -0,0 +1,134 @@
   +    $predefined_languages = ($config['list_type'] == 'all') ?
   

   ===

6. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Language.php
   @@ -0,0 +1,134 @@
   +    // Generate the language_list setting as expected by the CKEditor
   +    // Language plugin, but key the values by the full language name
   +    // so that we can sort them later on.
   

   Fill up to 80 cols.

7. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Language.php
   @@ -0,0 +1,134 @@
   +        'un' => $this->t('United Nations subset'),
   

   subset is a scary, complex word to most people. What if we rename this to United Nations' official languages?

8. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Language.php
   @@ -0,0 +1,134 @@
   +      '#description' => $this->t('The list of languages to show in the language dropdown. The basic list will only show the <a href="@url">six official languages of the UN</a>. The extended list will show all @count languages that are available in Drupal.', array(
   

   Needs to be updated for [#2571689].

9. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Language.php
   @@ -0,0 +1,134 @@
   +        '@count' => sizeof($predefined_languages),
   

   We always use count(), not sizeof().

I'm stupid.

This fixes the last fail: the Stable theme requires a duplicate of each CSS file to exist, to have a stable base to develop against (zero changes allowed to the duplicate in Stable).

Should come back green now. Still NW for #141.

I only made two actual changes:

1. Added a config schema.
2. Copied the newly added CSS to Stable, and updated Stable.

Both of those are just to make the existing patch work on latest Drupal 8. Both are tiny changes. I can still act as reviewer on this issue and RTBC it when the time comes.

Thanks Wim for your help. Would never have found these fixes myself.
I'll work on #141 in the next few days.

Excellent! @mgifford, will you be able to review this patch again later this week, or perhaps next week?

I believe that I fixed everything except for the todo in #141-2.
Is there a way to see if a button is enabled in the $editor instance?

Let's see what the testbot thinks.

Language in the WYSIWYG editor

The generated HTML output

Looking great :) Here's another review round.

Code review

1. +++ b/core/modules/ckeditor/config/schema/ckeditor.schema.yml
   @@ -38,9 +38,9 @@
   +    language_list_type:
   ...
   +      label: 'Language list type ID'
   

   What does "language list type" mean? This is about selecting a list of languages, right? Either the UN one, or all languages.

   I don't see why either is a type of language list.

   Not sure which is best. But I'd rather debate this now to make sure we get it right the first time.

2. +++ b/core/assets/vendor/ckeditor/skins/moono/icons_hidpi.png
   --- a/core/lib/Drupal/Core/Language/Language.php
   +++ b/core/lib/Drupal/Core/Language/Language.php
   

   Please revert the changes here, unrelated to this patch.

3. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
   @@ -333,6 +333,28 @@ public static function getStandardLanguageList() {
   +   * http://www.un.org/en/aboutun/languages.shtml/ and it
   +   * uses the same format as getStandardLanguageList().
   

   80 cols.

4. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
   @@ -333,6 +333,28 @@ public static function getStandardLanguageList() {
   +   *   An array of language code to language name information.
   +   *   Language name information is an array of English and native names.
   +   */
   

   80 cols.

   An array of language code to language name information.
   ->
   An array with language codes as keys, and English and native language names as values.

5. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
   @@ -333,6 +333,28 @@ public static function getStandardLanguageList() {
   +      'en' => array('English', 'English'),
   

   s/array()/[]/

6. +++ b/core/modules/ckeditor/css/plugins/language/ckeditor.language.css
   @@ -0,0 +1,28 @@
   + * border. We also append the value of the tag between brackets, eg: '(en)'.
   

   s/eg/for example/

7. +++ b/core/modules/ckeditor/css/plugins/language/ckeditor.language.css
   @@ -0,0 +1,28 @@
   +html[lang] {
   

   Hrm, I wonder if we can get rid of this by changing the selector above to
   html [lang]?

8. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Language.php
   @@ -0,0 +1,133 @@
   +    return array(
   ...
   +    return array(
   +      'Language' => array(
   

   s/array()/[]/

9. +++ b/core/modules/ckeditor/src/Tests/CKEditorAdminTest.php
   @@ -148,7 +148,7 @@ function testExistingFormat() {
   -    $this->assertIdentical($expected_settings, $editor->getSettings(), 'The Editor config entity has the correct settings.');
   +    $this->assertEqual($expected_settings, $editor->getSettings(), 'The Editor config entity has the correct settings.');
   

   Why exactly are these changes necessary again?

   #125 explains the original reason. But I wonder if that's still necessary. Because we have the ksort() in Language::getConfig().

   If I revert a few of these locally, tests still pass at least. So let's try removing these. Fewer changes = better. Especially if those changes reduce strictness.

TODO investigation

+  // @todo: This should only be loaded when the Language button is enabled;
+  //    the CKEditor text editor plugin should load this automatically,
+  //    CKEditor plugins that want to load additional CSS *IN* CKEditor
+  //    shouldn't have to implement hook_ckeditor_css_alter().
+  $css[] = drupal_get_path('module', 'ckeditor') . '/css/plugins/language/ckeditor.language.css';

It's possible to at least already do the first bit (This should only be loaded when the Language button is enabled;), so I added that. To do so, I added a new public method to CKEditorPluginManager, extracted from existing code, and now with explicit test coverage added.
Addressing the general problem is out of scope here, so I opened an issue for that: #2645100: CKEditorPluginCssInterface: Allow CKEditor plugins to add CSS to iframe CKEditor instances.

The failures are due to ckeditor.language.css no longer being loaded always. This is a good thing. I'll leave that to @BarisW to address along with #150.

Note that this test failure does not block an accessibility review from @mgifford.

Looks good to me. The only potential issue I could find was that the styles button moved from the left to the right:

Before patch the "Styles" button is to the left.

After patch "Styles" button is to the right:

Some folks will notice the change, but I really don't think it's something to be concerned with. Looking forward to seeing the new patch to account for the test failure.

The only potential issue I could find was that the styles button moved from the left to the right:

What do you mean, moved from the left to the right?

Wim, thanks again for the thorough review and thanks for adding getEnabledButtons().

What Mike means is that the Style dropdown button is positioned as the first button in the toolbar /before/ applying the patch, and as last button /after/ applying the patch.
I can't explain this behavior. Odd.

Regarding naming the list (language_list_type): I think you are right. I was thinking about theme_item_list when I wrote this; that function has a parameter 'list_type' (ol/ul), so I was thinking "language list type' (un/all). But I agree, Language Type is better, so I changed it.

The reason that I changed assertIdentical to assertEqual was that at the time my local tests passed, but the drupal.org tests failed.
Together with Xano we figured out that using assertEqual fixed the issue. So I'm curious it the testbot returns green this time, after reverting the change.

I just tried a new install, without applying the patch and the Styles dropdown is right aligned as well.
So I don't think this change is related to my patch.

Dang. Forgot to fix the test. Will do that tomorrow.
The good thing is that assertIdentical no longer fails.

I just tried a new install, without applying the patch and the Styles dropdown is right aligned as well.

This is why I still don't understand Mike's remark about that: I also don't see any difference before vs. after. @mgifford, can you provide before/after screenshots showing the problem?

The good thing is that assertIdentical no longer fails.

Indeed, yay! Fewer changes :)

1. +++ b/core/lib/Drupal/Core/Language/Language.php
   @@ -79,7 +79,7 @@ class Language implements LanguageInterface {
   -  public function __construct(array $values = array()) {
   +  public function __construct(array $values = []) {
   

   This is the only remaining out-of-scope change. Please revert this too.

2. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
   @@ -333,6 +333,28 @@ public static function getStandardLanguageList() {
   +   *   names as values. Language name information is an array of English and
   +   *   native names.
   

   Language name information is an array of English and native names. is already explained in the prior sentence. This sentence can be removed.

3. +++ b/core/modules/ckeditor/ckeditor.libraries.yml
   @@ -72,8 +78,17 @@ drupal.ckeditor.stylescombo.admin:
   -    - core/jquery.once
        - core/drupal.vertical-tabs
   -    - core/drupalSettings
        # Ensure to run after ckeditor/drupal.ckeditor.admin.
        - ckeditor/drupal.ckeditor.admin
   +
   +drupal.ckeditor.language.admin:
   +  version: VERSION
   +  js:
   +    js/ckeditor.language.admin.js: {}
   +  dependencies:
   +    - core/jquery
   +    - core/drupal
   +    - core/jquery.once
   +    - core/drupal.vertical-tabs
   +    - core/drupalSettings
   

   I asked to remove core/jquery.once and core/drupalSettings. But I asked to remove it not from the ckeditor/drupal.ckeditor.stylescombo.admin library, but from the ckeditor/drupal.ckeditor.language.admin library.

   You can keep the removal of core/jquery.once in the stylescombo library, but you need to add back core/drupalSettings, because that is used.

4. --- /dev/null
   +++ b/core/modules/ckeditor/css/plugins/language/ckeditor.language.css
   

   You updated the copy of this file in core/themes/stable. You need to keep this one in sync. Just copy the updated contents over from the other file, that's it :)

Sorry Wim. I updated #156 to address the difference. I had uploaded both the before & after, but didn't include the before link in the comment.

The after screenshot was taken after I'd moved the "i18n" button down to the toolbar. The "Styles" button was on the right the whole time.

So, I think I have all now. Let's go testbot!

Glad the bot likes it. I'm still seeing inconsistencies in the Toolbar Configuration here /admin/config/content/formats/manage/full_html

Put them both on the same screenshot:

Notice the position of the Styles button. This really isn't a big deal, but it is a UI change.

Hi Mike,

I just spun up a plain Simplytest.me, without the patch. The styles button is aligned on the right.
So I'm sure this change (?) is unrelated to my patch.

See: https://dmkys.ply.st/admin/config/content/formats/manage/basic_html

On a fresh install of D8 on simplytest.me, I see it's left-aligned. On my local install of D8 (without this patch), it's right-aligned. This is a problem independent of this patch, and I'm not sure it's really problem. The ordering of the "available buttons" doesn't really matter. (As @mgifford already says in #156.)

• @mgifford has reviewed the accessibility side of this in #156.
• I've thoroughly reviewed the code and tested it.
• This patch was in fact already RTBC'd back in November 2014 (see #132), but it was not accepted at the time because we needed to focus on getting Drupal 8 out.

Therefore marking this RTBC once again :) Great work, @BarisW & co!

CR created: https://www.drupal.org/node/2648056.
IS updated.

To the committer: my only contributions to this patch:

• Fix a failing test by adding the initial config schema in #140/#143, which @BarisW has since refined
• Fix a failing test by copying the new CSS to the stable theme, which @BarisW has since refined
• Fix one outstanding problem due to a lacking API, that only I as a maintainer of this module in core could reasonably have fixed: in #150 I extracted a bit of existing code and made it into a public function (and therefore created an API addition), and added explicit test coverage for it.

The first two are unquestionably trivial. The third is also trivial: the code is pre-existing, I only had to add trivial test coverage.

If needed, the sole API addition here could be moved into a separate issue. But doing so would mean introducing an API without any user of this API. So it makes more sense to do it as part of this issue.

I was comparing Simplytest.me:

https://simplytest.me/project/drupal/8.1.x
https://simplytest.me/project/drupal/8.1.x?patch[]=https://www.drupal.or...

It's a little issue though. Shouldn't be a concern.