add (all languages) hint to synchronized fields on translation add/edit form

#1807692: Introduce a column synchronization capability and use it to translate alt and titles through the image field widget got committed.

So... I was looking for a D8MI issue that I could write the initial patch for, and this one seemed promising (yay, no patch yet!) but I got stuck on what should actually be done (from a UX perspective), before I could even begin to think about how to implement it in code.

The original description suggests putting "(all languages)" on the Image field label to indicate the image will be synchronized to all language versions. And we're indicating the "Alternative text" is translatable by adding nothing to it's label. However, in pratice that looks like this (real Drupal 8 site mod'ed slightly with Firebug):

The potential problem is from a UX perspective, is that it looks like the "(all languages)" concerns everything in the Image field, including the "Alternative text"!

Maybe we should mark translatable sub-fields with "(translatable)" when not everything is? So, something like this:

Or maybe the "(all languages)" bit should appear somewhere else, closer to the thumbnail so it's clearer that this affects only the image file?

In any case, once I know what should actually be done, I'd be happy to take a stab at the initial patch. :-)

Thanks in advance for any feedback!

Er, I didn't mean to add accessibility. :-)

Er, what is going on with tags! I just want them to remain as they were when I got here. Sorry for the mess...

And fixed my type-o in the title. Double extra sorry for the mess! ;-)

Jammed with YesCT on this! Here are mock-ups of the 3 most reasonable states of the translation add/edit form...

Case #1: Nothing on the image field is translatable, it will be the same for all languages:

Case #2: Only the Alternative text is translatable, the File will be the same for all languages:

Case #3: Everything (both the File and the Alternative text) are translatable:

This issue is to make this happen! I'm going to take a stab at it...

I didn't create mock-ups for the some of the possible configurations that don't make sense. Here is an issue to decide if we should actually disallow these insane configurations: #1938422: When configuring which parts of an image field are translatable, should we allow insane combinations?

Jammed some more with YesCT and decided that Case #2 still doesn't make the reality of the configuration very evident to users. That is:

• If they change the weight of the images on a field with multiple values, those weight changes will be saved for all languages.
• If they remove an image, it will be removed for all languages.

The one idea we had was to put a message above or below the field when the images were re-ordered or an image was removed. This would be similar to the message from core/misc/tabledrag.js that tells you that weight changes won't be saved until the form is submitted.

The goal is: we don't want to users to be surprised when suddenly the image field changes for all languages and they didn't expect it.

I think printing a message AFTER the file was removed might be too late. Reordering you can fix, but file remove you cannot. So maybe a permanent description for that case too?

@Gábor Hojtsy: Yeah, you're right... The question is where to put that? Below every "Remove" button? That seems ... not optimal. :-)

Maybe window.confirm(Drupal.t('Remove this image will remove it for all languages. Are you sure you want to continue?')); or something like that?

While working on this I've kept having to fight with this bug #1938870: After changing the cardinality of the field_image created by the standard profile, images can no longer be uploaded which I've just finished figuring out how to reproduce.

Yes, a confirm like that maybe on the first one and then not afterwards? Sounds like it could be tedious to always click through all the time. Alternatively change button label to Remove from all translations?

About an hour ago I sat down to finally write this patch and I got stuck on some other bug that's preventing me from uploading images: http://drupal.org/node/1416878#comment-7236512

Unhappy face. :-/

I'll try to get back into this tomorrow!

I've attached an initial patch. It desperately needs review!

There lots of ugly things in there where I just don't know the correct approach because I'm not that familiar with all these new systems. For example:

1. When looping through $entity_ng->getPropertyDefinition() how can I tell if a property is a field? Right now, I'm just calling field_info_instance() and seeing if something gets returned.
2. How best to pass the 'translation_sync' values to EntityTranslationController? Right now, I'm stashing them on the form.
3. How to connect the 'file' in 'translation_sync' to a form element so that we can change it's #title? Right now, this is hard-coded.

These are only a few of the many terrible things going on in this patch. ;-)

Oh, and I haven't addressed any of the reordering or remove button stuff yet (discussed in comments #11-13)...

Thanks in advance!
David.

Forgot to mark as 'needs review'!

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationController.phpundefined
@@ -395,6 +398,45 @@ protected function addTranslatabilityClue(&$element) {
   /**
+   * Adds a clue about columns inside a form element that will be synchronized
+   * for all languages.

Function intros should be on one line. Further explanation should be on another line and an empty line inbetween.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationController.phpundefined
@@ -395,6 +398,45 @@ protected function addTranslatabilityClue(&$element) {
+   *
+   * @todo This should be 'protected' not 'public', but we get an access
+   *   violation..

Yeah FAPI will call it back, so it will be called from outside the class. Leave it public and remove this note.

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -624,6 +637,8 @@ function translation_entity_form_alter(array &$form, array &$form_state) {
+          // @todo Use the same logic as above... should probably refactor to
+          //   function.

Not yet repeating?

@Gabor: Thanks!

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationController.phpundefined
@@ -395,6 +398,45 @@ protected function addTranslatabilityClue(&$element) {
+   *
+   * @todo This should be 'protected' not 'public', but we get an access
+   *   violation..

Yeah FAPI will call it back, so it will be called from outside the class. Leave it public and remove this note.

Right but I felt it should match it's brother: addTranslatabilityClue(), which is protected. It's in there because FAPI calls entityFormSharedElements(), which is public. Anyway, yeah, consistancy is nice but probably doesn't matter.

++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -624,6 +637,8 @@ function translation_entity_form_alter(array &$form, array &$form_state) {
+          // @todo Use the same logic as above... should probably refactor to
+          //   function.

Not yet repeating?

Yeah, basically needs the same code for Entity and EntityNG. I've been testing with Node which is EntityNG and didn't want to copy-paste the same code for Entity. I'll refactor to a function so it adheres to DRY in a subsequent patch.

Thanks again for the review!

Er, tags always seem to get messed up! Returning to how YesCT had them.

Hmm, after applying patch sync-hint-1909218-18.patch by dsnopek in #18 to Drupal 8, I ran into a number of errors. See screenshots. Good first start and let's keep trying! I may be doing something wrong too, but I tried to follow all the steps cleanly from the issue steps to test.

In core terms, my patch is *old*. :-) So, I'm not surprised it breaks. I'll try to find some time to get it working with current HEAD.

At the moment, it seems like the patch doesn't add translation hints to the image field. I enabled translation on the article content type and the image field. In the field edit page, I made only the alt text and title text translatable:

Here's a screenshot of the image field (with the image file synchronized and the alt text translatable). I would think we would want to add an (all languages) hint to the image file upload itself and a (language-specific) hint to the alt text label. I've added these to the screenshot in red:

Straight reroll of #18 after #2024867: Rename translation_entity to content_translation.

In any case there's more to do.

added steps to test and updated remaining tasks

I think I duplicated this in #3152587: Add a mark when editing a field property will affect all the translations as we do with fields themselves. There are some bits here that I like more, and there I workaround-ed a fix for

+    // @todo This essentially 'field_file' specific.. How can this be
+    //   made more generic?