Follow up for #1807692-72: Introduce a column synchronization capability and use it to translate alt and titles through the image field widget

Problem/Motivation

in the translation add/edit form, fields that are not translated (have the same value in the content and all translations) have a (all languages) hint on them. The parts of a synchronized field that are the same in all languages do not have a hint. This is confusing/misleading.

Proposed resolution

Put a note that the field is sync'd in the edit

Remaining tasks

  • (done) create initial patch to add the hint
  • review approach and suggest better strategy (see #18)
  • iterate on improving patch (doc on creating patch: http://drupal.org/node/1424598 includes link to interdiff instructions)
  • add screenshots of interface
  • improve steps to test

Steps to test

It helps to do this once without the patch to see the interface before.
Then repeat with the patch.

  1. install a fresh drupal 8 [with the most recent patch applied] (from git or simplytest.me)
  2. enable the content translation module under Extend
  3. configure it to add a couple languages
  4. configure the content language settings to turn on translation for content that has something to synchronize, like images. Article works.
  5. Create article and add an image.

User interface changes

No ui changes

API changes

No API changes expected.

Files: 
CommentFileSizeAuthor
#29 sync-hint-1909218-29.patch4.08 KBPancho
PASSED: [[SimpleTest]]: [MySQL] 58,728 pass(es). View
#25 addlanguageshintbefore1.png51.76 KBandymartha
#25 addlanguageshintbefore2.png42.73 KBandymartha
#25 addlanguageshintafter1.png139.06 KBandymartha
#18 sync-hint-1909218-18.patch4.06 KBdsnopek
PASSED: [[SimpleTest]]: [MySQL] 53,640 pass(es). View
#9 Selection_049.png41.48 KBdsnopek
#9 Selection_050.png41.28 KBdsnopek
#9 Selection_051.png40.49 KBdsnopek
#5 Selection_047.png28.19 KBdsnopek
#5 Selection_048.png28.91 KBdsnopek

Comments

YesCT’s picture

Issue tags: -accessibility
YesCT’s picture

Status: Active » Postponed

postponing on #1807692: Introduce a column synchronization capability and use it to translate alt and titles through the image field widget
will need some change to issue summary to reflect improved ui in the original issue.

Gábor Hojtsy’s picture

YesCT’s picture

the hint should probably just be (all languages)

"synchronized" is not in the ui anymore.

dsnopek’s picture

Title: add (synchronized) hint similar to (all-languages) on translation add/edit form » add (all languages) hint similar to synchronized fields on translation add/edit form
Issue tags: +accessibility
FileSize
28.91 KB
28.19 KB

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!

dsnopek’s picture

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

dsnopek’s picture

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

dsnopek’s picture

Title: add (all languages) hint similar to synchronized fields on translation add/edit form » add (all languages) hint to synchronized fields on translation add/edit form

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

dsnopek’s picture

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:

Nothing translatable

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

Only alternative text is translatable

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

Everything is translatable

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

dsnopek’s picture

Assigned: Unassigned » dsnopek

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?

dsnopek’s picture

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.

Gábor Hojtsy’s picture

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?

dsnopek’s picture

@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?

dsnopek’s picture

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.

Gábor Hojtsy’s picture

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?

YesCT’s picture

@dsnopek Thanks for being so thoughtful with this.

I suggest adding a warning to the "changes in order are not saved until, it's saved" or whatever that message is, to warn that the change will effect every translation.

I also suggest trying #15

change button label to Remove from all translations

when it does remove from all translations.

Lets get a patch in for that and then we can ask for a usability review on it.

dsnopek’s picture

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!

dsnopek’s picture

FileSize
4.06 KB
PASSED: [[SimpleTest]]: [MySQL] 53,640 pass(es). View

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.

dsnopek’s picture

Status: Active » Needs review

Forgot to mark as 'needs review'!

Gábor Hojtsy’s picture

+++ 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?

YesCT’s picture

For someone looking to get involved, doing a manual review and/or posting updated screenshots would be great.

For experienced folks, giving advice on approach that should be used would be helpful.

this is kind of both needs review and needs work. :)

dsnopek’s picture

@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!

dsnopek’s picture

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

YesCT’s picture

I updated issue summary with steps to test.
Improve those by editing the issue summary and posting a comment with results of what you tried. Anyone can do that.

andymartha’s picture

Status: Needs review » Needs work
Issue tags: -Needs screenshots
FileSize
139.06 KB
42.73 KB
51.76 KB

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.

addlanguageshintbefore1.png

addlanguageshintbefore2.png

addlanguageshintafter1.png

dsnopek’s picture

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.

pixelite’s picture

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:

Field edit page

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:

Synchronized image field

Gábor Hojtsy’s picture

Component: translation_entity.module » content_translation.module
Pancho’s picture

Status: Needs work » Needs review
FileSize
4.08 KB
PASSED: [[SimpleTest]]: [MySQL] 58,728 pass(es). View
Pancho’s picture

Status: Needs review » Needs work

In any case there's more to do.

Pancho’s picture

Issue summary: View changes

added steps to test and updated remaining tasks

klonos’s picture

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.