This is a follow-up to #1188388: Entity translation UI in core.

Problem/Motivation

Currently enabling translation for a particular bundle requires going down three level of configuration: entity type/bundle/field. And most of them have the wrong default values in place, which means that every time a new entity/bundle is added all non-shared fields need to be enabled for translation. Moreover these settings are split accross different configuration pages, which makes almost impossibile for a user not knowing the required workflow to correclty setup translation without using the help section.

Another non-UX related problem with the current approach is that it requires every entity to provide its own configuration UI, otherwise even if an entity is supported out of the box by ET, it will not possibile to translate it because there is no way to enable translation for it.

Proposed resolution

Implement a centralized workflow allowing to setup all the three level of configuration for any defined entity type and bundle and put sensible defaults in place, mainly make fields translatable by default. We agreed to start from the current D7 configuration page, add the field configuration level to it and improve the overall UX.
We want to exploit this UI to configure also language settings leveraging what already implemented in #1739928: Generalize language configuration on content types to apply to terms and other entities.

This works with language enabled alone or also with entity translation.

From #184
with entity translation enabled
wiz-s01-Content language settings | 8.x-wizard 2012-12-28 20-51-58.png
and
without entity translation enabled
wiz-s03-notrans-Content language settings | 8.x-wizard 2012-12-28 21-02-46.png

Original screenshots in #8.

Remaining tasks

  • Provide a set of mockups describing how the new UI should work
  • Implement them
  • Provide test coverage
  • Follow-ups opened
  • use label for= for accesssibility
  • code review
  • repurpose #1855036: ARIA and accessibility improvements in entity (content) translation settings page for ARIA
  • (maybe) open Q2 "Field UI formatter settings model" follow=up
  • identify any missing follow-ups
  • put new patch on server for accessibility review(not needed, and see the ARIA followup)
  • (feedback gotten) Accessibility review. See #119, #71(see the ARIA followup)

User interface changes

Only additions.

API changes

No particular API change foreseen.

Follow-ups

Related Issues

Background cut and pasted from et ui part 2

Global settings page for all translatable entities, alongside the per-entity ones. Follow-up #1810386: Create workflow to setup multilingual for entity types, bundles and fields opened based on #81 Currently the implementation of the entity translatability setting is a bit hacky, we need to clean it up now that #1739928: Generalize language configuration on content types to apply to terms and other entities landed. This might be done here and not part of a follow-up.

CommentFileSizeAuthor
#184 wiz-s01-Content language settings | 8.x-wizard 2012-12-28 20-51-58.png90.88 KBYesCT
#184 wis-s02-type-2012-12-28_2054.png145.18 KBYesCT
#184 wiz-s03-notrans-Content language settings | 8.x-wizard 2012-12-28 21-02-46.png68.18 KBYesCT
#183 et-wizard-1810386-183.interdiff.do_not_test.patch2.35 KBplach
#183 et-wizard-1810386-183.patch42.1 KBplach
#170 et-wizard-1810386-170.interdiff.do_not_test.patch5.67 KBplach
#170 et-wizard-1810386-170.patch41.62 KBplach
#165 remove_the_language-content-settings-form_when_only_one_language.png70.66 KBklonos
#164 et-wizard-1810386-164.patch42.13 KBYesCT
#164 interdiff-159-164.txt9.42 KBYesCT
#164 wiz-s01-custom_language_settings-2012-12-20_0539.png104.03 KBYesCT
#159 et-wizard-1810386-159.interdiff.do_not_test.patch10.35 KBplach
#159 et-wizard-1810386-159.patch40.98 KBplach
#159 et-wizard-1810386-159.png23.31 KBplach
#154 et-wizard-1810386-154.patch41.86 KBYesCT
#154 interdiff-150-154.txt1.74 KBYesCT
#156 Content language settings | wizard-154 2012-12-19 01-22-32.png142.34 KBYesCT
#150 et-wizard-1810386-150.patch41.88 KBplach
#148 et-wizard-1810386-148.interdiff.do_not_test.patch3.19 KBplach
#148 et-wizard-1810386-148.patch36.21 KBplach
#145 et-wizard-1810386-145.interdiff.do_not_test.patch8.06 KBplach
#145 et-wizard-1810386-145.patch36.21 KBplach
#138 LanguageSettings.png180.76 KBGábor Hojtsy
#124 et-wizard-1810386-124.patch31.29 KBYesCT
#124 interdiff-121-124.txt1.06 KBYesCT
#121 etw-s01-Content translation settings | et-wizard-121 2012-12-10 22-07-49.png108 KBYesCT
#121 et-wizard-1810386-121.patch31.29 KBYesCT
#121 interdiff-104-121.txt3.61 KBYesCT
#110 NewWizarUI.jpg88.61 KBGábor Hojtsy
#104 et-wizard-1810386-104.patch31.69 KBYesCT
#104 interdiff-93-104.txt4 KBYesCT
#93 et-wizard-1810386-93.patch31.08 KBYesCT
#93 interdiff-92-93.txt822 bytesYesCT
#92 et-wizard-1810386-92.patch31.04 KBYesCT
#92 interdiff-84-92.txt2.14 KBYesCT
#90 et-w-a-s01-2012-11-29_0517.png114.82 KBYesCT
#88 et-w-s01-Content translation settings | et-wizard-83 2012-11-29 04-16-49.png99.14 KBYesCT
#88 et-wizard-1810386-84.patch30.84 KBYesCT
#88 interdiff-83-84.txt2.61 KBYesCT
#83 et-wizard-1810386-83.patch31 KBYesCT
#83 interdiff-73-83.txt3.76 KBYesCT
#79 w-s01-permissions-2012-11-28_2127.png159.14 KBYesCT
#79 w-s02-modules-2012-11-28_2206.png130.43 KBYesCT
#79 w-s03-users-2012-11-28_2218.png108.85 KBYesCT
#79 w-s05-blocks-changes-2012-11-28_2229.png93.55 KBYesCT
#79 w-s06-extend-2012-11-28_2236.png124.47 KBYesCT
#79 w-s07-confirm-2012-11-28_2240.png74.78 KBYesCT
#79 w-s08-trans-ui-2012-11-29_0011.png103.76 KBYesCT
#79 w-s09-html-changed-2012-11-29_0016.png214.32 KBYesCT
#79 w-s04-blocks-2012-11-28_2223.png140.64 KBYesCT
#79 w-s10-losing-labels-2012-11-29_0058.png142.56 KBYesCT
#76 et-ui-mock.png46.6 KBwebchick
#73 et-wizard-1810386-73.patch28.34 KBbforchhammer
#73 et-wizard-1810386-73.interdiff.do_not_test.patch11.19 KBbforchhammer
#70 et-configure.png64.55 KBwebchick
#70 et-extra-cols.png71.08 KBwebchick
#65 et-wizard-1810386-65.interdiff.do_not_test.patch918 bytesplach
#65 et-wizard-1810386-65.patch26.87 KBplach
#63 et-wizard-1810386-63.patch26.87 KBYesCT
#63 interdiff-59-63.txt901 bytesYesCT
#63 workflow-bundle_label-Content translation settings | workflow-again-59 2012-11-23 04-04-48.png78.22 KBYesCT
#62 interdiff-59-62.txt2.65 KBYesCT
#61 workflow-s00-bundle_label-2012-11-23_0221.png73.16 KBYesCT
#61 workflow-s01-after_enable-2012-11-23_0112.png120.06 KBYesCT
#61 workflow-s01b-config_on_module-2012-11-23_0115.png74.02 KBYesCT
#61 workflow-s01c-config_on_config-2012-11-23_0116.png98.19 KBYesCT
#61 workflow-s02-config-2012-11-23_0120.png75.86 KBYesCT
#61 workflow-s02b-node-2012-11-23_0122.png71.32 KBYesCT
#61 workflow-s02c-tax-2012-11-23_0129.png60.68 KBYesCT
#61 workflow-s03a-article-2012-11-23_0131.png81.01 KBYesCT
#61 wordflow-s03b-morechanges-2012-11-23_0134.png96.69 KBYesCT
#61 workflow-s04a-check-article-2012-11-23_0139.png107.22 KBYesCT
#61 workflow-s04b-check-body-2012-11-23_0140.png47.29 KBYesCT
#61 workflow-s04c-check-body-enabled-2012-11-23_0140.png57.7 KBYesCT
#61 workflow-s04d-check-image-not-2012-11-23_0141.png94.18 KBYesCT
#61 workflow-s05-permissions-2012-11-23_0145.png103.51 KBYesCT
#61 workflow-s06a-disable-article-2012-11-23_0147.png76.5 KBYesCT
#61 workflow-s06b-disabled-article-image-shows-trans-2012-11-23_0154.png105.81 KBYesCT
#61 workflow-s06c-try-uncheck-node-2012-11-23_0157.png79.86 KBYesCT
#59 et-wizard-1810386-59.interdiff.do_not_test.patch12.39 KBplach
#59 et-wizard-1810386-59.patch26.88 KBplach
#46 et-wizard-1810386-46.interdiff.do_not_test.patch6.38 KBplach
#46 et-wizard-1810386-46.patch24.6 KBplach
#45 instance-s01-2012-11-20_1430.png109.22 KBYesCT
#34 Settings1.jpg32.49 KBGábor Hojtsy
#34 Settings2.jpg56.9 KBGábor Hojtsy
#34 Settings3.jpg63.99 KBGábor Hojtsy
#31 et-wizard-1810386-31.patch20.02 KBplach
#30 et-wizard-1810386-30.patch20.01 KBplach
#19 Install_translatability_multiple_entities_at_once2.png56.37 KBBojhan
#18 VocabSettings.jpg57.94 KBGábor Hojtsy
#15 Install_translatability_multiple_entities_at_once.png44.01 KBBojhan
#15 Configure_translatability_per_entity.png47.41 KBBojhan
#8 et-config_ui-1810386-8.png54.11 KBplach
#8 et-config_ui-step1-1810386-8.png34.55 KBplach
#8 et-config_ui-step1_2-1810386-8.png37.67 KBplach
#8 et-config_ui-step3-1810386-8.png30.34 KBplach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT’s picture

Status: Postponed » Active

Since #1739928: Generalize language configuration on content types to apply to terms and other entities was committed (it might have a little work done on it still, but the main stuff is committed), this might be done as a part of #1188388: Entity translation UI in core directly and not as a follow-up.

plach’s picture

Title: clean up implementation of entity translatability setting [Follow-up to Entity Translation UI in core] » Provide a centralized workflow to enable translatability for entity types, bundles and fields
Component: language system » translation_entity.module
Issue tags: +Needs issue summary update, +feature freeze
plach’s picture

Priority: Normal » Major
Gábor Hojtsy’s picture

Issue tags: +sprint

Marking it for the sprint.

plach’s picture

Issue tags: +Usability
ytsurk’s picture

i would love to see a bulk update functionality, f.e. for the Users may translate this field bool for the fields on a contenttype.

plach’s picture

Title: Provide a centralized workflow to enable translatability for entity types, bundles and fields » Provide a centralized workflow to configure language setting and enable translatability for entity types, bundles and fields
plach’s picture

Issue summary: View changes

Created issue summary

plach’s picture

Bojhan’s picture

Assigned: Unassigned » Bojhan

I will work on this.

Bojhan’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Title: Provide a centralized workflow to configure language setting and enable translatability for entity types, bundles and fields » Provide a centralized workflow to configure language settings and enable translatability for entity types, bundles and fields
Bojhan’s picture

Title: Provide a centralized workflow to configure language settings and enable translatability for entity types, bundles and fields » Create workflow to setup multilingual for entity types, bundles and fields
  • General settings
    • - Per entity settings
      • - Per bundle settings
        • - Per field settings

Noting down thoughts; 1) States to toggle bundles. 2) headers for bundle, 3) table for field settings. Almost feels like its going towards field UI inline form and views combo.

Bojhan’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

Title: Create workflow to setup multilingual for entity types, bundles and fields » Create workflow (wizard) to setup multilingual for entity types, bundles and fields
plach’s picture

Title: Create workflow (wizard) to setup multilingual for entity types, bundles and fields » Create workflow to setup multilingual for entity types, bundles and fields

We agreed with Bojhan this won't be a wizard :)

YesCT’s picture

That's cool. :)

Bojhan’s picture

Alright, I am going to start of with a crazy idea :) I worked really hard trying to figure out how to get all of this into nice vertical tabs and fieldsets, but the more and more I got into it the more and more I started to find it didn't work. Because it doesn't scale and introduces loads and loads of visual elements.

What if you select a entity, and it just automatically enables translation for all its children and grandchildren, for example you enable you want translation for "Content" that automatically enables it for all its Content types and fields. You can then go on a per case business, if you want to disable it. This should significantly speed setup time and simply go with smart defaults out of the box.

Tables to rule them all

When you select "Content" it spawns a table, with all the content types and fields below. For each entity a table is added, and populated with smart defaults for all its parts. I am not yet sure what "Comments" or "User" will have in them. Could we have screenshots for these?

* Small bug I meant to also select, taxonomy term below.
Install_translatability_multiple_entities_at_once.png

Modal dialog to configure one

With modal dialogs going into core this might be a prefect place to start using them. However I am not yet betting on them being available for us, on these kind of screens - so we can go with the off the page, to return to listing pattern too (like in field UI configuration).

Below, shows what happens if you click "Configure" on the Article row.
Configure_translatability_per_entity.png

Just wanted to get this out there, so we can see what complexities come up. The concerns I see now, is that this would be a new pattern and we could have usability issues with people forgetting to Save especially as they go of to edit one. Perhaps we can introduce a "Views" like local saving here.

Bojhan’s picture

Assigned: Bojhan » Unassigned
Bojhan’s picture

Issue summary: View changes

Updated issue summary added background so not to lose information from the et-ui follow-up process.

Gábor Hojtsy’s picture

Reviewing #15:

- If you have many content types / vocabularies, that could easily push down the Save button and made it hard to see it is even there
- Enabling all content types right away might be too much, but this UI should give you a quick way to turn them off
- How do you turn on/off translatability in this form/table? It is not on the configure modal and it would be painful to need to turn off one by one like that.
- What happens when you add a new content type/vocabulary, etc. later? You are doing that independent of this UI, and that UI you are doing it you don't have a quick way to turn things on/off, so assuming all disabled or enabled for translation might equally be problematic then. This UI plan here kind of assumes you built out *all* your content types and vocabularies before you came here, but that is not what happens on an iteratively built site (IMHO 80%).
- I think your "taxonomy terms" area is flawed, there are *per vocabulary* translatability settings and terms are just instances. An analogy would be if you'd listed actual nodes in the content listing. Terms are instances of a type of term which are organized into vocabularies. So field setup and translatability are per vocabulary not per term.
- What would be under global settings?
- As for the modal, the "hide language neutral" thing we even have an issue for at #1314250: Allow filtering/configuration of which languages apply to what (UI, nodes, files, etc) - so I'm happy this is in consideration as a UI cleanup possibility :) there are just some things were (not all) special languages apply; I think the content of the modal is just pulled from the D7 settings, just noting we need a bit more here IMHO

Gábor Hojtsy’s picture

FileSize
57.94 KB

Also, one more thing. In Drupal 7 it is very confusing that the base language settings are on the content type, taxonomy vocabulary, etc. BUT there are advanced settings that are tucked elsewhere, so if you find this screen, you can do advanced settings but no basic settings and vice versa. I think whatever settings are exposed here on the configure modals, those should be available on the content type as well. Eg. the default language one depicted here is already in D8 as a base content type / vocabulary, etc. setting.

So this same settings structure is present on content types, vocabularies, etc.
VocabSettings.jpg

I think this "wizard" screen should only include settings available elsewhere.

Bojhan’s picture

- If you have many content types / vocabularies, that could easily push down the Save button and made it hard to see it is even there

Yes, I considered this but given that its a "whole setup" form, it should be likely they scroll down to see everything.

- Enabling all content types right away might be too much, but this UI should give you a quick way to turn them off

You are right, I introduced checkboxes. These should have some *magic* to allow you to disable children.

- How do you turn on/off translatability in this form/table? It is not on the configure modal and it would be painful to need to turn off one by one like that.

Is this similar to checkboxes or should I introduce something more?

- What happens when you add a new content type/vocabulary, etc. later? You are doing that independent of this UI, and that UI you are doing it you don't have a quick way to turn things on/off, so assuming all disabled or enabled for translation might equally be problematic then. This UI plan here kind of assumes you built out *all* your content types and vocabularies before you came here, but that is not what happens on an iteratively built site (IMHO 80%).

That is something, I am not sure how to tackle yet. Perhaps after they see this form once, it allows you to toggle between "new" and "old" setups? Also wondering if this 80% should be tackled in the setup of these content,vocabulary,term types -e.g. enabled by default maybe? Since that is the more natural flow. This is really only tackling the "setup phase", when you enable translation and want it to work for menu's, blocks, vocabs, terms etc.

- I think your "taxonomy terms" area is flawed, there are *per vocabulary* translatability settings and terms are just instances. An analogy would be if you'd listed actual nodes in the content listing. Terms are instances of a type of term which are organized into vocabularies. So field setup and translatability are per vocabulary not per term.

Changed it to vocabulary.

- What would be under global settings?

Added examples.

- As for the modal, the "hide language neutral" thing we even have an issue for at #1314250: Allow filtering/configuration of which languages apply to what (UI, nodes, files, etc) - so I'm happy this is in consideration as a UI cleanup possibility :) there are just some things were (not all) special languages apply; I think the content of the modal is just pulled from the D7 settings, just noting we need a bit more here IMHO

I would love to incorporate whatever settings there need to be here, I did indeed just look at D7.

Also, one more thing. In Drupal 7 it is very confusing that the base language settings are on the content type, taxonomy vocabulary, etc. BUT there are advanced settings that are tucked elsewhere, so if you find this screen, you can do advanced settings but no basic settings and vice versa. I think whatever settings are exposed here on the configure modals, those should be available on the content type as well. Eg. the default language one depicted here is already in D8 as a base content type / vocabulary, etc. setting.

I would like to split that issue of, from this. This seems to be about having a consistent place/model for configuring a entity. Clearly that should not be across tabs, but instead on the primary "edit" screen. We clearly made a mistake in D7 not giving that a singular place.

Install_translatability_multiple_entities_at_once2.png

Let me know if this is a good direction, and enough to go from.

Gábor Hojtsy’s picture

Yes, I agree this looks like a good direction. It will need some magic, so eg. if you enable a field for translation, it marks all other uses of the field for translation as well for example. Or it would not let you translation-enable fields until the parent entity bundle is translation enabled. Also terms can have fields as well, but that is easy to imagine being on the screen, so no need to create yet another version IMHO :)

plach’s picture

I think there is a problem with the current proposal: we have a configure link for each field row, which is pretty confusing because the settings in the modal apply at entity type/bundle level. Unless the configure link points to the field edit page, but this would imply an inconsistent UX IMHO.

plach’s picture

Also, what about using a vertical tab for each entity type table to save some vertical space. Or a fieldset which would be collapsed for each already configured entity type?

Bojhan’s picture

@plach No, the modal refers to only the translation part of each thing; e.g. the translation settings for a content type, the translations settings for a field.

I do not want to use vertical tabs - since this is all the primary interaction, we could potentially hide using collapsed fieldsets. But it is exactly those patterns, I am trying to avoid because rather than making this more usable it instead creates a overwhelming effect of different groupings.

plach’s picture

No, the modal refers to only the translation part of each thing; e.g. the translation settings for a content type, the translations settings for a field.

But the translatability settings for most fields (99,9%) will be just the checkbox to enable/disable it: the configure link + modal dialog in that case are nearly useless IMHO.

Gábor Hojtsy’s picture

@plach: so ignore those configure links :)

Bojhan’s picture

Yes, totally ignore them then :D

plach’s picture

Assigned: Unassigned » plach

Working on this.

YesCT’s picture

One of the things I remember from et-ui was that when enabling translation on a field that had data, the batch process started and it had a warning. In the warning is says something like: caution this will effect everywhere the field is used. But it does not tell you where else the field is used. With a field with no data, it was just a checkbox and the action went directly.

Gabor mentions this in #20 and I think being able to see here where the same field is used in other content types (entities) would help a lot. Maybe something like the modules Extend page, where it lists in small text the dependency relationship between modules. This could list the other entities (which content-types, vocabs, tags ... user I think is just user) use the same field. To the best of my knowledge that information is not anywhere else. (for example on a field edit tab, even in the global settings, it does not tell you what else is also using that same tab).

If we find a way to know that info in order to use it here with regards to translations, it would be nice to make that separate and re-usable. Because I can see it being useful on that field edit global settings and the warning message ... and there may be some other places too.

What about a link/box for "enable everything possible" and "disable everything"?

plach’s picture

A quick update: I'm mostly done with the basic stuff, tomorrow I'll work on the front-end magic and hopefully post a first draft for review.

plach’s picture

Status: Active » Needs review
FileSize
20.01 KB

And here is the first patch, it already looks fairly functional to me. It's a bit late for screenshots, but it's pretty close to @Bojhan's mockups. The main difference is that, since we have no modal dialogs yet, the language settings widget is shown instead of the "configure" link. We can totally fix this after feature freeze if we cannot do it earlier.

plach’s picture

FileSize
20.02 KB

Forgot to complete a comment.

plach’s picture

Gábor Hojtsy’s picture

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -157,12 +160,20 @@ function translation_entity_menu() {
+  $items['admin/config/regional/translation_entity'] = array(
+    'title' => 'Content translation settings',

Now would be the time to add this as a config link in the .info file? :)

Gábor Hojtsy’s picture

FileSize
63.99 KB
56.9 KB
32.49 KB

Screenshots!

Default state of the screen, nothing is translatable:
Settings1.jpg

After I picked nodes and taxonomy terms:
Settings2.jpg

After I picked the article content type to make translatable (it defaulted to make all checkboxes checked too):
Settings3.jpg
(This is where you can see the embedded settings form that plach talked about).

The settings also properly worked. When I went to save this form I got an error because I selected taxonomy terms but then did not set any of the vocabularies to be translatable, so it did not make sense. So I deselected that. Then saving the form, my article type and the fields got translatable. Cross-checked with settings on the content type.

Gábor Hojtsy’s picture

Discussed this with @Bojhan on IRC and both of us were wondering why it is called a "Node". We avoid this terminology as much as possible on the UI. We call it "Content" even in obscure corners of views :) The only related code I found was this below, so I assume the "Node" terminology comes from the entity metainfo system. Can we somehow patch it there or alter it for display here so that it says the more friendly "Content"?

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/Core/Entity/Comment.phpundefined
@@ -35,6 +35,9 @@
+ *   bundle_keys = {
+ *     "label" = "Node type"
+ *   },

+++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Entity/Node.phpundefined
@@ -37,7 +37,8 @@
- *     "bundle" = "type"
+ *     "bundle" = "type",
+ *     "label" = "Content type"

Let's call it Content type in both cases. That is the human facing name. :)

plach’s picture

Assigned: plach » Unassigned

Now would be the time to add this as a config link in the .info file? :)

+++ b/core/modules/translation_entity/translation_entity.info
@@ -4,3 +4,4 @@ dependencies[] = language
+configure = admin/config/regional/translation_entity

It's already there :)

Discussed this with @Bojhan on IRC and both of us were wondering why it is called a "Node". We avoid this terminology as much as possible on the UI. We call it "Content" even in obscure corners of views :) The only related code I found was this below, so I assume the "Node" terminology comes from the entity metainfo system. Can we somehow patch it there or alter it for display here so that it says the more friendly "Content"?

I know we should be calling nodes "Content", fact is that as you are correctly pointing out we'd need to change the human-readable entity type label in the entity information. This change is pretty far-reaching although being rather easy to implement, I was wondering whether it would deserve a dedicated issue where exploring the possibile side effects.

Let's call it Content type in both cases. That is the human facing name. :)

Ok!

I'll fix the things above and work on test coverage tonight, if none beats me to it before. Then we should be mostly done, unless someone has complaints about code.

The only UX aspect I'm not totally happy about right now is that field translatability is a per-field setting, hence if any instance is marked as translatable, all of them will be checked after saving, even if they were unchecked on submit. A possibile fix is adding some JS magic to synchronize the checkbox status among instances belonging to the same field, maybe using a fading-out background color to highlight the affected rows for a moment, but I think this is telling us we just need to make field translatability an instance setting.

Gábor Hojtsy’s picture

Well, it would definitely be great to make it an instance setting (in another issue). Would that be very hard? Ie. should we focus on this JS magic instead of making it an instance setting?

plach’s picture

From the top of my head I don't see big troubles ahead, but the devil is in the details :)
However if we decide to go this way I think we can work on this after the feature freeze, right?

Btw, what about "Node" vs "Content"?
(Just realized the page title is "Content translation settings")

Gábor Hojtsy’s picture

Yeah, I think the "node" vs. "content" question gets interesting when we also need to refer to the collection of entity types, because we also kind of call them content overall (re page title). This might be the first cross-entity type feature that needs a name for the concept, although maybe Views beat us to that :) Maybe Views has some clues as to how to refer to entities as a concept in a user friendly way? If we call the whole thing content translation, then you configure things under it as "content", "user", etc. that is confusing. Are users content? :D "Content" is definitely "content" :) @Bojhan?

plach’s picture

I think here we are dealing with (translation of) content attached to entites: users are not content but they have content that can be translated. Nodes are, well, content with attached content :D

andypost’s picture

#34 is great, I think for field settings better to follow the same UI-patter as used for text-formats VT

Gábor Hojtsy’s picture

@plach: I never thought of "content" like that, even though it kind of makes sense that is not how we use "content" in core.

@andypost: is that a suggestion for change? Can you provide a screenshot to explain?

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

I think we sort out what to replace node with later, or just override it in this one place to be Content-types. Since this whole page is content translation settings... We know better than to call it *entity* translation settings. but having one of the content types be Content type is still awkward. So I'm ok with postponing that particular discussion.

There is no "select all". Is that on purpose or reserved for evaluation of whether it's even a good idea later?

Also, there is no setting to: make new types translateable by default.. Maybe because of the conflict Gabor found (that the form could not be saved while the tag field was checked but the vocabulary was not) perhaps that is some magic to be added later: that when check a type, all the fields that can have translation enabled are shown, and right now checked. But some of them might should not be checked. The tag field check box should be disabled and a red? note that checking the tags vocabulary as translatable is required/a dependency? (I'm on phone. Can upload a screenshot later if that would help)

I note not all the fields/properties are shown. For example, content types with the default title. This is a Good Thing, since they cannot have translation enabled.

Gábor Hojtsy’s picture

@YesCT: well, this UI provides "progressive disclosure" :) So first no settings are displayed, only checkboxes with entity type names. Once you pick entity types to translate, you get each bundle listed under their respective title. But there is only one row for each bundle. Then when you make a bundle translatable (such as a node type), you get checkboxes for each field (these are all automatically turned on). So the check-all feature is limited to the bundle level, and when you check an entity type, it does not apply to all bundles. I think that makes sense.

YesCT’s picture

in #36 @plach

..but I think this is telling us we just need to make field translatability an instance setting.

I admit I dont know exactly what "instance setting" is.
Would making it per instance move it out of the global settings?
instance-s01-2012-11-20_1430.png

This is in regards to fields that are shared across content types (maybe across entities in general). An example is that body is shared between Basic Page and Article.
Other settings that are not global, a quick look shows: label, help text, default value, required, input filter text processing
Other settings that are global: number of values

I'm betting this has something to do with the way the data is stored in the tables.

It might be that if people want to have, for example, a body on the Article that is translatable and a body on the Basic Page that is not translatable.. that they should not be sharing it across the content types?

This would effect some of the other issues, like expanding the global settings field set if translation is not enabled.. wait. we dont have an issue for that.. I think this is the one I must have been thinking of:
#1807902: add hint to field collapsed global settings: GLOBAL SETTINGS: Number of values (1), Field translation (disabled)

Do we know anyone who uses shared fields and does translation that we could ask about this? Do they normally want the field to either have translation enabled everywhere or not have it enabled everywhere? Or would they like to be able to control it per entity, uh, content-type, which would involve changing the setting everywhere when it is changed?

[edit: the word end in that screenshot should be "edit"]

plach’s picture

Here is the test coverage + minor improvements to the JS code and UX: now the language selector is automatically shown when making a bundle translatable.

plach’s picture

Category: task » feature

Since this is bound to the feature freeze I'd say this is a feature :)

Bojhan’s picture

Category: feature » task

Nope, its not. This is a significant usability problem that arguably almost renders it unusable to people starting out, not a "nice to have"

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Also, this is a centralised UI for existing features, so I agree it is not a feature request.

Opened #1847952: "Node" entity type not viable name for user presentation for the Node terminology problem that is not introduced by this patch but exposed by it.

I think this is as far as we can get here at first. Also given that we don't have more settings to configure per entity type, we might not want to have that in a modal at this point anyway, the inline settings form works.

The added tests look good, so let's get this improvement in. It will be much welcome!

plach’s picture

Assigned: Unassigned » webchick

I guess @webchick will want to have a look to this one.

plach’s picture

Issue summary: View changes

Updated issue summary.

bforchhammer’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Entity/Node.php
@@ -37,7 +37,8 @@
  *   bundle_keys = {
- *     "bundle" = "type"
+ *     "bundle" = "type",
+ *     "label" = "Content type"
  *   },

1. Does the "label" key belong into the "bundle keys" array? It's not really a key but static metadata. Maybe a top-level "bundle_label" key would be more fitting. (See hook_entity_info() for an overview).

+++ b/core/modules/translation_entity/translation_entity.admin.inc
@@ -8,6 +8,196 @@
+    '#attached' => array(
+      'css' => array(drupal_get_path('module', 'translation_entity') . '/translation_entity.admin.css'),
+      'js' => array(drupal_get_path('module', 'translation_entity') . '/translation_entity.admin.js'),
+    ),

2. (minor) CSS+JS are currently attached to the "list of entity types" instead of the table, or the form overall. (CSS actually only applies to the table, theme_translation_entity_admin_settings_table).

+++ b/core/modules/translation_entity/translation_entity.admin.inc
@@ -8,6 +8,196 @@
+          '#translation_entity_skip' => TRUE,

+++ b/core/modules/translation_entity/translation_entity.module
@@ -658,19 +689,20 @@ function translation_entity_enable_widget($entity_type, $bundle, array &$form, a
+  if (empty($element['#translation_entity_skip'])) {

3. I don't understand what the new "#translation_entity_skip" key on the "language_configuration" form element does... can we document this somewhere, maybe on the language_element_info() function (which defines the "language_configuration" element)?

+++ b/core/modules/translation_entity/translation_entity.admin.inc
@@ -8,6 +8,196 @@
+  // If an entity type is non translatable all its bundles and fields must be
+  // marked as non translatable. Similarly if a bundle is made non translatable
+  // all of its fields will be non translatable.

4. (minor) "non translatable" -> "not translatable"? Also, missing comma after "Similarly".

+++ b/core/modules/translation_entity/translation_entity.admin.inc
@@ -8,6 +8,196 @@
+    array('data' => t('Operations'), 'class' => array('operations')),

5. The "Operations" column currently does not show any operations but the settings form directly... rename respectively?

+++ b/core/modules/translation_entity/translation_entity.module
@@ -39,6 +39,9 @@ function translation_entity_help($path, $arg) {
+      return '<p>' . t('Setup your translation settings for all the different types on your website, this allows you to enable/disable and configure it at once. Newly created content types, vocabularies, etc will be added here.') . '</p>';

6. Help text looks like it could use some work... ;-)
- "types" sounds very vague, maybe better "types of content"?
- Use full-stop instead of comma in the middle of 1st sentence
- 2nd sentence doesn't make much sense to me.

+++ b/core/modules/translation_entity/translation_entity.module
@@ -394,9 +425,9 @@ function translation_entity_permission() {
-    'toggle field translatability' => array(
-      'title' => t('Toggle field translatability'),
-      'description' => t('Toggle translatability of fields performing a bulk update.'),
+    'administer entity translation' => array(
+      'title' => t('Administer entity translation'),
+      'description' => t('Configure translatability of entities and fields.'),

7. There's one more usage of the 'toggle field translatability' permission which also needs to be replaced, in translation_entity_form_field_ui_field_edit_form_alter() (~ l. 632).

+++ b/core/modules/translation_entity/translation_entity.module
@@ -715,3 +747,63 @@ function translation_entity_language_configuration_element_submit(array $form, a
+  foreach ($fields as $field_name => $translatable) {
+    $field = field_info_field($field_name);
+    $field['translatable'] = $translatable;
+    field_update_field($field);
+  }

8. This does not trigger the batch operation for changing field translatibility... the batch is currently only triggered when changing translatibility on the field settings page; the behavior should probably be the same in both places. When you disable translatibility directly on the field settings page, then you a) get a warning that it affects all fields, and b) translation values are deleted as well...

9. Also, during testing I had a case when the batch process for "disabling field-translatability" stopped completely and didn't finish. Reverting the patch fixed the issue; unfortunately I haven't been able to reproduce it since. The log showed the following notice: Notice: Undefined property: Drupal\node\Plugin\Core\Entity\Node::$original in file_field_update() (Line 248 of \core\modules\file\file.field.inc).

plach’s picture

Assigned: webchick » plach

Thanks :)

I'll fix the rest later, but let me answer to the last points:

This does not trigger the batch operation for changing field translatibility...

Once all entities are migrated to the new Entity Field API, we are dropping the migration batch completely because then fields will always be stored under the LANGUAGE_DEFAULT key, regardless of their translatability. So trying to adapt the migration code here would just be a waste of time.

Also, during testing I had a case when the batch process for "disabling field-translatability" stopped completely and didn't finish.

This is unrelated, it's just hard to reproduce: #1833108: disabling translation of fields with data stalls and does not work.

YesCT’s picture

Assigned: plach » webchick
Status: Needs work » Reviewed & tested by the community

This is so sweet!

Steps to test drive

  1. enable translation module under Extend
  2. configure translation (from the modules page, configure link, or under Configuration >> Regional and language >> Content translation settings)
  3. pick a couple types to enable translation on, like the tags vocabulary, and node page and article
  4. notice the defaults are better: translation enabled for fields were it is possible, and the default is NOT to hide the language selector.
  5. if desired, try something like unchecking the image field in article
  6. if desired, hide the language selector for basic page
  7. save settings.

Check it:

  • See that this worked by going to the entity edit pages under Structure. Cool! it did work!

There are some cool follow-ups we can make after this is in.

  1. magic when a checkbox of a shared field is changed it happens in the other places on that settings page, with some kind of visual indicator and words. See #28
  2. or instead, make translation an instance setting #36 and #45
  3. have the types (entity types) link to their settings under structure.
  4. a bit related: #1833104: Add a "translatable" column to Manage Fields will help when go to the entity type to see there what has translation enabled.
  5. reword the module help
  6. have a message when the module is enabled that adding a language is the next step
  7. have a link to the languages settings page from the top of this page (maybe only show.. if there is one non system language? and assume if they have more than one language then they know where to go to add more. because even after enabling translation here, after making content, it cannot be translated without an added language)
YesCT’s picture

Assigned: webchick » Unassigned

oops. cross posted with #51 and #52
I can't assign back to plach...

I did upload a video of testing this patch (setup 0-3:20), demo the patch (3:20-)
http://blip.tv/cathytheys/testing-drupal-d8mi-entity-translation-create-...
[edit: this link should be immediately available http://a1.video3.blip.tv/0340025772791/Cathytheys-testingDrupalD8miEntit...

sun’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -106,6 +106,7 @@
+ *   - label: The human-readable name of the entity bundles, e.g. Vocabulary.

I don't really understand why we need this new key in entity info — isn't this label the exact same label as is registered for the bundle entity types already?

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationSettingsTest.php
@@ -0,0 +1,98 @@
+    $this->profile = 'standard';

1) This should be defined as a regular class property.

2) Why do we need the Standard profile here? We actually want to stop using it and remove all test dependencies on it, instead of adding new ones...

+++ b/core/modules/translation_entity/translation_entity.admin.css
@@ -0,0 +1,20 @@
+  width: 25%

Missing semicolons.

+++ b/core/modules/translation_entity/translation_entity.admin.inc
@@ -8,6 +8,196 @@
+          '#translation_entity_skip' => TRUE,

Can we add some comments here and elsewhere to clarify what kind of effect this property has?

+++ b/core/modules/translation_entity/translation_entity.admin.inc
@@ -8,6 +8,196 @@
+  $element['#children'] = theme('table', array('header' => $header, 'rows' => $rows));
+
+  // We now need to render the parent element to ensure states are properly
+  // processed.
+  unset($element['#theme']);
+  unset($element['#theme_wrappers']);
+  return drupal_render($element);

Heh? ;)

You should be able to return this:

$output = theme('table', ...');
$output .= drupal_render_children($element);
return $output;

+++ b/core/modules/translation_entity/translation_entity.module
@@ -394,9 +425,9 @@ function translation_entity_permission() {
+    'administer entity translation' => array(
+      'title' => t('Administer entity translation'),
+      'description' => t('Configure translatability of entities and fields.'),
     ),

The description is more precise than the label, so we can drop the description and use it as label. :)

+++ b/core/modules/translation_entity/translation_entity.module
@@ -715,3 +747,63 @@ function translation_entity_language_configuration_element_submit(array $form, a
+function translation_entity_theme($existing, $type, $theme, $path) {

We can remove the extra arguments here.

plach’s picture

Assigned: Unassigned » plach

Working on this.

YesCT’s picture

potential followup 6 from #53 is already there...
You just added content translation capabilities to your site. To exploit them be sure to --link--enable at least two languages--endlink-- and enable translation for content types, taxonomy vocabularies, accounts and any other element whose content you wish to translate.
but it's flashing by for some reason. I'll look into it and open a separate issue if needed.

That potential follow-up can be replaced with a potential followup to make "enable translation", in that message, a link to this great workflow page.

YesCT’s picture

plach’s picture

Status: Needs work » Needs review
FileSize
26.88 KB
12.39 KB

The attached patch should address the reviews above:

9ee49db Issue #1810386: Refactored bundle label entity info.
b283b7f Issue #1810386: Attached resources to the $form array.
143cf5c Issue #1810386: Improved comments and readability.
c1d1555 Issue #1810386: Improved text.
3159720 Issue #1810386: Replaced old permission.
7775f12 Issue #1810386: Switched to the testing profile.
042ac41 Issue #1810386: Added missing semicolons to CSS.
99f224d Issue #1810386: Fixed entity table rendering code.

A couple of answers.

@bforchhammer:

The "Operations" column currently does not show any operations but the settings form directly... rename respectively?

As soon as we have modal dialogs we are planning to replace the embedded widget with a "configure" link (see the screenshots in the OP), this is why the column is named operations.

@sun:

Thanks for the review :)

I don't really understand why we need this new key in entity info — isn't this label the exact same label as is registered for the bundle entity types already?

Well, I wasn't sure we are guaranteed that a bundle will always be a configuration entity. Is this assumption correct? And do we have a generic API to leverage to retrieve the configuration entity type corresponding to a given bundle?

Heh? ;)

My bad, I initially tried something like what you suggested but it didn't work...

The description is more precise than the label, so we can drop the description and use it as label. :)

Please, bear with me, I like to have a title and a description, it's just aesthetics :)
Having just the title would made the ET permissions section look a bit unbalanced...

YesCT’s picture

patch in #59 passes the test bot (the test bot is just having trouble reporting back)
http://qa.drupal.org/pifr/test/390833

YesCT’s picture

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/Core/Entity/Comment.phpundefined
@@ -18,6 +18,7 @@
  *   label = @Translation("Comment"),
+ *   bundle_label = @Translation("Content type"),

This should be "Comment", not "Content type".. or just left using label default.

Probably a cut and paste thing. :)

+++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Entity/Node.phpundefined
@@ -18,6 +18,7 @@
  *   id = "node",
  *   label = @Translation("Node"),
+ *   bundle_label = @Translation("Content type"),

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Core/Entity/Term.phpundefined
@@ -18,6 +18,7 @@
  *   label = @Translation("Taxonomy term"),
+ *   bundle_label = @Translation("Vocabulary"),

but we should have seen comments as content type, and we did not.

Nor do nodes show up as Content type or taxonomy term as vocabulary.

check user account too. I dont see account getting a bundle_label set.

0. bundle_label

workflow-s00-bundle_label-2012-11-23_0221.png

Updated screenshots

other than that, i think everything is fixed.
here are updated screenshots showing the new wordings and other nice things.

1. after enable, message has config link

workflow-s01-after_enable-2012-11-23_0112.png

1b. also a config link on the modules page, where the help and permissions link is

workflow-s01b-config_on_module-2012-11-23_0115.png

1c. and a third way to get to the new workflow config page from Configuration!

(In fact, I had tested this patch a couple times and then later when testing something else I went to configure translation and though 8.x was broke because I could not find this handy link. Goes to show how intuitive it is to come to config to configure translation.)
workflow-s01c-config_on_config-2012-11-23_0116.png

2. initial configure page

workflow-s02-config-2012-11-23_0120.png

2b. pick node

workflow-s02b-node-2012-11-23_0122.png

2c. pick taxonomy

workflow-s02c-tax-2012-11-23_0129.png

3a. pick article

workflow-s03a-article-2012-11-23_0131.png

3b. pick more, change some of the defaults

wordflow-s03b-morechanges-2012-11-23_0134.png

4a. verify it worked article

workflow-s04a-check-article-2012-11-23_0139.png
See other attached s04 images to verify things were enabled or not correctly.
http://drupal.org/files/workflow-s04b-check-body-2012-11-23_0140.png
http://drupal.org/files/workflow-s04c-check-body-enabled-2012-11-23_0140...
http://drupal.org/files/workflow-s04d-check-image-not-2012-11-23_0141.png

5. permissions

workflow-s05-permissions-2012-11-23_0145.png

6a. disable article

I tested disabling, only with no translated content.
As mentioned earlier, batch disabling is not triggered. So I knew that. It will be handled later as mentioned in #52
Disabling does work otherwise here.
workflow-s06a-disable-article-2012-11-23_0147.png

6b. image field still disabled

I was perplexed why I was seeing translation settings for fields when the content type had translation disabled. So I checked 8.x without this patch and the same behavior is there. So maybe open a followup. But does not need to be handled in this issue.
workflow-s06b-disabled-article-image-shows-trans-2012-11-23_0154.png

6c. also tried disabling whole entity types

That worked too.
workflow-s06c-try-uncheck-node-2012-11-23_0157.png

YesCT’s picture

Status: Needs work » Needs review
FileSize
2.65 KB

I was not sure if bundle_label was left off of user, so I added it in.

I fixed the bundle_key to use the bundle_label

Note that right now, the list of check boxes uses just the "label"

function translation_entity_admin_settings_form(array $form, array $form_state, array $supported) {
  $entity_info = entity_get_info();
  $labels = array();
  $default = array();

  foreach ($supported as $entity_type) {
    $labels[$entity_type] = isset($entity_info[$entity_type]['label']) ? $entity_info[$entity_type]['label'] : $entity_type['id'];
    $default[$entity_type] = translation_entity_enabled($entity_type) ? $entity_type : FALSE;
  }

@plach... discussing this in irc with Gabor

I wont post the patch so as not to over-load the test bot but here is the interdiff in the mean time.

YesCT’s picture

plach’s picture

#63 looks good to me, thanks.

plach’s picture

I just realized CSS styles were not applied anymore. Don't know what happened. Now columns for all tables are lined up again as shown in #34.

plach’s picture

(bot green)

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Changes look good. AFAIS all review feedback was addressed.

plach’s picture

Assigned: plach » webchick

Back to @webchick's queue.

xjm’s picture

The patch could use a bit of docs polish. (Most of this would be fine as either a reroll or a followup. There is some missing parameter documentation, but only for a test helper method and a theme function where it's fairly clear anyway.)

  1. +++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationSettingsTest.phpundefined
    @@ -0,0 +1,102 @@
    + * Definition of Drupal\entity\Tests\EntityTranslationSettingsTest.
    

    "Contains" now rather than "Definition of".

  2. +++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationSettingsTest.phpundefined
    @@ -0,0 +1,102 @@
    +  /**
    +   * Overrides \Drupal\simpletest\WebTestBase::setUp().
    +   */
    

    Our coding standards specify excluding this docblock. It's completely inconsistent with anything else, but that's how it is. Reference: http://drupal.org/node/325974

  3. +++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationSettingsTest.phpundefined
    @@ -0,0 +1,102 @@
    +    // Setup two content types to test instances shared among different bundles.
    

    "Set up" should be two words here since it's a verb. The comment is also a little unclear. Don't be afraid to use a second line. :)

  4. +++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationSettingsTest.phpundefined
    @@ -0,0 +1,102 @@
    +  function testSettingsUI() {
    +    // Test that by marking only an entity type and no bundle as translatable a
    +    // form error is raised and the settings are not saved.
    ...
    +    // Test that by marking only a bundle and not the related entity type as
    +    // translatable the settings are ignored.
    ...
    +    // Test that by marking only a field as translatable and not the related
    +    // entity type and bundle the settings are ignored.
    ...
    +    // Test that by marking entity type and bundle as translatable the settings
    +    // are stored.
    

    These comments are a bit unclear. I'd reword these to put the important information first and try to shorten them for clarity, e.g., "Test that the translation settings are ignored if the bundle is marked translatable but the entity type is not."

  5. +++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationSettingsTest.phpundefined
    @@ -0,0 +1,102 @@
    +  /**
    +   * Asserts that translatability has the expected value for the given bundle.
    +   */
    +  protected function assertSettings($entity_type, $bundle, $enabled, $edit) {
    

    This needs parameter documetation.

  6. +++ b/core/modules/translation_entity/translation_entity.admin.incundefined
    @@ -8,6 +8,199 @@
    + * Administration settings page callback.
    

    Should be "Page callback: Returns the translation administration settings form." Reference: http://drupal.org/node/1354#menu-callback

  7. +++ b/core/modules/translation_entity/translation_entity.admin.incundefined
    @@ -8,6 +8,199 @@
    +/**
    + * Form builder for the administration settings form.
    + */
    +function translation_entity_admin_settings_form(array $form, array $form_state, array $supported) {
    

    This should have an @see reference to its validation and submission handlers. Also, I guess the standard form is "Form constructor" (not form builder).

  8. +++ b/core/modules/translation_entity/translation_entity.admin.incundefined
    @@ -8,6 +8,199 @@
    +/**
    + * Form validation handler for translation_entity_admin_settings_form().
    + */
    +function translation_entity_admin_settings_form_validate(array $form, array &$form_state) {
    ...
    +/**
    + * Form submission handler for translation_entity_admin_settings_form().
    + */
    +function translation_entity_admin_settings_form_submit(array $form, array &$form_state) {
    

    These should have @see references to each other. Reference: http://drupal.org/node/1354#forms

  9. +++ b/core/modules/translation_entity/translation_entity.admin.incundefined
    @@ -8,6 +8,199 @@
    +  // If an entity type is not translatable all its bundles and fields must be
    +  // marked as not translatable. Similarly, if a bundle is made non translatable
    +  // all of its fields will be non translatable.
    

    Really trivial: "non-translatable" should be hyphenated.

  10. +++ b/core/modules/translation_entity/translation_entity.admin.incundefined
    @@ -8,6 +8,199 @@
    +/**
    + * Theming function for an administration settings table.
    + */
    +function theme_translation_entity_admin_settings_table(array $variables) {
    

    I think this needs a bit more documentation, see: http://drupal.org/node/1354#themeable

  11. +++ b/core/modules/translation_entity/translation_entity.installundefined
    @@ -66,7 +66,8 @@ function translation_entity_install() {
    +  $message = t('You just added content translation capabilities to your site. To exploit them be sure to <a href="!language_url">enable at least two languages</a> and <a href="!settings_url">enable translation</a> for <em>content types</em>, <em>taxonomy vocabularies</em>, <em>accounts</em> and any other element whose content you wish to translate.', $t_args);
    

    This wasn't added here, but the word "exploit" has negative connotations. Should we say something more neutral, like "use"? Also, we're not supposed to use "we" or "you" in text, and "whose" is not correct for inanimate concepts. Maybe: "Content translation has been enabled. To use content translation, enable at least two langauges. and enable translation for content types, taxonomy vocabularies, accounts, or any element you wish to translate."

webchick’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Accessibility
FileSize
71.08 KB
64.55 KB

HOLY FRIGGING WOW THIS PATCH IS AMAZING!!!! Like, seriously. I can't get over how much easier this is compared to the way the UI was when the original ET patch went in. GREAT WORK!!!

Things that I love about this patch:

- Right after enabling the Entity Translation module, I can expand the section on the modules page and see an easy link to "Configure" the module without having to go hunting and pecking around looking for the settings, and eventually falling back to Help.
- It gives me a great overview of what things on my site are and are not translatable, just at a glance. This will be really helpful for troubleshooting problems.
- It makes things that can be translatable discoverable, and helps make sure I didn't miss anything.
- If I mark a bundle as translatable, then all of its fields are automatically marked translatable, with the option to shut it off on a per-field basis. Exactly the flow that I want.
- Unifies the configuration settings available across all entities so nodes aren't this weird one-off.
- Probably 100s of other things, but YAY YAY YAY.

I found a few minor things in going through:

1) When first selecting an entity type, the table below has some "phantom" columns in it. It might be nice to only add those once a bundle has been selected, if it's something easy to do.

Table initially shows rows for Fields and Operations, but no content inside them.

2) I don't think that column should be labeled "Operations" since it's not an operations column by the normal definition of the word (no drop-buttons, etc.). Maybe more like "Configuration"?

Table column heading says 'Operations' but what lies below are configuration settings about which language to default to and whether or not to show the language selector

3) It doesn't look like the help text under admin/help/translation_entity changed as a part of this patch, and probably should.

Then one major-ish thing... has this been tested in a screen-reader at all? :\

Let's get a re-roll for the docs changes and 1 & 2 if they're easy (if they're not, no worries, can be a follow-up). I'm happy to work with someone in an Etherpad for 15 mins to drastically simplify the admin/help* text (though that makes me sad, knowing how hard we worked on that, I'm *very* happy to see that vastly cut down). If it looks like it's going to take longer than 15-30 mins, then this can also be a follow-up since technically the text is correct right now, just not showing the optimal path. Accessibility, however, is a gate.

webchick’s picture

Issue summary: View changes

Updated issue summary.

falcon03’s picture

@mgifford ask me to review the patch; unfortunately I cannot do a complete review right now (I'll try to come back to this issue tomorrow), but here's some quick observations on the patch:

1. For consistency, I would expect the "translate" checkbox to be the first column (see the "enabled" column on the modules page);
2. The pattern of fields coming out in the table (where I won't expect to see them) sounds weird to me, although I have no idea how to improve this; just wondering if we can't assume that if a user enable translation for an entity bundle he wants all the fields attached to the bundle to be translatable. If ever he/she wants to make a field untranslatable, he/she can find the settings in the field instance; isn't it?

It seems obvious that using my suggested approach we would get rid of the "field" column..

Also: what should the "operations" column contain?

Let's work on this change; we need it to make entity translation much easier!

Bojhan’s picture

1. Although I would agree on consistency, visually the label is the landmark - which makes it much more logical to be placed after. Because we do indenting of fields, the check box would otherwise be too detached from the label. This is assuming there are more fields, than content types - which I think is a correct assumption. I think if this doesn't cause any accessibility issues, I would love to keep it in the 2nd column as it has a big impact on scanability.

2. This needs feedback from others, I assumed it is common to not toggle translatability for fields such as price and/or other numeric values.

bforchhammer’s picture

Rerolled the patch and fixed #53, #69 and #70 to the best of my abilities :)

Remaining issues (most of which can be follow-ups I think):

  1. Documentation for theme_translation_entity_admin_settings_table() could use some more work. I couldn't figure out how to properly document information on bundles (rows in the table) from element_children(). (see #69/10)
  2. Handling of fields shared between different bundles: either add explanation about what happens when one instance is changed (visual indicator, text, warning), or make translatibility an field setting on the instance-level (see #28, #36, #45)
  3. Removal of translatability switch batch operation. Depends on all entities being migrated to the entity field api. Not part of this issue, but do we have an issue for tracking this? I'm listing this here because it's an inconsistency that needs to be addressed eventually (see also #52)
  4. Adjustment of help text (see #70/3)
  5. Modal for operations column: the plan was (is?) to replace the form the operations column with a "configure" link and a modal. Is this postponed on another issue? I renamed the column to "Configuration" for now. (see #59, 70)
YesCT’s picture

I think we should keep the fields on the tables. It makes it transparent what "enabling translation on the entity" actually does. And gives a chance to quick unselect a few that the site builder does not want to be translatable.

The help text still needs attention to address webchick's #70 3.

and #70 @webchick

one major-ish thing... has this been tested in a screen-reader at all? :\

Is beginning to be addressed with the start of the discussion between @falcon03 in #71 and @Bojhan in #72

Just to clarify, @falcon03, you used a screenreader, right?

YesCT’s picture

Assigned: webchick » Unassigned

We are not ready for webchick yet, so unassigning this from her.

webchick’s picture

FileSize
46.6 KB

One thought that might resolve both my feedback and falcon03's is to use the UI pattern similar to the permissions page here. This way we're also not inventing anything new, in terms of UI patterns, so accessibility becomes a lot less of a concern.

Please excuse my crappy mockup:

Table with several rows in it. The table header reads 'Translation', 'Name', and 'Operations'. Left column is checkboxes, middle column is bolded in the case of a bundle name (e.g. 'Article') or un-bolded and subtly indented for field names. Operations column contains a configure link on only those bundle rows that are checked.

Bojhan's concern about scannability is sound, but I'm curious if not using a separate table column for indentation, but instead using more subtle CSS-based indenting within the same column like the permissions page does would work.

plach’s picture

Works for me, I'll implement it asap if none has objections.

Jose Reyero’s picture

Nice but I think this can get extremely long once you have a lot of content types and fields. I'm thinking we'll need -once again- some module to collapse stuff, just like admin module does for modules and permissions :-)

Have you guys ever thought about some (per node type) vertical tabs?

YesCT’s picture

    Overall, combining columns++

  1. Visually, I like combining the fields names with the .. entity type .. names in that column.
  2. We would lose some labels. But losing labels might be good. If the information is still implied.
  3. losing labels
    w-s10-losing-labels-2012-11-29_0058.png
  4. blocks

  5. blocks
    w-s04-blocks-2012-11-28_2223.png
  6. The blocks page uses the label in the header "block" on the column for the name of the block, but blocks under the same region are grouped together. The region names are not labeled differently, they are under the column for "block" (they have a class on the cell "region", but that wont help screen readers I think know they are regions and not block names. Compare this to later in this comment where I give an example of another page using label class="element-invisible"
  7. indenting

  8. The blocks are indented (and the regions not) because the blocks have cross hairs (similar but different to webchick's suggestion to use em to indent.) An em to indent would not help screen readers I think because... indenting alone doesn't have a meaning.
  9. changes on block page

  10. blocks changes
    w-s05-blocks-changes-2012-11-28_2229.png
  11. permissions combines columns

  12. the permissions page table also combines columns, the header is "permission" and most of the rows are permissions, but they are grouped under rows in the table for the ... module/permission group. They dont use an invisible label.
  13. permissions zebra

  14. the permissions page does use zebra striping on rows a little different, each row alternates. The module/permission groups are not always white. (the zebra pattern does not reset in each group.
  15. translation config does it right (imo)

  16. In the translation config page in this patch, the entity-type rows are all white and it does reset the zibra striping to begin for each group of fields under the entity types.
  17. permissions
    w-s01-permissions-2012-11-28_2127.png
  18. no label in header of checkbox column

  19. Looking at examples elsewhere in core, I think we can have NO label on the checkbox column header. (see the modules page)
  20. leave configure column. it's ok

  21. Lets leave the configure column for now,
  22. But I'm thinking maybe in the future, we take the label off that column also. and use that column for the translations settings for the entity types,
  23. and in the rows for the fields, that is where we put the magically created "this field is shared with Basic pages, Articles, Article comments, Tags"
  24. extend/modules

  25. extend
    w-s06-extend-2012-11-28_2236.png
  26. confirm to handle shared fields (follow-up)

  27. If we could.. when saving configuration, a confirmation page would ask: Are you sure you want to enable (or disable) translation on "body"? It is shared among: Basic pages, Articles, Article comments, Tags
  28. confirm
    w-s07-confirm-2012-11-28_2240.png
  29. element-invisible

  30. in #1833104-31: Add a "translatable" column to Manage Fields I note that
    @ksenzee sent me this helpful link in irc: http://zufelt.ca/blog/drupal-7-two-new-system-classes-improve-accessibility so lets try it here.

    which suggests using span class="element-invisible"

  31. In that issue:
    And in there, this is used: '1 new post in topic %title' If that is the normal way. Perhaps here we can use that like: 'Yestranslation is enabled' and 'n/atranslation cannot be enabled'
  32. With element-invisible, I *think* screen-readers would see it all the time, it would not be toggled between hidden and shown. So.. they would not get the advantage of having stuff hidden.
  33. hiding with collapsed

  34. Another way that stuff is hidden, is in collapsed field sets. That works I think because the person is clicking very close in proximity to the thing they are collapsing or expanding. It's not making something happen far from where they are clicking.
  35. extend/modules hiding

  36. modules page hides and unhides using expanded and collapsed stuff. So maybe the check boxes at the top of this page (content, comment, taxonomy, user) could cause other sections on the page to collapse or expand? (instead of toggling display none or the invisible class) .. but
  37. modules
    w-s02-modules-2012-11-28_2206.png
  38. blocks has stuff that jumps around

  39. I dont know what happens on the blocks page. But that page *changes*. When a block is moved from one region to another, the row jumps. maybe we can look there to see what that page is doing accessibility wise. *if* that page is doing it The Right Way.
  40. select things from a list and do *one* thing

  41. I could not find an example where core does *one* thing to a list of things that are selected... that would be views VBO. would we be building this page with views?!?! I dont think so, because of the hiding/showing toggling.
  42. users list

  43. users list has check boxes on the left
  44. when the users page wants to hide some users, or show some, it has a filter... which causes the whole page to reload. (not just some rows being inserted with no way for a screen reader to know they have appeared somewhere)
  45. admin/people has a list of people, and check boxes to select some, and then *a drop down* of actions, with an update button.
  46. If we did that, our drop down would only have one thing in it: enable translation. Or.. it would have enable translation and disable translation. And we would only use the check boxes to pick the rows we want to *change* the values of. And we would have to have another indication of if a row already had translation enabled.
  47. The users list page uses a bright yellow-ish color on changed rows. Only not really there either. It's only using the color to highlight all the rows with the check box checked. On that page, that is almost the same, because when you come to it, initially, every time, all the user rows are unchecked. Our situation here is different because if we come back to configure translation later, some rows will have check boxes, some will not. So we dont want to highlight all the rows that are checked, we want to highlight the changed rows. Or, we *might* want to, but if we did want to, it would just be the changed ones.
  48. users
    w-s03-users-2012-11-28_2218.png
  49. yellow to signal changed things

  50. There is some pattern of using a color to signal which rows in a table have changed, or will have changed values when a settings page is saved. The blocks admin page uses a bright yellow color on the row which has been changed (say the region changed), but is not saved yet. But I think that does not really work because if you change two rows, only the most recent row changed has that indicator. And if you change the value back (say move it from the footer, to the header, and then back to the footer) it still has the yellow changed color, even if saving that page will not really change the value.
  51. blocks changes
    w-s05-blocks-changes-2012-11-28_2229.png
  52. Oh, there is another page where you can change stuff and it indicates what has changed but that the values are not saved yet...
  53. translating the ui.
  54. translate ui
    w-s08-trans-ui-2012-11-29_0011.png
  • a. changed rows turn yellow, and stay yellow if additional rows change.
  • b. They get an astrix.
  • c. There is a warning at the *top* of the table that changes are not saved until save button is clicked
  • d. And the save button is at the *bottom* of the table.
  • html changed
    w-s09-html-changed-2012-11-29_0016.png
  • css

    • a. rows get: <tr class="even localemark-processed changed">
    • b. changed class is
      .changed {
          background: none repeat scroll 0 0 #FFFFBB;
      }
    • the value in a cell in the column with the "source string" header are labeled with invisible text <label class="element-invisible" for="edit-strings-34-original">Source string </label> Tuesday
    • The astrix gets is meaning added right there with <abbr class="warning ajax-changed" title="Changed">*</abbr>
    • The class warning on the astrix changes it's color with
        div.warning, .warning {
            color: #884400;
        }
  • OH, and.. where are we on the NOT saving thing. Like as soon as the check box is checked, the stuff is changed to be translateable, or disabled? On a really large page, that might help the "I clicked all around, but forgot to click the save button" problem.
  • Would that be a page reload every time a check box for a row in the table was clicked? Hmm. Could be bad as the page would appear sluggish. But could be good, because if you enabled a shared field, then on the reload, all the shared fields would have the same value.
  • webchick’s picture

    Wow, that's an incredibly detailed analysis! But do you have a TL;DR summary / mockup suggestion coming out of it?

    Meanwhile, I really tried to get to the help text tonight, but "unfortunately" we were only a touch above thresholds so I decided to plunge and start committing a bunch of the more minor features that have been hanging out.

    What I did do was start an Etherpad with a "before" / "after" goal, which we can collaborate on tomorrow: http://lullapad.com/et-docs

    Gábor Hojtsy’s picture

    Issue tags: -feature freeze

    *If* we *want* to go to drawing board mode instead of "let's get this in" mode, then here is some feedback I collected while doing user testing of this UI with 5 participants last weekend (don't have quotes handy from the participants, might have them next week):

    - It is fantastic! All participants were *overjoyed* to see this UI.

    - When you check a translatable checkbox, it overwrites existing settings (for language setup)

    - It defaults imagefields to be translatable which was surprising for at least one test participant

    - The language setup can be configured elsewhere even if the entity (bundle) is not configurable but not on this UI

    - In fact people were surprised there is this fantastic UI when the entity translation module is enabled but not without (for setting language configuration options for entities) - ideally this would be an existing UI even without the translation feature to have an overview of language config for entity types / bundles and then extended with the translation feature when the entity translation module is enabled

    - At least one person found it hard to see what changes do those checkboxes at the top do; if you have more of them checked, when you check another one, the table showing up outside of your viewport is not visible at all; you need to be aware that something changed outside of your viewport

    - #1853720: Hide language selection option is backwards

    - Two participants found that the content type names repeating for "Content" and "Comment" made it confusing (especially Comment showing up before Content) and they wanted to enable comment translation when they meant to enable content translation (because there are minimal differences between Content and Comment as a word, and the content types names are repeating); one participant noted the "$type comments" wording before setting it up and backed off; maybe we want to weigh the Content (node) entity type to first since that is the 80% use case?

    - (Again) It is fantastic! All participants were *overjoyed* to see this UI. :) Good job!

    (Removed feature freeze tag since several of us argued this is not bound to feature freeze at all).

    Gábor Hojtsy’s picture

    Obviously either of these can be followups if we accept the current user interface is enough of an improvement that we don't need to have 1-2 people working on it in isolation but we want to unleash it on Drupal core instead which could open the gates for more people doing smaller adaptations. I think the user testing showed this is a huge step forward in itself already and even if we fix everything we found in user testing, we'll likely found more to fix so it is just a matter of mentally accepting that things that are not absolutely perfect can land in core.

    YesCT’s picture

    Assigned: Unassigned » YesCT
    FileSize
    3.76 KB
    31 KB

    Here are the help text changes.
    Note since this configuration page is so much better than what we had before, that a bunch of help text is not even needed anymore.

    I'll take a stab at what I think is needed for the accessibility gate and then we can discuss optimizations in a follow-up.

    Status: Needs review » Needs work
    Issue tags: -JavaScript, -Usability, -Accessibility, -Needs manual testing, -translatable fields, -D8MI, -sprint, -language-content

    The last submitted patch, et-wizard-1810386-83.patch, failed testing.

    YesCT’s picture

    Status: Needs work » Needs review
    Issue tags: +JavaScript, +Usability, +Accessibility, +Needs manual testing, +translatable fields, +D8MI, +sprint, +language-content

    #83: et-wizard-1810386-83.patch queued for re-testing.

    plach’s picture

    I'd obviously prefer that other people could work on this too. However the changes proposed by Angie should not be hard to implement. If we can get a concrete set of todos that can be implemented in 2/3 hours of work I guess there's no harm in implementing them before commit.

    plach’s picture

    @Jose Reyero:

    Have you guys ever thought about some (per node type) vertical tabs?

    Sorry, I missed you post :)

    I proposed vertical tabs initially, AAMOF the D7 UI exploits precisely them (see the screenshots in #8). And I think they could somehow alleviate the problems also Gabor's testing outlined with tables popping up outside of the viewport.

    However @Bojhan is against those, altough TBH he didn't provide the underlying motivations behind his position :)

    YesCT’s picture

    This does #79 3.
    Except for handling the (green) shared fields which we have already said is going to be in a follow-up.

    et-w-s01-Content translation settings | et-wizard-83 2012-11-29 04-16-49.png
    [edit: forgot to embed img]

    Left to do is the accessibility items.

    YesCT’s picture

    @plach I could use some help figuring out where to get in the

    YesCT’s picture

    Assigned: YesCT » Unassigned
    Status: Needs review » Needs work
    FileSize
    114.82 KB

    I'm unassigning myself. Hopefully someone can jump in for this last bit.

    Well based on what I found out in #79
    I think the minimum to move forward is to
    1) get some direction on if using hide() in the javascript is ok to do
    2) add label's with the element-invisible class to show "Field" or the Bundle type name if it is, for example a content type.

    <td class="field">
    <label class="element-invisible">Field </label>
    Body
    </td>
    

    But, I could not figure out at what point that gets put in.
    I thought it would be in
    translation_entity.admin.inc
    around
    : array('data' => $bundle_label, 'class' => array('bundle')),
    and array('data' => check_plain($field['#label']), 'class' => array('field')),
    for the fields.

    Here is what I think we want the html to end up like:

    et-w-a-s01-2012-11-29_0517.png

    YesCT’s picture

    Assigned: Unassigned » YesCT
    Issue tags: -Usability, -Needs manual testing, -translatable fields, -sprint

    I think I got it. patch coming soon.

    YesCT’s picture

    adds label class="element-invisible" to the table for the column that has the name of the bundle type or the name of the field.

    YesCT’s picture

    Status: Needs work » Needs review
    Issue tags: +Usability, +Needs manual testing, +translatable fields, +sprint
    FileSize
    822 bytes
    31.08 KB

    tests passing locally now.

    adding back tags that disappeared.

    plach’s picture

    +++ b/core/modules/translation_entity/translation_entity.admin.inc
    @@ -188,6 +188,7 @@ function theme_translation_entity_admin_settings_table(array $variables) {
    +    $bundle_label=$bundle_label_text;
    

    For the next reroll: missing spaces around =

    plach’s picture

    Issue summary: View changes

    Updated issue summary.

    YesCT’s picture

    I updated the issue summary with links to some follow-up issues

    I think that means concerns that have been brought up are all addressed or have follow-ups.
    Let me know if I missed something, or feel free to make a follow-up and add it to the issue summary.

    Status: Needs review » Needs work
    Issue tags: -JavaScript, -Usability, -Accessibility, -Needs manual testing, -translatable fields, -D8MI, -sprint, -language-content

    The last submitted patch, et-wizard-1810386-93.patch, failed testing.

    plach’s picture

    Status: Needs work » Needs review

    #93: et-wizard-1810386-93.patch queued for re-testing.

    Status: Needs review » Needs work
    Issue tags: +JavaScript, +Usability, +Accessibility, +Needs manual testing, +translatable fields, +D8MI, +sprint, +language-content

    The last submitted patch, et-wizard-1810386-93.patch, failed testing.

    falcon03’s picture

    Status: Needs work » Needs review

    @YesCT:

    <td class="field"> <label class="element-invisible">Field </label> Body </td>

    what's the goal of this label? From this chunk of code I understand that it's not associated with any form field; is it?

    Btw, I've not reviewed the latest patch yet...

    Ah, and I was forgetting: you're right, I reviewed the patch with a screen reader; I cannot review anything without it! :-)

    YesCT’s picture

    I added the label in because ... because the ui translations page tabel uses one. I assumed to make it more accessible. You would know though.. so tell me. :)

    The intent was that it would help make it clear that the Body, was a field. Since the field column is being merged with the bundle/entitytype.
    The row that is a entity type for Basic Page gets a label that says Content.
    The row for a body for the Basic Page gets a label that says Content Basic Page Field.

    Visually, the Fields in the merged column are indented, but I figured an indent is not meaningful for a screenreader. So that is why I added the label.

    Like I said, this is my first time trying to make anything accessible, so I'm happy to just follow any direction you can give. :)

    bforchhammer’s picture

    +++ b/core/modules/translation_entity/translation_entity.admin.inc
    @@ -8,6 +8,219 @@
    +    array('data' => t(''), 'class' => array('translatable')),
    

    Remove empty call to t().

    +++ b/core/modules/translation_entity/translation_entity.admin.css
    @@ -0,0 +1,21 @@
    +#translation-entity-admin-settings-form table .bundle {
    

    Can we make the bundles bold (font-weight: bold;) to make them more visible in the table?

    +++ b/core/modules/translation_entity/translation_entity.admin.inc
    @@ -8,6 +8,219 @@
    +    $bundle_label=$bundle_label_text;
    

    Missing spaces.

    +++ b/core/modules/translation_entity/translation_entity.admin.inc
    @@ -8,6 +8,219 @@
    +        array('data' => '<label class="element-invisible">' . $element['#bundle_label'] . '</label>' . $bundle_label, 'class' => array('bundle')),
    ...
    +          array('data' => '<label class="element-invisible">' . $element['#bundle_label'] . ' ' . $bundle_label_text . ' ' . t('Field') . '</label>' . check_plain($field['#label']), 'class' => array('field')),
    

    AFAIK <label>s should always be tied to a specific form input field (via the "for" attribute). I think this is what @falcon03 meant in #99. A <span> tag may be more appropriate for the invisible element.

    It may still be a good idea to wrap the whole label (invisible + visible part) into a label tied to the checkbox in the left column... something like this:

    format_string('<label for="@label-target"><span class="element-invisible">@text-invisible</span> @text</label>', array(
      '@label-target' => $field_name,
      '@text-invisible' => $element['#bundle_label'] . ' ' . $bundle_label_text . ' ' . t('Field'),
      '@text' => check_plain($field['#label']),
    ))
    falcon03’s picture

    @YesCT: bforchhammer is right. The "label" element is used to provide an accessible label for a form input element. So using it that way is not so good for accessibility.

    bforchhammer's approach could be good; but, unfortunately, I cannot test the patch and I can't give detailed feedback at this time.

    If you could give us more detailed information about where you find this kind use of the "label" element in core then.... Congratulations, you have found your first accessibility bug! :-)

    YesCT’s picture

    Assigned: YesCT » Unassigned
    Status: Needs review » Needs work

    I see. I'll tackle this later unless someone else gets to it first.

    YesCT’s picture

    Status: Needs work » Needs review
    FileSize
    4 KB
    31.69 KB

    This patch addresses the remaining feedback...
    Except, getting the label for= to work. I suggest making that a follow-up.
    My reasoning is, the current patch is ok for accessibility (uses span and element-invisible). The follow-up could make it even better.
    I have uploaded my attempt (it's just the whole function) in case someone wants to see. The trouble I had was the links were being ... filtered and showing the anchor tags, etc. and getting the target text to exactly match what I think the render function is generating.

    So I think we just need to get this to pass the testbot.

    YesCT’s picture

    I forgot the code for my attempt. I'll add it on the follow-up #1855036: ARIA and accessibility improvements in entity (content) translation settings page.

    Gábor Hojtsy’s picture

    @falcon03 what do you think of the updated patch? :)

    YesCT’s picture

    updated patch has screen shot very similar to #88 I can't make a new one right now, but can later. I think the main change visually is the bolded entity/content types in the combined column.

    webchick’s picture

    Is it possible to throw this patch on a demo site so falcon03 (or anyone else from the a11y team) could take a look quickly without having to futz with patches?

    YesCT’s picture

    Assigned: Unassigned » YesCT

    I'm working on getting a test site up now. I'll post with the address when I'm done.

    Gábor Hojtsy’s picture

    FileSize
    88.61 KB

    Here is an up to date screenshot. I'm not sure I like the links, if you click a little besides the checkbox, you end up on a term list page or the content type admin page. Not sure if these belong at those critical places.

    NewWizarUI.jpg

    Xano’s picture

    If we really need those links, would it make sense to put them in an "operations" column? The links don't literally represent the config or entity displayed in the table row, but it would be somewhat more consistent with the rest of core's UI.

    YesCT’s picture

    Assigned: Unassigned » YesCT

    Here we go! (Thanks @chx)
    [edit: It's rackspace in case someone is curious.]

    http://i18n.negyesi.net/

    admin
    d8wizard

    YesCT’s picture

    Assigned: YesCT » Unassigned

    unassigning myself.
    ok. now a review should be easier.

    mgifford’s picture

    Assigned: YesCT » Unassigned
    Issue tags: +aria

    @YesCT That's a great summary in #79.

    Ultimately you're going to have to have some integration of ARIA to add the required semantics.

    The Standardista's gave a good presentation at DrupalCon London and have this intro here:
    http://www.standardista.com/standards/wai-aria-accessible-rich-internet-...

    There are lots of other references online, including:
    http://www.twylah.com/alistapart/topics/aria
    http://www-03.ibm.com/able/resources/wai_aria_intro.html

    mgifford’s picture

    Issue summary: View changes

    Updated issue summary with followup issue numbers and to reflect some accessibility feedback was encorporated

    YesCT’s picture

    I added #1855036: ARIA and accessibility improvements in entity (content) translation settings page to the summary follow-up list.

    Also, I wonder:
    can we can handle the ARIA there?
    do we need to handle it here to get this, what it is so far, committed?
    is it a separate problem and needs it's own follow up?

    did @falcon03 try out the site from #112

    falcon03’s picture

    @YesCT: sorry, but I've been out for a blind football match. I should be able to review the demo site (thank you very much for it) tomorrow.

    BTW, why cannot we handle the "label for" problem in this issue? It's not too hard to solve it, IMO.

    Gábor Hojtsy’s picture

    @falcon03: I'm sure we can solve it if we get some guidance on how best to do it :)

    YesCT’s picture

    falcon03’s picture

    Ok, this is the markup review I promised; it is later than >I expected to do it, I am sorry for that. Here we go!

    <th class="translatable"><span class="element-invisible">Translatable</span></th>

    Question from a novice just like me: can we delete the "span" wrapper, assigning the element-invisible class to the th element?

    Original code (refers to the first two columns of the table:

    <td class="translatable"><span class="element-invisible">Content typeTranslatable</span><div class="form-item form-type-checkbox form-item-settings-comment-comment-node-page-translatable">
     <input type="checkbox" id="edit-settings-comment-comment-node-page-translatable" name="settings[comment][comment_node_page][translatable]" value="1" class="form-checkbox" />
    </div></td>
    <td class="bundle"><span class="element-invisible">Content type: </span><a href="/admin/structure/types/manage/page/comment">Basic page comment</a></td>
    

    IMO the markup should look like this instead:

    <td class="translatable"><div class="form-item form-type-checkbox form-item-settings-comment-comment-node-page-translatable">
     <input type="checkbox" id="edit-settings-comment-comment-node-page-translatable" name="settings[comment][comment_node_page][translatable]" value="1" class="form-checkbox" />
    </div></td>
    <td class="bundle">
    <label for="edit-settings-comment-comment-node-page-translatable">Make Basic page comment translatable</label>
    </td>
    

    In short, my proposal consists of:

    1. Deleting the "invisible" text before the checkbox in the first column of the table;
    2. Wrapping the text in the second column of the table into a label associated to the checkbox by assigning the "id" value of the checkbox to the "for" attribute of the label;
    3. Changing a bit the label text to give blind users all the needed context (BTW, we can replace "make X translatable" with "Enable translation for X", where X is the element that we can enable translation for;
    4. Turning the element name into plain text (from the link). IMO the link can be confusing; if we need a connection to the element configuration page, we can always add it to the "configuration" column;

    However I will ask for an opinion of more experienced accessibility team members, since this pattern is not very consistent with the admin modules page one. To make it consistent, we should simply use "X" instead of "make X translatable" (where X is the element that we can enable translation for) as the checkbox' label. Maybe this approach makes more sense, since it is consistent and good for accessibility.

    Thoughts?

    Ah, and I was forgetting: maybe we should discuss some strings (for accessibility). But we can do this later (also as a follow up), so that we can get this in as soon as possible!

    Bojhan’s picture

    Can we remove the linking to the content type, I have no idea what that was added. Also the indenting is screwed up because of the inline form, I'd love if we can just move that to the Field UI formatter settings model.

    Also, the save button says "Save settings" that is not a common pattern it should be either "Save" or "Save configuration"

    YesCT’s picture

    summary of changes

    Makes the entity types plain text instead of linked.
    Addresses #119 and #120
    Removes the element-invisible, uses the label for=
    Overrides the bold of all labels to make fields not bold.

    Questions

    I dont see the problem @Bojhan saw with indent

    Also the indenting is screwed up because of the inline form

    and I dont know how to do

    I'd love if we can just move that to the Field UI formatter settings model.

    Q1: @Bojhan, can you link me to an issue or example?
    Q2: Also, can it be a follow-up?

    screenshot

    etw-s01-Content translation settings | et-wizard-121 2012-12-10 22-07-49.png

    snippet of html to see how the label for= goes with the checkbox id

    one row for entity, one row for a field

    <tr class="bundle-settings odd"><td class="translatable"><div class="form-item form-type-checkbox form-item-settings-node-page-translatable">
     <input type="checkbox" class="form-checkbox translation-entity-admin-hide-processed translation-entity-admin-bind-processed" value="1" name="settings[node][page][translatable]" id="edit-settings-node-page-translatable">
    </div>
    </td>
    <td class="bundle"><label for="edit-settings-node-page-translatable">Basic page</label></td><td class="operations"><div class="form-item form-type-item" id="edit-settings-node-page-settings" style="display: block;">[...]
    
    </div>
    </td> </tr>
     <tr class="field-settings even" style="display: table-row;"><td class="translatable"><div class="form-item form-type-checkbox form-item-settings-node-page-fields-body">
     <input type="checkbox" class="form-checkbox" value="1" name="settings[node][page][fields][body]" id="edit-settings-node-page-fields-body" checked="checked">
    </div>
    </td><td class="field"><label for="edit-settings-node-page-fields-body">Body</label></td><td class="operations"></td> </tr>
    

    Manual test

    I tested it manually, and the form still saves the settings ok.

    Next steps

    code review
    repurpose #1855036: ARIA and accessibility improvements in entity (content) translation settings page for ARIA
    (maybe) open Q2 "Field UI formatter settings model" follow-up
    identify any missing follow-ups
    put new patch on server for accessibility review
    accessibility review
    update issue summary (I'll do that right away)

    YesCT’s picture

    Issue summary: View changes

    Updated issue summary added the label for = follow up

    YesCT’s picture

    Issue summary: View changes

    updated with new next steps

    Status: Needs review » Needs work

    The last submitted patch, et-wizard-1810386-121.patch, failed testing.

    YesCT’s picture

    Status: Needs work » Needs review

    The server is updated with the patch from #121

    admin
    d8wizard

    http://i18n.negyesi.net/admin/config/regional/translation_entity

    YesCT’s picture

    oops. forgot to change the save button name in the tests.
    this passes tests locally.

    YesCT’s picture

    Issue summary: View changes

    Updated issue summary with recent screenshot

    plach’s picture

    Looking at the screenshot, I'm wondering why the first column has lost the 'Translatable' label: IMHO this way we have no clue of what checking the checkboxes actually does.

    I'll review the code here as soon as a11y concerns are resolved.

    (maybe) open Q2 "Field UI formatter settings model" follow-up

    I definitely think this should be a follow-up, since the original plan was using modal dialogs. I guess we need some discussion about this.

    identify any missing follow-ups

    http://groups.drupal.org/node/271918 has some good indications for these.

    YesCT’s picture

    The column heading was taken out to better follow a common pattern I saw ... See #79 16. 24. 32. for examples of other admin pages without a column header for the checkbox column.

    When I did that survey, I was surprised too, for example, to notice that on the modules/Extend page, the column with the checkboxes to enable moudles does not have a header label.

    What about the batch problem? Do we need to fix that to get this in? what's an estimate to how far off is getting the actual solution in?

    See #52

    Once all entities are migrated to the new Entity Field API, we are dropping the migration batch completely because then fields will always be stored under the LANGUAGE_DEFAULT key, regardless of their translatability. So trying to adapt the migration code here would just be a waste of time.

    Gábor Hojtsy’s picture

    I think the batch problem is very real, because anybody who would try out this patch would encounter the bug where ALL their field data is *simply gone* when they turn on translatability on this UI. Not if they turn it on on the other (per content type/field) UI. But if they do it here, that happens. So I think that would be so big of a WTF that it is hard to argue to get this committed without it being resolved.

    I also have a novel idea to resolve potential accessibility problems here. Let's step one back and consider the usability feedback. Bojhan said before he would want to see this best as a "Settings" tab under languages, because he considers this to belong there. Well, even until entity translation is enabled, this page would be very useful for the entity types, bundles and the content of the Configuration cell (no translation checkboxes though). That would be an overall configuration page for language setup of all content types, entities and bundles. It would not have any dynamic components to it (and no batch concern), so could fly right in core quick.

    Then we need to figure out how to integrate the translation functionality. Since the above page would have all entity types and bundles exposed right away (since all of them have base language configuration applicable regardless of whether the bundle/entity is translatable or not), it would need to be slightly adjusted and use much less dynamism in the form building, again less of an accessibility concern.

    It would overall be a more "progressively disclosed" UI and much more correct in terms of functionality. The current proposed UI does not let me set up default language and language selector visibility for content types without making them translatable first, which is contradictory to how it works in practice.

    This could have been done as followups but since the usability testing provided all this great feedback and the patch was held up by accessibility concerns, let's kill two birds with one stone as they say and solve these problems IMHO.

    plach’s picture

    So I think that would be so big of a WTF that it is hard to argue to get this committed without it being resolved.

    Then let's just postpone this on the Field API > Entity Field API conversion. There are too many things to work on to waste our time writing a multi-field conversion batch just to throw it away soon afterwards.

    I also have a novel idea to resolve potential accessibility problems here. Let's step one back and consider the usability feedback. Bojhan said before he would want to see this best as a "Settings" tab under languages, because he considers this to belong there. [...]

    I think this could fly, actually this was my initial approach but then I found some complication I can't remember exactly right now. One note about the settings tab: in this scenario it would make sense, but one of the findings of the UX test was that the Content translation settings menu item was easy to discover. If we bury this below the language item, aren't we risking to make it harder to find and, moreover, that people won't get back to it after enabling ET?

    Gábor Hojtsy’s picture

    Well, we could keep it as "content language configuration" or something we don't need to put it under a tab of languages, if we consider that would be hard to discover. Of course we can test either way once the split of the UI to base language and translations is available. The base language screen would be easily committable and does not require batches :)

    Bojhan’s picture

    Could anyone hop by IRC on this one? I am having trouble following this.

    Gábor Hojtsy’s picture

    @Bojhan: have been looking for you on IRC all day unsuccessfully.

    YesCT’s picture

    I'm confused about the vision here, and the timeline. Could we get the batch working, get this in and then get the other general entity settings improvement working? Aside from the batch, this is ready I think. I guess I dont have a reference for how much time would be wasted getting the batch to work. Maybe we could bring in someone to try their hand at that to free up plach for other stuff, if plach provided some hints to get someone started.

    Dave Reid’s picture

    Have we considered how this page is going to end up looking on a typical site with a large number of fields? Is it just going to end up being a massive page of really big tables?

    YesCT’s picture

    @Dave Reid Assuming you are talking about the screenshots posted and the current patch (and not the recent brainstorm: #127 novel idea),
    I suspect that is why the dynamic: tables do not appear until the entity types are clicked and the stuff in the tables does not expand unless looking at it. to mitigate that massive page of giant tables concern.

    falcon03’s picture

    @YesCT: thank you very much, the accessibility of this UI has been significantly improved! :D Great work!
    We have some minor A11y issues, things like finding the exact string for showing fields...

    However, as I've already said in #71, fields showing up dynamically as you enable translation for entities sounds weird to me. So I would be curious to know further details about Gábor Hojtsy's proposal to refactor the UI: how should the new UI work?

    webchick’s picture

    Well, I'd like to not deviate too much from this UI, because it has tested very well http://groups.drupal.org/node/271918 So I really would like to commit this soon! :D falcon03, do you have specific suggestions on fixing the remaining a11y issues?

    However, I also didn't quite follow what Gábor is suggesting here. I also don't quite follow why plach is resistant to fixing a data loss bug.

    Dave: Yes, just like the permissions page becomes crazy in edge case sites with tons of modules and tons of roles, this page will also grow crazy if you have an edge case of tons of entities with tons of fields marked translatable. But I think for the majority of cases it will be fine, and it's just one page in one particular module, not a new pattern we're aiming to roll out elsewhere.

    plach’s picture

    Well, I'd like to not deviate too much from this UI, because it has tested very well http://groups.drupal.org/node/271918 So I really would like to commit this soon!

    I guess we can commit this as-is and address #127 in a follow-up. It would be great if we could user test also the proposed changes and see how they perform wrt the current status.

    I also don't quite follow why plach is resistant to fixing a data loss bug.

    Because what's being proposed here is a (supposedly) expensive stop-gap fix which will be totally blown away as soon as the proper solution is in place. However if this is needed to guarantee consistent user test results before Field API is fully converted or even just to make Angie smile of joy, I will do it ;)

    @Dave:

    I'd really really wish us to explore the possibility to have a vertical tab for each table or at least I would greatly appreciate that someone explained me why this is a bad idea (you always learn something), however we have a follow-up aiming to explore possibile solutions for this: #1854030: Add hint to translation settings page when tables appear off screen.

    Gábor Hojtsy’s picture

    FileSize
    180.76 KB

    Once again I don't think we should introduce a data-loss bug even if we consider it temporary. It would be a critical bug to open right away. Not a good way to solve a major task :)

    My suggestion/problem based on the user testing results was that this UI exposes the language settings of translatable entity types / bundles *ONLY*, while the same settings are valid / applicable even for entities that will never be translatable. This UI treats those settings as secondary to translation, which is not in fact true. I can go and edit the default language and language selector exposure settings of a content type that is never going to be translatable via the content type's config UI but never on this UI. So my point was that there should be a first step UI built right into language module that does this one page configuration only for the language options (no translations checkboxes, no fields, only entity types and bundles). There is not really any way to hide any entity type or bundle there because *all of them* have these language settings. Then expand from that UI to this UI, which then even questions the checkboxes to use for the entity types and probably points to either a list of tables always shown (no top checkboxes) or vertical tabs. Then the new feature with the translation module would be the translatability checkboxes (with the language configuration NOT being conditional on those checkboxes in the interaction because it is not conditional on that in practice either).

    LanguageSettings.png

    The proposed simplified UI should not have accessibility issues I believe since it has no dynamic interactions, it is a plain and simple form. (Also it has no data loss bug because it does not involve fields at all). Then extending to the translation use case from there would result in a slightly different UI.

    plach’s picture

    @YesCT:

    The column heading was taken out to better follow a common pattern I saw ... See #79 16. 24. 32. for examples of other admin pages without a column header for the checkbox column.

    When I did that survey, I was surprised too, for example, to notice that on the modules/Extend page, the column with the checkboxes to enable moudles does not have a header label.

    It's totally not the same thing: when you select multiple nodes in the content admin page, the check meaning is implicit: you are selecting them and if you submit the form without choosing any operation nothing will happen. Here checking the checboxes has a totally different meaning and saving the form after checking them has well-defined consequences. Hence I strongly think we should restore the "Translatable" label because it's definitely not clear what checking those checkboxes will do otherwise.

    Gábor Hojtsy’s picture

    @plach: as for why it is suggested generally for usability to not use vertical tabs for whole forms (like this one I guess), Bojhan has slides relevant at http://www.slideshare.net/Bojhan/drupal-7-interface-pattern/21 (avoid using for the main interaction because it should keep being skippable "advanced" settings"), and http://p1.drupalusability.org/pattern/vertical-tabs ("Don't use it for the main interaction in the form" and "Don't use a pane that is too long. The vertical tabs are meant to be in view with the content of the page - to allow orentation" (which is relevant for having many content types and fields). That's what I found.

    Gábor Hojtsy’s picture

    BTW this video demonstrates the same checkbox / things happening down on the page problem with the filters screen and proposes vertical tabs as well (which is how I found the suggestions against that exact approach from the usability team): http://screencast.com/t/YSQ5KnFTtP (via #1834682: Consolidate filter options in the UI when configuring a format).

    Bojhan’s picture

    I really don't think its necessary here, I have commented several times that it is not the appropriate pattern both for 1) it being the main interaction 2) it creating loads of visual clutter (long tables, inside vertical tabs). Even on filter options, I'd argue its the wrong approach (but since its just forms, not really the main interaction, and we are replacing it with something more usable in the case of aloha - I can live with it). We could solve a usability issue with it, but we also create a lot of new ones with orientation - which is not as easy to test, but very important.

    I don't really get what this issue is about, we seem to be in the waters each large translation patch is in going back and forth over 100 comments over many different things.

    plach’s picture

    @Gabor, @Bojhan:

    Thanks for taking the time to explain, I've limited frontend dev experience on drupal core and I never met the arguments you are making above before.

    I'll try to post an RTBC-candidate (a11y stuff permitting) tonight, implementing at least the bulk migration for all fields. I'll see if I can split the UI so that the main part is provided by the Language module and ET just provides the translatability stuff on top of it. I'll aim to get the same UI we have now, when both Language and ET are enabled.

    Gábor Hojtsy’s picture

    Well, the problem with the current UI is you cannot get the default language / language selector visibility settings without making an entity type translatable, whereas the core logic (and the UI at other places, such as taxonomy config or content type config) perfectly allows you to set default language and language setting visibility. So making it work like this makes it impossible to use as a language configuration UI for those entity types that you do not want to make translatable, even though they have those settings, this UI just does not display them.

    plach’s picture

    The attached patch restores the translatable column label and implements the data migration for all fields whose translatability is switched. I think I found a bug in the entity query SQL implementation meanwhile. It would be good to get @chx to have look to the first hunk of the interdiff.

    I could not split the UI into content language settings and translation settings yet.

    Status: Needs review » Needs work

    The last submitted patch, et-wizard-1810386-145.patch, failed testing.

    chx’s picture

    Per http://drupal.org/node/1801726#comment-6640350 and the comment following we intentionally decided not to do $column = isset($field['columns']['value']) ? 'value' : key($field['columns']); as it is too much magic. Unless it is very much necessary for this patch I would skip it.

    plach’s picture

    Status: Needs work » Needs review
    FileSize
    36.21 KB
    3.19 KB

    Unless it is very much necessary for this patch I would skip it.

    It's not required at all but the current approach puts the burden of specifying a valid column while adding an exists condition onto the API consumer's shoulders, but only if the column is not 'value'. I'm not sure this is good DX: I didn't even realize I had to specify a column initially (see the new interdiff). Anyway, this is not the place to discuss this: rolled back that part. And since all this code will go away hopefully sooner than later, no point in refining it to death.

    Btw, the tests failures above look unrelated to me.

    Gábor Hojtsy’s picture

    Related: #1314250: Allow filtering/configuration of which languages apply to what (UI, nodes, files, etc) which also proposed a per-entity type / bundle settings screen much like the one introduced here to configure the set of languages as they apply to each entity type / bundle. I don't think there are any action items for this issue related to that though, just cross-linking.

    plach’s picture

    FileSize
    41.88 KB

    Ok, here is a new one splitting the settings between the Language module and ET. Too late for screenshots. Hopefully we should be ok now.

    Not including an interdiff since the patch has been almost totally rewritten.

    Status: Needs review » Needs work

    The last submitted patch, et-wizard-1810386-150.patch, failed testing.

    YesCT’s picture

    wow! I'll try and test this tonight.

    tim.plunkett’s picture

    +++ b/core/modules/translation_entity/translation_entity.admin.incundefined
    @@ -8,6 +8,308 @@
    +function theme_translation_entity_admin_settings_table(array $variables) {
    ...
    +  $output = theme('table', array('header' => $header, 'rows' => $rows));
    +  $output .= drupal_render_children($element);
    +  return $output;
    

    It seems to me that a large portion of this could be moved to a template_preprocess_translation_entity_admin_settings_table(), in order to keep it alterable and clean.

    Also, I don't think we type hint $variables in theme functions

    +++ b/core/modules/translation_entity/translation_entity.admin.incundefined
    @@ -8,6 +8,308 @@
    +      'class' => array('translatable')
    ...
    +      'class' => array('bundle')
    ...
    +      'class' => array('operations')
    

    Missing trailing commas

    +++ b/core/modules/translation_entity/translation_entity.admin.incundefined
    @@ -8,6 +8,308 @@
    +          'data' => drupal_render($element[$bundle]['translatable']),
    ...
    +          'data' => drupal_render($element[$bundle]['settings']),
    ...
    +            'data' => drupal_render($field),
    

    AFAIK, you shouldn't have to be calling drupal_render here, it will handle render arrays.

    +++ b/core/modules/translation_entity/translation_entity.admin.jsundefined
    @@ -0,0 +1,35 @@
    +    $('table .bundle-settings .translatable :input:not(:checked)').once('translation-entity-admin-hide', function() {
    ...
    +    $('table .bundle-settings .translatable :input').once('translation-entity-admin-bind', function() {
    

    This would likely be improved by doing
    var $bundles = $(context).find('table .bundle-settings .translatable :input');

    And then doing $bundles.find(':not(:checked)') later.

    +++ b/core/modules/translation_entity/translation_entity.moduleundefined
    @@ -343,6 +354,26 @@ function translation_entity_enabled($entity_type, $bundle = NULL, $skip_handler
    +    if (!empty($info['fieldable']) && !$entity_class->implementsInterface('Drupal\Core\Config\Entity\ConfigEntityInterface')) {
    

    Not sure how temporary this is supposed to be, but ConfigEntities aren't fieldable, so if that's all that matters, this could be simplified.

    Also, it'd be nice to stop using !empty() where it's not needed. A simple if ($info['fieldable']) { is sufficient, since it will always be a Boolean.

    YesCT’s picture

    Status: Needs review » Needs work
    FileSize
    1.74 KB
    41.86 KB

    I think Tim was reviewing #148
    Most of it still applies.
    I did some simple things.

    I did not do:
    taking out the drupal_render. It made some strange things appear. I left it in. [edit: added punctuation]

    I did not do and maybe should still be done:
    moving to a template_preprocess_translation_entity_admin_settings_table()
    the .js suggestion

    I only took out the !empty() in the line that was altered by this patch. Maybe doing the rest should be a follow-up.

    YesCT’s picture

    Status: Needs work » Needs review

    having the bot check to see if this has the goto failure too.

    YesCT’s picture

    Testing:
    enabling does fire off the batch. ok.

    Hiding the language selector is checked by default, and when translation is enabled, it is unchecked automatically.
    Fields are automatically marked translatable when the entity is checked to be translatable.

    When there are no fields to enable, enabling translation of the entity is not allowed.

    The label=for is still there ok.

    Enabling translation and setting the default language and hiding/unhiding the language selector work.

    Screenshot:
    Content language settings | wizard-154 2012-12-19 01-22-32.png

    Leaving it as needs review since the change in #150 was a big one.

    plach’s picture

    Status: Needs work » Needs review

    I think we are missing a screenshot with ET disabled an Language enabled.

    Will look into @tim's review ASAP.

    tim.plunkett’s picture

    I attempted to review 150, not 148

    plach’s picture

    Here is a new one addressing #153. The attached screenshot shows how the page appears when ET is disabled.

    Not sure how temporary this is supposed to be, but ConfigEntities aren't fieldable, so if that's all that matters, this could be simplified.

    Until it's more clear what we are actually able to support we are skipping both non-fieldable entities and config ones.

    The tables have onlt the language settings but no checkboxes to switch transltability for bundles and fields

    Status: Needs review » Needs work
    Issue tags: -JavaScript, -Usability, -Accessibility, -Needs manual testing, -aria, -translatable fields, -D8MI, -sprint, -language-content

    The last submitted patch, et-wizard-1810386-159.patch, failed testing.

    YesCT’s picture

    Status: Needs work » Needs review
    Issue tags: +JavaScript, +Usability, +Accessibility, +Needs manual testing, +aria, +translatable fields, +D8MI, +sprint, +language-content

    #159: et-wizard-1810386-159.patch queued for re-testing.

    YesCT’s picture

    sorry @tim.plunkett. no worries though. It's really nice to have some more eyes on this. (I thought that because in 150, !empty($info['fieldable'] is in language.module not translation_entity.module. maybe this is a dreditor weirdness)

    @plach Oh! I totally missed that main point, that this worked without et.
    Are there any other followups you think need created?

    I'll update the issue summary.

    Next steps: a (final?) code review.

    YesCT’s picture

    Issue summary: View changes

    Updated issue summary to point out that it works with language alone, update the resent screenshots, update remaining tasks.

    YesCT’s picture

    Issue summary: View changes

    Updated issue summary fixing img src

    Gábor Hojtsy’s picture

    Fantastic improvement with the language module only screen :) I was a little puzzled with the "Language support" screenshots, because from a **technical** perspective, everything has language support, they just default to not show the language selector and to assign the site default language :) We can define that scenario as "no language support" in a sense, if language support is defined by "some visual UI for language" or at least "different to default setup". Maybe I'd document that those things that do not have language support are saved with site default language and have no language selector or somesuch. So it is clear this screen is to configure deviations from the default assumptions :)

    Checked to see the logic in the code:

    +++ b/core/modules/language/language.admin.incundefined
    @@ -1027,3 +1027,168 @@ function language_negotiation_configure_browser_delete_form_submit($form, &$form
    +    // If any language configuration setting has changed for any bundle, we
    +    // assume language support has been enabled for this entity type.
    +    foreach (entity_get_bundles($entity_type) as $bundle) {
    +      $conf = language_get_default_configuration($entity_type, $bundle);
    +      if (empty($conf['language_hidden']) || $conf['langcode'] != 'site_default') {
    

    Logic looks good :)

    Also in the meantime, vocabularies were converted to config entities. Not sure if that affects this patch in any way, because we are configuring terms, not vocabularies per say, however it is worth double checking. I have seen the tests cover comments but not terms.

    YesCT’s picture

    A few docs fixes and fix for the new url of the settings page.

    I used "By default, language settings hide the language selector and the language is the site's default language."
    but I *wanted* to use "By default, language settings hide the language selector and _things_ are saved in the site's default language."
    I tried elements or content instead of _things_ but I could not find the right word.

    +++ b/core/modules/language/language.admin.incundefined
    @@ -1027,3 +1027,168 @@ function language_negotiation_configure_browser_delete_form_submit($form, &$form
    + * Adds a render element representing the bundle language settings table.
    

    http://drupal.org/node/1354#hookimpl

    +++ b/core/modules/language/language.admin.incundefined
    @@ -1027,3 +1027,168 @@ function language_negotiation_configure_browser_delete_form_submit($form, &$form
    + * Theming function for an administration settings table.
    

    http://drupal.org/node/1354#themeable

    wiz-s01-custom_language_settings-2012-12-20_0539.png

    klonos’s picture

    Yeah, "Language support" always stroke kinda strange to me too. I think that "Multilingual support" or "Multiple language support" are better alternatives. I realize that the later is a bit "wordy", but the term multilanguage does not exist in any dictionary I looked up (it redirects to multilingualism/multilingual in The Free Dictionary).

    Also (because of #1869292: Remove confusing "multiple" language from core and in light of #1869328: Restore simplicity of language list), I don't get why we have the language-content-settings-form shown at all when there's only one (non-system) language enabled on the site. Please see attached screenshot:

    remove the language-content-settings-form when only one language

    Gábor Hojtsy’s picture

    Yes, not specified and not applicable might have significance on a single language site. Eg. if you are aggregating a feed of cat photos into entities, then technically correct would be to set the entities to "Not applicable", because those are just cat photos, no text. If you are aggregating the twitter feed of a multilingual person for example, such as myself (I tweet sometimes in English, less often in Hungarian), then the entities where these are saved would technically be "Not specified", since you don't really know if the aggregated content was in which language. These are technical things, and that is why we default to the site default language as is appropriate for the 80% use case. In short, even on single language websites, content that has no language or is of unknown language might easily be present.

    Also that is why "multilingual" might not be a good word for this, because this setting applies on single language websites.

    plach’s picture

    Ok, are we missing anything to get back to RTBC? Tonight I'll have a final pass to resolve any a11n concern raised by the WAVE toolbar and then at least basic requirementes should be met. I don't think it's worth to hold this up any longer, since we are getting no feedback on that side.

    Gábor Hojtsy’s picture

    @plach: I don't see any other problems that would hold this up.

    plach’s picture

    I just run an analysis through the WAVE toolbar and in the relevant page area it found:

    • a number of errors due to labels incorrectly associated to non-existing input elements (the table titles); this is an issue introduced by the 'item' form element, not something related to this patch;
    • a number of warnings due to elements hidden through display: none rules, which I think are fine because those elements are meant to be hidden until revealed also to screen-readers.

    Hence I think we should be ready to go.

    plach’s picture

    Latest patch review + fixes.

    +++ b/core/modules/language/language.admin.inc
    @@ -1027,3 +1027,171 @@ function language_negotiation_configure_browser_delete_form_submit($form, &$form
    +    // If any language configuration setting has changed for any bundle, we
    +    // assume language support has been enabled for this entity type.
    

    This comment needs to be updated.

    +++ b/core/modules/translation_entity/translation_entity.admin.inc
    @@ -8,6 +8,176 @@
    +      // Here we do not want the widget to be altered and hold also the
    +      // "Enable translation" checkbox, which would be redundant. Hence we
    +      // add this key to be able to skip alterations.
    

    Comment not wrapping correctly.

    +++ b/core/modules/translation_entity/translation_entity.module
    @@ -158,12 +160,12 @@ function translation_entity_menu() {
    -  $items['admin/config/regional/translation_entity/translatable/%'] = array(
    +  $items['admin/config/regional/content-language/translatable/%'] = array(
    
    @@ -581,7 +583,7 @@ function translation_entity_form_field_ui_field_settings_form_alter(array &$form
    -    $path = "admin/config/regional/translation_entity/translatable/$field_name";
    +    $path = "admin/config/regional/content-language/translatable/$field_name";
    

    Wrong search/replace.

    +++ b/core/modules/translation_entity/translation_entity.module
    @@ -395,9 +397,9 @@ function translation_entity_permission() {
    +    'administer entity translation' => array(
    +      'title' => t('Administer entity translation'),
    +      'description' => t('Configure translatability of entities and fields.'),
         ),
    

    Actually this permission should be used to limit access to the translation settings.

    Status: Needs review » Needs work
    Issue tags: -JavaScript, -Usability, -Accessibility, -Needs manual testing, -aria, -translatable fields, -D8MI, -sprint, -language-content

    The last submitted patch, et-wizard-1810386-170.patch, failed testing.

    plach’s picture

    Status: Needs work » Needs review
    Issue tags: +JavaScript, +Usability, +Accessibility, +Needs manual testing, +aria, +translatable fields, +D8MI, +sprint, +language-content

    #170: et-wizard-1810386-170.patch queued for re-testing.

    Gábor Hojtsy’s picture

    Status: Needs review » Reviewed & tested by the community

    Looks good to me with these updates, the data loss bug has been fixed and accessibility concerns have been resolved as far as I see.

    YesCT’s picture

    #170: et-wizard-1810386-170.patch queued for re-testing.

    falcon03’s picture

    Status: Reviewed & tested by the community » Needs review

    Sorry, I haven't had time to comment on this issue. Tomorrow I'll post an A11Y review of the latest patch...

    I've set this issue status to "needs review" for this reason, feel free to set it back to RTBC if I did a mistake...

    YesCT’s picture

    The test server does not have the most recent #170 patch. @falcon03 should we get it live?

    falcon03’s picture

    @YesCT: I noticed that the live site wasn't working, so I tried applying the patch locally. But unfortunately I can't see the first column of the table (the column containing the checkboxes) anylonger. Am I doing something wrong?

    I've added "italian" (my spoken language :D) to the languages used in the Drupal installation and I've enabled language and interface translation modules, but nothing changed. I get this problem with all the entities (comment, content, user, etc).

    If you can help me solving this issue (if this is not a bug introduced by the patch) I'll solve it locally. Otherwise, if we need to get it in sooner, it would be great to get the demo site working again. Anyway I'll post the final review tomorrow (sorry, today I've been busy working on the link title issue).

    plach’s picture

    @falcon03:

    I think you just need to enable the Content translation module, which will enhance the Content language settings page with the content translation options.

    falcon03’s picture

    @plach#178: thank you, enabling the content translation module solved the problem.
    However, I would have never expected this behavior: can we reconsider it? It's weird to see the interface if I cannot do anything!

    From an accessibility point of view, we have two important issue to handle in this patch:

    1. Before each table, there is a label that describes the content of the table (e.g. "content" before the content settings table). It is created with a form label, but it's not associated to any form component: it shouldn't be a label! :-) Can we make it an heading (as we've already done in the new Views UI for enabled/disabled views)?
    2. Can we add the entity bundle that a field belongs to before each field label?

    Another thing that sounds weird to me is that, after enabling translation for "article comments" I can see another "comment" checkbox related to the article content type...

    And finally (last but not least :-)) can we add an ARIA live region to alert screen reader users of field showing/hiding in the table? We can solve this last point in a follow up.

    YesCT’s picture

    @falcon03

    I would have never expected this behavior: can we reconsider it? It's weird to see the interface if I cannot do anything!

    Yeah, I missed this kind of also. Now this page can change the language settings and it can do it separate from having translation enabled.

    So, you can "do something" even without content/entity translation enabled. What you can do is to change the default language content is created in, and you can change the hide/show setting to show a language selector.

    For example, without enabling entity translation, you can use this page to have the basic page content type default to spanish and to not hide the language select.

    I think similar confusion regarding enabling translation (entity/content translation) when language/locale module is enabled is mentioned in a recent post about user study... http://groups.drupal.org/node/274458

    Addressing that confusion will be a follow-up of the user testing most likely.

    YesCT’s picture

    2. Can we add the entity bundle that a field belongs to before each field label?

    So something like... <span class="element-invisible">Article </span>body on the row for the body under article and <span class="element-invisible">Basic page </span>body for the row for the body under Basic page.

    YesCT’s picture

    Status: Needs review » Needs work
    Issue tags: -Needs manual testing

    setting to needs work to address 1. (use heading instead of label) and 2. (add words to identify which bundle a field is in) from #179

    After that, I think this is ready.

    The rest will be follow-ups.

    plach’s picture

    Status: Needs work » Needs review
    FileSize
    42.1 KB
    2.35 KB

    Here it is! Hopefully we should be done :)

    0ba80c6 Issue #1810386: Converted table labels into headings.
    2a812e3 Issue #1810386: Added a hidden bundle lable prefix to field names.
    
    YesCT’s picture

    The code looks clean, and it does the fix for 1. and 2. :)

    here are some updated screen shots.

    wiz-s01-Content language settings | 8.x-wizard 2012-12-28 20-51-58.png

    wis-s02-type-2012-12-28_2054.png

    With no entity translation enabled:

    wiz-s03-notrans-Content language settings | 8.x-wizard 2012-12-28 21-02-46.png

    YesCT’s picture

    Issue summary: View changes

    Updated issue summary still fixing img link

    YesCT’s picture

    Issue summary: View changes

    Updated issue summary with recent screenshots and updated remaining tasks!

    YesCT’s picture

    Issue summary: View changes

    corrected comment that screenshots are for.

    webchick’s picture

    Status: Reviewed & tested by the community » Fixed

    YEAH!!!! Awesome work, everyone! :D

    Committed and pushed to 8.x. YEEHAW!

    Gábor Hojtsy’s picture

    Woot, woot! Thanks everyone, fabulous work!

    nod_’s picture

    Totally my fault should have rerolled the patch earlier. Created a follow-up for the JS part of it #1877048: Make Translation entity module JS follow newer core rules.

    Gábor Hojtsy’s picture

    Issue tags: -sprint

    Removing sprint tag.

    Gábor Hojtsy’s picture

    YesCT’s picture

    Status: Fixed » Closed (fixed)

    Automatically closed -- issue fixed for 2 weeks with no activity.

    YesCT’s picture

    YesCT’s picture

    Issue summary: View changes

    nit: updating comment number for screenshot

    Gábor Hojtsy’s picture

    @alexpott asked for a change notice on a followup, so added one at http://drupal.org/node/1987978 and included this issue on it.

    YesCT’s picture

    Issue summary: View changes

    added new follow up for deciding how this page is build

    YesCT’s picture

    Issue summary: View changes

    adding new issue that relates to this. might be a follow-up. at least it's linked now.

    Gábor Hojtsy’s picture

    Now that we have this page, it is quite problematic that the "no fields configured translatable" button on the initial experience of the translation summary table does not point here but to the field listing. See #2154709: "No translatable fields" link points to misleading place.. Need reviews and get to RTBC quick :D

    plach’s picture

    Gábor Hojtsy’s picture

    In #2355909: language.settings config is not scalable, @chx argues this screen will not work in real life.

    mgifford’s picture

    Issue tags: +aria-live