Caution, this is not a simple issue to work on, its category of "normal" is not the difficulty, but a measure of its impact.

Problem/Motivation

Image fields have a Title and Alt sub-fields. Both sub-fields are optional and translatable, but enabling/disabling each sub-field does not show/hide the corresponding translation controls. This is confusing, especially because Alt is enabled by default, but Title is not.

Proposed resolution

  1. When editing an image field, use Form API states to hide the Alt checkbox in the Translatable elements list when the Enable Alt field checkbox is unchecked.
  2. When editing an image field, use Form API states to hide the Title checkbox in the Translatable elements list when the Enable Title field checkbox is unchecked.
  3. When editing an image field, display a message ("Disabled fields are not shown. Please first enable the fields you need to make translatable.") below the Translatable elements list if any Translatable elements checkboxes are being hidden.
  4. On the Content language settings page at admin/config/regional/content-language, hide the Alt checkbox if Enable Alt field is unchecked.
  5. On the Content language settings page at admin/config/regional/content-language, hide the Title checkbox if Enable Title field is unchecked.
  6. On the Content language settings page, display a message ("Disabled fields are not shown. Please first enable the fields you need to make translatable.") if any checkboxes are being hidden.

Note that testbot cannot run front-end tests automatically (see #2232271: [Meta] Use Behat for validation testing or #2229187: SiteEffect: Automated frontend regression testing, also #825436: Create selenium-RC PIFR plugin for full functional testing).

Aside: should this issue be split into two?

It's been suggested the issue be split into two issues: one to change the image field edit page, and one to change the Content language page.

The problem with splitting into two issues is that whichever one is fixed first might be broken by trying to fix the other, and we wouldn't know because we don't have tests (this happened at some point in working on this issue).

Remaining tasks

  • Wait for #2290849: Regression: Fields missing from translation overview since "Clarify handling of field translatability".
  • Determine whether it is necessary to announce to screenreaders when the checkboxes are shown/hidden.
  • Decide whether to postpone this issue until we have automatic front-end tests.
  • Write front-end tests.
  • Show/hide checkboxes on the image field edit page, and announce this to screenreaders.
  • Display a warning when checkboxes are being hidden on the image field edit page.
  • Show/hide checkboxes on the Content language settings page, and announce this to screenreaders.
  • Display a warning when checkboxes are being hidden on the Content language settings page.
  • Usability review.

Steps to reproduce

  1. Install a fresh Drupal 8.0 site using the Standard install profile (drush -y site-install standard).
  2. Enable the Content Translation module (drush -y pm-enable content_translation).
  3. Go to admin/config/regional/language and add a second language.
  4. Go to admin/structure/types/manage/article/fields/node.article.field_image.
    1. There is a checkbox labeled "Enable Alt field". It is checked by default.
    2. There is a checkbox group titled "Translatable elements", containing a checkbox labeled "Alt". It is checked by default.
    3. There is a checkbox labeled "Enable Title field". It is unchecked by default.
    4. Expected behavior:
      1. There is no checkbox labeled "Title" in the "Translatable elements" checkbox group. There is a message saying "Disabled fields are not shown. First enable the fields before changing their translatable setting."
    5. Actual behavior:
      1. There is a checkbox group titled "Translatable elements", containing a checkbox labeled "Title". It is checked by default.
  5. Uncheck the checkbox labeled "Enable Alt field".
    1. Expected behavior:
      1. The checkbox labeled "Alt" in the "Translatable elements" checkbox group disappears.
      2. The message "Disabled fields are not shown. First enable the fields before changing their translatable setting." stays visible.
    2. Actual behavior:
      1. The checkbox labeled "Alt" in the "Translatable elements" checkbox group stays visible.
      2. No message is shown.
  6. Check the checkbox labeled "Enable Alt field".
    1. Expected behavior:
      1. A checkbox labeled "Alt" in the "Translatable elements" checkbox group appears. It is checked by default.
      2. The message "Disabled fields are not shown. First enable the fields before changing their translatable setting." stays visible.
    2. Actual behavior:
      1. The checkbox labeled "Alt" in the "Translatable elements" checkbox group stays visible.
      2. No message is shown.
  7. Check the checkbox labeled "Enable Title field".
    1. Expected behavior:
      1. A checkbox labeled "Title" in the "Translatable elements" checkbox group appears. It is checked by default.
      2. The message "Disabled fields are not shown. First enable the fields before changing their translatable setting." disappears.
    2. Actual behavior
      1. The checkbox labeled "Title" in the "Translatable elements" checkbox group remains visible.
      2. No message is shown.
  8. Uncheck the checkbox labeled "Enable Title field".
    1. Expected behavior:
      1. The checkbox labeled "Title" in the "Translatable elements" checkbox group disappears.
      2. The message "Disabled fields are not shown. First enable the fields before changing their translatable setting." appears.
    2. Actual behavior
      1. The checkbox labeled "Title" in the "Translatable elements" checkbox group remains visible.
      2. No message is shown.
  9. Check the checkbox labeled "Enable Title field".
  10. Click Save configuration.
  11. Go to admin/config/regional/content-language. In the Custom language settings checkbox group, check Content. In the Content table, check the Translatable checkbox for the Article content type.
    1. A list of fields available for translation appears.
    2. Under "Article" -> "Image", the "File" checkbox is unchecked.
    3. Under "Article" -> "Image", the "Alt" checkbox is checked.
    4. Under "Article" -> "Image", the "Title" checkbox is checked.
    5. No message is shown.
  12. Go to admin/structure/types/manage/article/fields/node.article.field_image.
  13. Uncheck the checkbox labeled "Enable Title field".
  14. Uncheck the checkbox labeled "Enable Alt field".
  15. Click Save configuration.
  16. Go to admin/config/regional/content-language. In the Custom language settings checkbox group, check Content. In the Content table, check the Translatable checkbox for the Article content type.
    1. A list of fields available for translation appears.
    2. Under "Article" -> "Image", the "File" checkbox is unchecked.
    3. Expected behavior:
      1. Under "Article" -> "Image", there is no "Alt" checkbox.
      2. Under "Article" -> "Image", there is no "Title" checkbox.
      3. The message "Disabled fields are not shown. First enable the fields before changing their translatable setting." is shown.
    4. Actual behavior:
      1. Under "Article" -> "Image", the "Alt" checkbox is checked.
      2. Under "Article" -> "Image", the "Title" checkbox is checked.
      3. No message is shown.

User interface changes

  • When editing image field settings:
    • Hide the Alt checkbox in the Translatable elements list when the Enable Alt field checkbox is unchecked.
    • Hide the Title checkbox in the Translatable elements list when the Enable Title field checkbox is unchecked.
    • Display a message when a checkbox is being hidden from the Translatable elements list.
  • On the Content language settings page:
    • Hide the Alt checkbox in the list of fields when the Enable Alt field checkbox is unchecked in the field settings.
    • Hide the Title checkbox in the list of fields when the Enable Title field checkbox is unchecked in the field settings.
    • Display a message when a checkbox is being hidden from the list of fields.

API changes

None.

(If problems are discovered in the config storage, that will be filed in a separate issue)

Files: 

Comments

YesCT’s picture

Priority: Major » Normal

not major.

YesCT’s picture

Category: feature » bug

also not a feature request :)

YesCT’s picture

FileSize
247.56 KB
199.6 KB

steps to reproduce:

  1. a clean d8 (git pull --rebase)
  2. install (drush si is nice)
  3. enable content translation
  4. add a language
  5. configure translation and language settings at admin/config/regional/content-language
  6. check "content" and "article" and save
    settings.png
  7. create content of type article, add an image.
  8. translate that content, notice when adding or editing the translation, there is no title field, even though when we enabled translation, title was checked.
    title_missing.png
YesCT’s picture

Note the system is working as it should.

The disconnect is in the expectations of the person setting up translation.
Both in the language and translation overview settings screen: admin/config/regional/content-language
and in the field settings (which is the image in the issue summary): admin/structure/types/manage/article/fields/field_image

Those show "title" checked (able to be checked) when title is not enabled on the field.
So the proposed solution here is to add some code to hide the title from the translation settings if title is not enabled for the field.
And more generically, to hide alt if it's not enabled on the field
And if we can be even more generic so this works for other fields besides images that have various parts to them.

e0ipso’s picture

FileSize
112.83 KB
2.82 KB
PASSED: [[SimpleTest]]: [MySQL] 52,214 pass(es). View

I've been experimenting a bit with this and I have found the following.

Both regional settings and field settings use the same function to populate the list of available column groups (translation_entity_field_sync_widget), when the Translatable elements checkboxes are generated (and default values set).

The proposed patch is for translation_entity because it tries to fix this for generic fields. The restriction is the key of the field being the same as the translation checkbox key (or adding "_field" to it).

Screenshot_3_2_13_17_56.png

e0ipso’s picture

Assigned: Unassigned » e0ipso
Status: Active » Needs review
YesCT’s picture

Status: Needs review » Needs work

I think we should either
1) also add a note to the disabled field explaining why it cannot be made translatable? and where to enable it, ... we can generate a link to the mangage fields tab, since we know what kind of content it relates to, right?

or

2) just hide it. If people knew a title was a possible field, they would probably also know where to enable it (so no hint needed to tell them where to do it)

YesCT’s picture

I looked at the code for coding style stuff and it looks excellent. :)

e0ipso’s picture

Status: Needs work » Needs review
Issue tags: +SprintWeekend2013
FileSize
3.17 KB
PASSED: [[SimpleTest]]: [MySQL] 52,820 pass(es). View
2.22 KB
243.56 KB

Added some feedback to the user in the form description.
The checkboxes are now hidden and disabled if the field is not checked as enabled.

Screenshot

robertgarrigos’s picture

Status: Needs review » Needs work

Works as expected. However needs some wording ("Disabled field are not shown" -> "Disabled fields are not shown")

e0ipso’s picture

Status: Needs work » Needs review
FileSize
921 bytes
3.18 KB
PASSED: [[SimpleTest]]: [MySQL] 52,588 pass(es). View

Corrected typo.

YesCT’s picture

updating tags.

Next:

  • manual testing.
  • embed updated screenshots.

See contributor task doc on manual testing: http://drupal.org/node/1489010

e0ipso’s picture

Issue tags: -Needs screenshots
FileSize
1.3 KB
4.18 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-hide-title-and-alt-groups-1920876-13.patch. Unable to apply patch. See the log in the details link for more information. View
169.74 KB
256.11 KB
180.17 KB
234.31 KB

Updated patch to work on admin/config/regional/content-language.

Browser coverage

This patch has been tested and proven to work under the following browsers.

  • Google Chrome: Version 25.0.1364.160 (Mac OSX)
  • Safari: Version 6.0.2 (8536.26.17) (Mac OSX)
  • Firefox: Version 19.0.2 (Mac OSX)
  • Internet Explorer: Version 8 (Windows XP)

Scenario 1

All elements for the image field (article node bundle) are enabled.

Image settings (all available)
Image_settings_for_Article_|_Sprint_Weekend 2.png
Content language settings (all available)
Content_language_settings_|_Sprint_Weekend-2.png

Scenario 2

Title element is unckecked in the image settings page.

Title element is not show.
Image_settings_for_Article_|_Sprint_Weekend.png
Disable title element and then save. Navigate to content language settings.
Content_language_settings_|_Sprint_Weekend.png

Status: Needs review » Needs work

The last submitted patch, drupal-hide-title-and-alt-groups-1920876-13.patch, failed testing.

killua99’s picture

Status: Needs work » Needs review
FileSize
3.8 KB
PASSED: [[SimpleTest]]: [MySQL] 53,188 pass(es). View

Fixing the patch to allow this feature.

I review it and seen like it's a small but useful feature.

Status: Needs review » Needs work

The last submitted patch, drupal-hide-title-and-alt-groups-1920876-15.patch, failed testing.

e0ipso’s picture

Patch in #15 from killua99 is a reroll of #13 from e0ipso. There are no differences to review.

e0ipso’s picture

Status: Needs work » Needs review
YesCT’s picture

Status: Needs review » Needs work
+++ b/core/modules/translation_entity/translation_entity.admin.incundefined
@@ -36,6 +36,7 @@ function translation_entity_field_sync_widget(array $field, FieldInstance $insta
+      '#description' => t('Disabled fields are not shown. Please enable the fields you need to make translatable first.'),

I dont see where this phrase shows in the screenshots.

+++ b/core/modules/translation_entity/translation_entity.admin.jsundefined
@@ -103,4 +103,43 @@ Drupal.behaviors.translationEntity = {
+ * Sync the Columns between.
+ */
+Drupal.behaviors.translationSyncColumns = {

This comment does not make sense.

Is it meaning:
"Synchronize the display checkbox for fields to enable translation of with those enabled in the content type."

Or something like that? I'm not sure what "column" is. (Maybe retitle the function.) If that is close, it needs some rewording still, as what I wrote is awkward.

Also, "sync" might mean something else in other code in terms of having a field (a field within a field) be the same in every translation.

e0ipso’s picture

FileSize
3.83 KB
PASSED: [[SimpleTest]]: [MySQL] 53,216 pass(es). View
726 bytes
100.69 KB

Re-wording and additional screenshot.

Screenshot_3_16_13_19_44.png

e0ipso’s picture

Status: Needs work » Needs review
e0ipso’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

FileSize
943 bytes
3.83 KB
PASSED: [[SimpleTest]]: [MySQL] 54,087 pass(es). View
404.35 KB
240.17 KB

title-hidden.png

on the overall language content settings configuration page, there is no message saying some fields are hidden, and no link to enable them. This might be ok...

go there, and save, then go to the content type manage fields and edit the image field.

--------

not-hidden.png

here, the added message is strange. moving the "first" word helps a bit, but it's still odd.

Also, I think the message should only be shown *if* some fields are hidden?

And... the title field is *not* hidden, it should be (or at least the message says it should be hidden.)

This page will make more sense if the translatable checkboxes are *after* the area where the fields are enabled/disabled. or at least nearer to those.

patch changes the helptext wording a bit.
needs review to let the bot run on it.
but needs work, to make the field actually be hidden when it's not enabled.

YesCT’s picture

Status: Needs review » Needs work

needs work because in the manage fields -> edit for image the disabled fields need to be hidden (and maybe the translatable group moved down nearer to the enabling checkboxes)

e0ipso’s picture

FileSize
4.23 KB
PASSED: [[SimpleTest]]: [MySQL] 54,734 pass(es). View
1.78 KB

There was some code that got lost in the re-roll that happened in #15 that caused the bug described in #23.

e0ipso’s picture

Status: Needs work » Needs review
bookmarvel’s picture

im about to test this patch

bookmarvel’s picture

Issue summary: View changes

Updated issue summary.

bookmarvel’s picture

Status: Needs review » Needs work
FileSize
151.2 KB

i followed the steps to reproduce.
the patch is better because the title is hidden, but the group of checkboxes is not moved down.

e0ipso’s picture

Status: Needs work » Needs review

I don't think we can move the checkboxes down after the required checkboxes. This patch attempts to work not only for the image case, but for other cases too.

Is this fact a blocker?

YesCT’s picture

Do we have any other case we can look at?
I think in other cases, we would still want it below.

Gábor Hojtsy’s picture

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

FileSize
4.32 KB
PASSED: [[SimpleTest]]: [MySQL] 57,533 pass(es). View

I followed reroll (https://drupal.org/patch/reroll) and made a new patch. I haven't tested it.

e0ipso’s picture

FileSize
1.74 KB
4.87 KB
PASSED: [[SimpleTest]]: [MySQL] 58,869 pass(es). View

After talking with @YesCT in Prague, we decided to position the Translatable elements at the bottom as an special case for the image module.

YesCT’s picture

Status: Needs review » Needs work
FileSize
413.43 KB
127.87 KB
426.83 KB

the structure, content types, article, manage fields, image
works!
and the translatable section is after we pick if we want it or not. :) it is coded as a special case of images, and I think that is fine.

without enable title, the place to enable translation of title is hidden.
titlenotenabled.png

with enabled, we can see it to enable it.
titleenabled.png

but... I tried the other page

it used to work... see first screenshot in #22 that on the content language settings page, that when title is not enabled, that the title will be hidden, so we do not try and enable translation for it.

titlenotenabled-contentlanguagesettingspage.png

YesCT’s picture

Issue summary: View changes

Made steps to reproduce from comment 3

klonos’s picture

Schnitzel’s picture

Assigned: e0ipso » Unassigned

cleaning up assigness

sidharthap’s picture

Updating issue tag. Will have a look at Drupal Camp Mumbai sprint on this weekend.

klonos’s picture

@sidharthap: did you intentionally remove those tags along with the one you've added Sidhartha or did the tag monster actually survive the d.o D7 upgrade? If not intended, please add them back. Thanx.

sidharthap’s picture

I just updated the tag and unsure how the old tags gets deleted.

klonos’s picture

...adding accidentally removed tags back then (most of them - not sure if "#SprintWeekend" will be added because of #2171455: d.o does not autocomplete tags starting with # hash anymore).

alimac’s picture

Issue tags: +Needs reroll

Patch in #32 needs rerolling.

alimac’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
4.99 KB
PASSED: [[SimpleTest]]: [MySQL] 64,537 pass(es). View

One conflict was auto-merged. Another conflict had a lot of context changes.

YesCT’s picture

Status: Needs review » Needs work

I think this is needs work, because the last testing in #33 showed that when the title was not enabled, making it translatable on manage fields for a content type was hidden, but it was still shown on the content translations settings page. It should be hidden there.

Next manual tester, please try that, and also making sure other disable-able fields are hidden. Make sure the alt field works similarly (it should be hidden in the translatable enable area when it's not enabled as a field for the content type field setting).

Should we write a simple test web test to make sure disabled fields dont show on both the content type manage fields settings and also the content translation settings overview page?

klonos’s picture

jlbellido’s picture

I'm going to test manually #41

jlbellido’s picture

I've tested #41 and unfortunally text field still available for be translatable if we have disbled it:

Before apply #41:

After apply #41:

Image field settings in both cases:

I'm going to see what's happening.

jlbellido’s picture

I've fixed some of the following things from #41:

+<<<<<<< HEAD
 })(jQuery, Drupal, drupalSettings);
+======= 
+>>>>>>> Applying patch from issue 1920876 comment 7892481

After remove this changes from #41, it still not working :-(. The problem is that 'translation_sync_options' setting isn't arriving to the behaviour. The translation_sync_options setting is set at the 'content_translation_field_sync_widget' function but it doesn't arrive to the behaviour. After a day searching why is happening this, I couldn't find why.

I attach a new patch with the fixes from #41.

Remaining tasks

- Find why this isn't working.
- Next manual tester and, make sure the alt field works similarly (it should be hidden in the translatable enable area when it's not enabled as a field for the content type field setting).
- Add tests.

javisr’s picture

I'm working on this issue

javisr’s picture

Assigned: Unassigned » javisr
javisr’s picture

FileSize
4.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,738 pass(es). View

Re-rolled.
I will continue with this issue

heddn’s picture

Status: Needs work » Needs review
javisr’s picture

Assigned: javisr » Unassigned

I give it up

jsbalsera’s picture

FileSize
3.59 KB
5.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,850 pass(es), 11 fail(s), and 0 exception(s). View

Working with @javisr on it, it seems that it wasn't checking if the alt and title where enabled or not. Probably it's not the right way to do it, but at least it seems to work, although I'm not sure that it's working the way it should when we save the admin/config/regional/content-language form.

Status: Needs review » Needs work

The last submitted patch, 52: drupal-hide-title-and-alt-groups-1920876-52.patch, failed testing.

javisr’s picture

Assigned: Unassigned » javisr
javisr’s picture

Hi!
To try what is happening with the test I need to undestand very well what are the expected behaviors.
I write here the testcases of the behavior that i undestood but i dont know if it is well.

[A]: Enable Title field
Enable Alt field

[B]: Image
File
Alt
Title

[1] /admin/config/regional/content-language
[2] /admin/structure/types/manage/article/fields/node.article.field_image?destination=admin/structure/types/manage/article/fields/node.article.field_image

As I understood, while [A] should only appear in [2], [B] should appear in both forms.
The behaivor that I should expect is the next one:

By Default:
- go to [2]
- [A] should appear unchecked
- [B] shouldn't appear.
- go to [1]
- [B] should appear without childrens.

[A] is going to be checked in [2]:
- go to [2]
- check [A]
- [B] should appear unchecked.
- go to [1]
- [B] should appear unchecked.

[A] is checked in [2]:
- [B] should appear in [1] & [2]
- Change any of then
- it should be also change in the other one.

Uncheck [A] in [2]:
- [B] shouldn't appear in [1] and [2]

Could someone confirm if this is the expected behavior please :)

PS: Im sorry about my poor English.

YesCT’s picture

[A]: Image
[A][t] Enable Title field
[A][a] Enable Alt field

[B]: Image
[B][f]File
[B][a]Alt
[B][t]Title

[1] /admin/config/regional/content-language
[2] /admin/structure/types/manage/article/fields/node.article.field_image?destination=admin/structure/types/manage/article/fields/node.article.field_image

---------
Behavior of the translation settings [B] in both [1] and [2] should always be the same.
---------

When
[A][t] is unchecked (not enabled) in [2], then [B][t] should not appear in [1] and should not appear in [2]
When
[A][a] is unchecked (not enabled) in [2], then [B][a] should not appear in [1] and should not appear in [2]

Writing the same another way:
When
[A][t] is disabled in [2], then [B][t] should be hidden in [1] and [2]
When
[A][a] is disabled in [2], then [B][a] should be hidden in [1] and [2]

Also same another way:
Writing the same another way:
When
[A][t] is enabled (checked) in [2], then [B][t] should be shown in [1] and [2]
When
[A][a] is enabled (checked) in [2], then [B][a] should be shown in [1] and [2]

Hope that helps.

YesCT’s picture

Issue tags: +Needs tests, +JavaScript

tagging with javascript tag, since this touches javascript files (and #2219493: Add search for issues with patches that touch certain file types (javascript, css, ...) or files doesn't have a solution yet)

tagging with needs tests. :) since this needs tests to both make sure it doesn't break in the future and to do a better job of communicating expected behavior within this issue.

Just for good measure, contributor task doc on adding automated tests: https://drupal.org/contributor-tasks/write-tests
---
So, the previous patch(es) do not change tests.
We might need to change tests or add tests.

in irc @javisr said might have some time later to look at tests. to help with that, I tried to locate other tests we might be able to add to or copy from.

since it deals with the content language settings page..
ag "admin/config/regional/content-language" core
shows tests in:
core/modules/content_translation/lib/Drupal/content_translation/Tests/ContentTranslationSettingsTest.php
core/modules/content_translation/lib/Drupal/content_translation/Tests/ContentTranslationSyncImageTest.php
core/modules/path/lib/Drupal/path/Tests/PathLanguageTest.php

ideally, the test would use a generic content type and field, and not specifically article, but we can start with article if that is easier.

core/modules/field_ui/lib/Drupal/field_ui/Tests/ManageFieldsTest.php around line 578 might also be useful to look at as an example of tests dealing with the field ui.

killua99’s picture

FileSize
3.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,971 pass(es). View

My chance,

Is not so magic, cause the object are diff and make it really hard to handle with JS to hide and show up the checkbox, is better and more secure show or hide the checkbox after submit any post.

This are the obj.
http://drupal8.dev/admin/structure/types/manage/article/fields/node.arti...
Drupal\field\Entity\FieldConfig

http://drupal8.dev/admin/config/regional/content-language
Drupal\field\Entity\FieldInstanceConfig

So the settings are a little diff, that's the reason is so hard to handle with JS.

This is a different way to handle this issue, hope help to solve it.

killua99’s picture

Status: Needs work » Needs review

change status...

YesCT’s picture

+++ b/core/modules/content_translation/content_translation.admin.inc
@@ -110,7 +118,7 @@ function _content_translation_form_language_content_settings_form_alter(array &$
-              $column_element = content_translation_field_sync_widget($definition);
+              $column_element = content_translation_field_sync_widget($definition, $definition);

wah?!

I'll try this out live to see how it feels.

YesCT’s picture

Re patch in #58 http://screencast.com/t/eLZaHoRKbDku
My feeling is that if we do not do this in js (showing/hiding the translatable checkbox as soon as it the enabled is toggled), then we should just leave it showing there all the time.

Maybe it only makes sense to hide/show it on the content language/translation settings overview page? (with a warning that only enabled fields are show?)

Tried #58 again http://screencast.com/t/jDEG3AhGS0

#52
http://screencast.com/t/jDEG3AhGS0

#52 is very close. the title or alt are hidden or shown on both [2] the content type manage fields page and [1] the translation overview settings page as they are disabled or enabled on [2] the content type manage fields page
But, as can be seen at 1:28 in the screencast, the very first time the translation settings are shown on [1], it is not realizing that title is not enabled.

[edit:]
Another problem with #52
http://screencast.com/t/EHe3bdsUaZ
the translatable checkbox on the manage fields tab is always shown as checked, even if the setting was saved to have it not be translatable. (the setting seems to be ok on the [1] translation overview settings page though.

killua99’s picture

FileSize
6.09 KB
6.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,007 pass(es). View

Done, added the JavaScript magic (not so magic, when you enabled for first time eg: Alt, the translation sync doesn't popup, but when you disable it, it will hide. The reason; could happen the translation sync is not even setup in the content-translation for that field)

So and I did the fix with the double $definition variable.

nod_’s picture

This is NW from a JS point of view, will do a proper review over the weekend.

Gaelan’s picture

Issue tags: -Needs manual testing

It seems that the manual testing here is done.

nod_’s picture

FileSize
1.62 KB
5.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,044 pass(es). View

seems part of the patch was included already. Light changes to the JS, It's consistent with the rest of the file so I won't touch it further.

killua99’s picture

FileSize
6.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,135 pass(es). View
4.96 KB

This issue is getting bigger. Needs review and close the requirement for this one.

This patch sole an Notice PHP and also prevent the check for some translation_sync when is not selected yet.

Bojhan’s picture

Issue tags: -Needs usability review

Looks good to me

YesCT’s picture

Status: Needs review » Needs work

except it doesn't work all the way.

go to the content type article, manage fields, edit the image field, and click to enable the title (part of the image info).
should see the checkbox show for the title in the translation section, but it does not show.

http://screencast.com/t/DqeLj1xpql

[edit: removed comment about needing a test. since we cannot have javascript tests just yet.]

YesCT’s picture

@killua99 in #58

+++ b/core/modules/content_translation/content_translation.admin.inc
@@ -17,30 +17,37 @@
-      $options[$group] = $info['label'];
+      if (!empty($instance->settings[$group . '_field']) || (!empty($translation_sync) && array_key_exists($group, $translation_sync))) {
+        $options[$group] = $info['label'];
+      }

why was this change made?
why test the sync in the OR, and why use !empty instead of isset?

YesCT’s picture

Status: Needs work » Needs review
FileSize
1.34 KB
5.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,620 pass(es). View

ok. another problem with the previous patch was that file was missing from the translatable checkbox list. it should always show.

the change I made makes all the checkboxes show. good. now the javascript should hide based on if it's enabled or not. there is no disable on the file, so it should always be shown.

also, the javascript needs to work both on the manage field edit form, and the content language settings form.
if we need different javascript, maybe we need something different for each form?

it seems with this patch that the javascript is showing/hiding the title and alt ok on the manage field image field edit form ok. but it is hiding file, that is not ok.
and on the content/translation settings page, the javascript is not doing anything there. (not ok)

YesCT’s picture

Status: Needs review » Needs work

needs work to make the javascript work.

fran seva’s picture

FileSize
5.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch core-translation-1920876-72.patch. Unable to apply patch. See the log in the details link for more information. View
769 bytes

@YesCT this patch fix the problem with the checkbox File in content type field configuration [1].
File doesn't have an enable button so, in JS the $target variable is empty. To don't hide the file button I check it to don't apply the hide() function on the element.

[1] /admin/structure/types/manage/article/fields/node.article.field_image?destination=admin/structure/types/manage/article/fields/node.article.field_image

YesCT’s picture

Status: Needs work » Needs review
YesCT’s picture

Issue summary: View changes
Issue tags: -Novice

Also, definitely not novice. :)

YesCT’s picture

Assigned: javisr » Unassigned
fran seva’s picture

@YesCT I'm working in this issue :)

fran seva’s picture

@YesCT I've been reviewing the last patch and trying to get a general point of view of the current state of the issue. I have doubts about how should work...sorry :(.

I'm going to try to summarize (I also attach a mind map with all I have reviewed and what I've seen) but I can't promise...

There are two moments, one just after activate the content translation module (before save configuration) and other after configure and save the content translation, setting the article image field translate properties.

1. Before save configuration in admin/config/regional/content-language:

If we go to node.article.field_image

Checkboxes

- In Translatable Elements fieldset we have:
Checbox File: unchecked and visible
Checbox Alt: checked and visible
Checkbox Title: checked and hidden
- In the form we also have:
Checkbox Enable Alt field: Checked
Checkbox Enable Title field: Not checked

At this point, I have a question:
If content translation is not configure for Article content type, why is being showed in Translatable Elements fieldset?

In this form, the JS is working as expected:

If check Enalbe title field -->Translatable Elements title checkbox appears
If check Enable alt field --->Translatable Elements alt checkbox appears
If uncheck Enable title field--> Translatable Elements title checkbox is hidden
If uncheck Enable alt field --->Translatable Elements alt checkbox is hidden
In Translatable Elements, if we check File checkbox, Title and Alt are checked automatically.
Note: hidden is not unchecked, the submitted value for hidden checkboxes could be checked or unchecked.

Currently, there is just one relationship between Translate elements and enable checkboxes. That is, if we check one Enable checkbox (title or alt), the related translate checkbox appears. But, this not affect the translate checkbox value, I mean, if the enable title is checked and the Title checkbox in content translate is checked too, if we uncheck the enable title checkbox, the title checkbox will be hidden and in background will be already checked and if we submit the form, the value for this checkbox will be checked. I just wanna be sure this is the expected functionality.

What happend when the form is submitted?

If Translate Elment has Alt checked
If Translate Elment has Title checked
If Translate Elment has Alt and Title checked

In all cases the cotent translation configuration for article is updated.
At this point, I have the same question. If we don't have set the content translation configuration for Article, could we do this from this form?

2. After save configuration in content-language:

In admin/config/regional/content-language form:

Now,we have set the content translation for Article and we have the following configuration for Image field in Article
Image checkbox is checked
Image > Title checkbox is checked
Image > Alt checkbox is checked

What happen when we change the configuration (submit):

The expecte result is that node.article.field_image should be updated with the changes.
If uncheck Image > Title and image > alt ----> node.article.field_image will not show translatable elements
If uncheck Image > alt ----> alt checkbox in Translatable checkbox will be unchecked
If uncheck Image > title ----> Title checkbox in Translatable checbox will be unchecked

JS is working as expected

If Image > File checkbox is checked Title and Alt are checked and disabled
If Image > File checkbox is unchecked Title and Alt are enabled (and are still checked)
I think this is the correct functionality but I wonder if by default, in node.article.field_image, before save content-language configuration for article, the Translatable Elements shouldn't be in the form. This changes could implies check if there is set the article configuration in content-language instead get directly the default configuration.

jhedstrom’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

fran seva’s picture

Thanks @jhedstrom I'm on it :)

The last submitted patch, 72: core-translation-1920876-72.patch, failed testing.

rpayanm’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
6.33 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 1920876-83_0.patch. Unable to apply patch. See the log in the details link for more information. View

rerolled

fran seva’s picture

Status: Needs review » Needs work

@YesCT I'm not sure about how the translation configuration should work.

YesCT’s picture

Issue summary: View changes
Issue tags: +accessibility
Related issues: +#2232271: [Meta] Use Behat for validation testing
FileSize
405.48 KB
296.21 KB

updated issue summary with steps to test.

I recommend we do not work on this bug until we have frontend tests.

Caution, this is not a simple issue to work on, its category of "normal" is not the difficulty, but a measure of its impact.
https://www.drupal.org/core/issue-priority

fran seva’s picture

Assigned: Unassigned » fran seva

I'm working in behat test

fran seva’s picture

Issue tags: +drupaldevdays
omar’s picture

In case this is useful, while working on another issue I identified some behaviours that may have contributed to the difficulties encountered with this ticket.

1) The javascript on the admin/config/regional/content-language form will check-all/un-check all the fields (except the "file" field) if/when the checkboxes for bundles are toggled... so while they load correctly to begin with, toggling may result in fields not representing their true "translatable" status (see #2492541: Toggling translatability on admin page shouldn't forget settings)
2) Saving the admin/config/regional/content-language form unexpectedly disables translation for all the (hidden) fields of the bundles for which translation is currently disabled. (see #2240139: Inconsistant behavior when enabling Content Translation) This leads to inconsistent/unexpected behaviour.

mgifford queued 83: 1920876-83.patch for re-testing.

The last submitted patch, 83: 1920876-83.patch, failed testing.

mparker17’s picture

Assigned: fran seva » mparker17
Issue tags: +Needs reroll

Reassigning as no progress for 4 months. Patch no longer applies, marking as needing re-roll.

mparker17’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
6.74 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,962 pass(es). View

Straight re-roll, so no interdiff.

mparker17’s picture

Issue summary: View changes

Clarify issue summary Problem/Motivation, Proposed resolution, Remaining tasks.

mparker17’s picture

Issue summary: View changes

Clarify issue summary User interface changes, API changes.

mparker17’s picture

Issue summary: View changes
Issue tags:

To clarify the accessibility tag, we need to know whether it's necessary to announce to screenreaders when these checkboxes are shown/hidden. @mgifford?


Updating the issue summary to disseminate some of the extra information to where it makes a bit more sense contextually.

mparker17’s picture

Issue summary: View changes

@joelpittet pointed out #825436: Create selenium-RC PIFR plugin for full functional testing as a possible blocker in IRC. Thanks Joël!

mparker17’s picture

Issue summary: View changes

Clean up / compact steps to reproduce.

mparker17’s picture

Status: Needs review » Needs work

Great, the tests pass. But there are still some things to be done (see issue summary). Back to needs work, I'll see what I can do... I've been wanting to try my hand at Behat tests for a while, so this should be good practice.

mparker17’s picture

mgifford’s picture

In my manual tests the disabled fields are shown, although the text "Disabled fields are not shown. Please first enable the fields you need to make translatable." is now visible.

I just did a quick test by enabling the appropriate language modules and then going to https://dfc31.ply.st/admin/structure/types/manage/article/fields/node.ar...

content type config

Going here with the alt/title disabled still showed translation an option -
https://dfc31.ply.st/admin/config/regional/content-language

content language config page

The former's more important as far as improved usability goes..

Also the image in the issue summary needs to change.

mgifford’s picture

Issue summary: View changes
mgifford’s picture

@mparker17 - "To clarify the accessibility tag, we need to know whether it's necessary to announce to screenreaders when these checkboxes are shown/hidden"

I didn't follow up on this, but yes. Certainly when the alt tag & title are enabled it would be important to announce that new material is available on the screen. It's important for discoverability.

mparker17’s picture

Issue summary: View changes

Updating issue summary.

mgifford’s picture

Assigned: mparker17 » Unassigned

If you've got time Matt...

kgoel’s picture

FileSize
533 bytes

I wrote behat test for the following scenario. I used https://www.drupal.org/project/drupalextension for Drupal integration with behat. Behat Drupal Extension module doesn't seem to work with user role and drush. For this test, I gave anonymous user permissions. Documentation for setting up Behat Drupal Extension can be found - https://behat-drupal-extension.readthedocs.org/en/3.1/localinstall.html.

Go to admin/structure/types/manage/article/fields/node.article.field_image.
There is a checkbox labeled "Enable Alt field". It is checked by default.

kgoel’s picture

FileSize
587 bytes

Updated version of test using Drupal API driver for the coverage of ....
Go to admin/structure/types/manage/article/fields/node.article.field_image.
There is a checkbox labeled "Enable Alt field". It is checked by default.

kgoel’s picture

FileSize
3.9 KB

Behat test for testing scenario 4,5 and 6 are covered in the attached test.

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.