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
and
without entity translation enabled
Original screenshots in #8.
Remaining tasks
Provide a set of mockups describing how the new UI should workImplement themProvide test coverageFollow-ups openeduse label for= for accesssibilitycode reviewrepurpose #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
- #1854030: Add hint to translation settings page when tables appear off screen
- #1854046: Add "changed" hints to translation settings page
- #1854056: Handle shared fields in translation settings
- #1855036: ARIA and accessibility improvements in entity (content) translation settings page
- #1977784: Content language settings configuration page needs to determine what entities and bundles to include
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.
Comments
Comment #1
YesCT CreditAttribution: YesCT commentedSince #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.
Comment #2
plachComment #3
plachComment #4
Gábor HojtsyMarking it for the sprint.
Comment #5
plachComment #6
ytsurki would love to see a bulk update functionality, f.e. for the Users may translate this field bool for the fields on a contenttype.
Comment #7
plachComment #7.0
plachCreated issue summary
Comment #8
plachscreenshots
Comment #9
Bojhan CreditAttribution: Bojhan commentedI will work on this.
Comment #9.0
Bojhan CreditAttribution: Bojhan commentedUpdated issue summary.
Comment #9.1
plachUpdated issue summary.
Comment #10
plachComment #11
Bojhan CreditAttribution: Bojhan commentedNoting 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.
Comment #11.0
Bojhan CreditAttribution: Bojhan commentedUpdated issue summary.
Comment #12
YesCT CreditAttribution: YesCT commentedComment #13
plachWe agreed with Bojhan this won't be a wizard :)
Comment #14
YesCT CreditAttribution: YesCT commentedThat's cool. :)
Comment #15
Bojhan CreditAttribution: Bojhan commentedAlright, 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.
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.
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.
Comment #16
Bojhan CreditAttribution: Bojhan commentedComment #16.0
Bojhan CreditAttribution: Bojhan commentedUpdated issue summary added background so not to lose information from the et-ui follow-up process.
Comment #17
Gábor HojtsyReviewing #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
Comment #18
Gábor HojtsyAlso, 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.
I think this "wizard" screen should only include settings available elsewhere.
Comment #19
Bojhan CreditAttribution: Bojhan commentedYes, I considered this but given that its a "whole setup" form, it should be likely they scroll down to see everything.
You are right, I introduced checkboxes. These should have some *magic* to allow you to disable children.
Is this similar to checkboxes or should I introduce something more?
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.
Changed it to vocabulary.
Added examples.
I would love to incorporate whatever settings there need to be here, I did indeed just look at D7.
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.
Let me know if this is a good direction, and enough to go from.
Comment #20
Gábor HojtsyYes, 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 :)
Comment #21
plachI 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.
Comment #22
plachAlso, 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?
Comment #23
Bojhan CreditAttribution: Bojhan commented@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.
Comment #24
plachBut 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.
Comment #25
Gábor Hojtsy@plach: so ignore those configure links :)
Comment #26
Bojhan CreditAttribution: Bojhan commentedYes, totally ignore them then :D
Comment #27
plachWorking on this.
Comment #28
YesCT CreditAttribution: YesCT commentedOne 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"?
Comment #29
plachA 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.
Comment #30
plachAnd 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.
Comment #31
plachForgot to complete a comment.
Comment #32
plachtagging
Comment #33
Gábor HojtsyNow would be the time to add this as a config link in the .info file? :)
Comment #34
Gábor HojtsyScreenshots!
Default state of the screen, nothing is translatable:
After I picked nodes and taxonomy terms:
After I picked the article content type to make translatable (it defaulted to make all checkboxes checked too):
(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.
Comment #35
Gábor HojtsyDiscussed 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"?
Let's call it Content type in both cases. That is the human facing name. :)
Comment #36
plachIt's already there :)
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.
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.
Comment #37
Gábor HojtsyWell, 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?
Comment #38
plachFrom 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")
Comment #39
Gábor HojtsyYeah, 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?
Comment #40
plachI 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
Comment #41
andypost#34 is great, I think for field settings better to follow the same UI-patter as used for text-formats VT
Comment #42
Gábor Hojtsy@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?
Comment #42.0
Gábor HojtsyUpdated issue summary.
Comment #42.1
plachUpdated issue summary.
Comment #42.2
plachUpdated issue summary.
Comment #43
YesCT CreditAttribution: YesCT commentedI 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.
Comment #44
Gábor Hojtsy@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.
Comment #45
YesCT CreditAttribution: YesCT commentedin #36 @plach
I admit I dont know exactly what "instance setting" is.
Would making it per instance move it out of the global settings?
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"]
Comment #46
plachHere is the test coverage + minor improvements to the JS code and UX: now the language selector is automatically shown when making a bundle translatable.
Comment #47
plachSince this is bound to the feature freeze I'd say this is a feature :)
Comment #48
Bojhan CreditAttribution: Bojhan commentedNope, its not. This is a significant usability problem that arguably almost renders it unusable to people starting out, not a "nice to have"
Comment #49
Gábor HojtsyAlso, 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!
Comment #50
plachI guess @webchick will want to have a look to this one.
Comment #50.0
plachUpdated issue summary.
Comment #51
bforchhammer CreditAttribution: bforchhammer commented1. 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).
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
).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)?4. (minor) "non translatable" -> "not translatable"? Also, missing comma after "Similarly".
5. The "Operations" column currently does not show any operations but the settings form directly... rename respectively?
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.
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).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).
Comment #52
plachThanks :)
I'll fix the rest later, but let me answer to the last points:
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.This is unrelated, it's just hard to reproduce: #1833108: disabling translation of fields with data stalls and does not work.
Comment #53
YesCT CreditAttribution: YesCT commentedThis is so sweet!
Steps to test drive
Check it:
There are some cool follow-ups we can make after this is in.
Comment #54
YesCT CreditAttribution: YesCT commentedoops. 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...
Comment #55
sunI 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?
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...
Missing semicolons.
Can we add some comments here and elsewhere to clarify what kind of effect this property has?
Heh? ;)
You should be able to return this:
$output = theme('table', ...');
$output .= drupal_render_children($element);
return $output;
The description is more precise than the label, so we can drop the description and use it as label. :)
We can remove the extra arguments here.
Comment #56
plachWorking on this.
Comment #57
YesCT CreditAttribution: YesCT commentedpotential 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.
Comment #58
YesCT CreditAttribution: YesCT commentedHere is the issue for the message flashing by: #1848210: [Tests] Submitting a form in Overlay shows content + dsm() for a split second before redirecting to front-end theme
Comment #59
plachThe attached patch should address the reviews above:
A couple of answers.
@bforchhammer:
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 :)
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?
My bad, I initially tried something like what you suggested but it didn't work...
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...
Comment #60
YesCT CreditAttribution: YesCT commentedpatch in #59 passes the test bot (the test bot is just having trouble reporting back)
http://qa.drupal.org/pifr/test/390833
Comment #61
YesCT CreditAttribution: YesCT commentedThis should be "Comment", not "Content type".. or just left using label default.
Probably a cut and paste thing. :)
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
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
1b. also a config link on the modules page, where the help and permissions link is
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.)
2. initial configure page
2b. pick node
2c. pick taxonomy
3a. pick article
3b. pick more, change some of the defaults
4a. verify it worked article
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
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.
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.
6c. also tried disabling whole entity types
That worked too.
Comment #62
YesCT CreditAttribution: YesCT commentedI 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"
@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.
Comment #63
YesCT CreditAttribution: YesCT commentedI understand the bundle_label better now. :)
Comment #64
plach#63 looks good to me, thanks.
Comment #65
plachI 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.
Comment #66
plach(bot green)
Comment #67
Gábor HojtsyChanges look good. AFAIS all review feedback was addressed.
Comment #68
plachBack to @webchick's queue.
Comment #69
xjmThe 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.)
"Contains" now rather than "Definition of".
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
"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. :)
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."
This needs parameter documetation.
Should be "Page callback: Returns the translation administration settings form." Reference: http://drupal.org/node/1354#menu-callback
This should have an
@see
reference to its validation and submission handlers. Also, I guess the standard form is "Form constructor" (not form builder).These should have
@see
references to each other. Reference: http://drupal.org/node/1354#formsReally trivial: "non-translatable" should be hyphenated.
I think this needs a bit more documentation, see: http://drupal.org/node/1354#themeable
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."
Comment #70
webchickHOLY 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.
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"?
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.
Comment #70.0
webchickUpdated issue summary.
Comment #71
falcon03 CreditAttribution: falcon03 commented@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!
Comment #72
Bojhan CreditAttribution: Bojhan commented1. 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.
Comment #73
bforchhammer CreditAttribution: bforchhammer commentedRerolled 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):
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) fromelement_children()
. (see #69/10)Comment #74
YesCT CreditAttribution: YesCT commentedI 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
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?
Comment #75
YesCT CreditAttribution: YesCT commentedWe are not ready for webchick yet, so unassigning this from her.
Comment #76
webchickOne 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:
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.
Comment #77
plachWorks for me, I'll implement it asap if none has objections.
Comment #78
Jose Reyero CreditAttribution: Jose Reyero commentedNice 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?
Comment #79
YesCT CreditAttribution: YesCT commentedOverall, combining columns++
blocks
label class="element-invisible"
indenting
changes on block page
permissions combines columns
permissions zebra
translation config does it right (imo)
no label in header of checkbox column
leave configure column. it's ok
extend/modules
confirm to handle shared fields (follow-up)
element-invisible
which suggests using
span class="element-invisible"
hiding with collapsed
extend/modules hiding
blocks has stuff that jumps around
select things from a list and do *one* thing
users list
yellow to signal changed things
css
<tr class="even localemark-processed changed">
<label class="element-invisible" for="edit-strings-34-original">Source string </label> Tuesday
<abbr class="warning ajax-changed" title="Changed">*</abbr>
Comment #80
webchickWow, 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
Comment #81
Gábor Hojtsy*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).
Comment #82
Gábor HojtsyObviously 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.
Comment #83
YesCT CreditAttribution: YesCT commentedHere 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.
Comment #85
YesCT CreditAttribution: YesCT commented#83: et-wizard-1810386-83.patch queued for re-testing.
Comment #86
plachI'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.
Comment #87
plach@Jose Reyero:
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 :)
Comment #88
YesCT CreditAttribution: YesCT commentedThis does #79 3.
Except for handling the (green) shared fields which we have already said is going to be in a follow-up.
[edit: forgot to embed img]
Left to do is the accessibility items.
Comment #89
YesCT CreditAttribution: YesCT commented@plach I could use some help figuring out where to get in the
Comment #90
YesCT CreditAttribution: YesCT commentedI'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.
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:
Comment #91
YesCT CreditAttribution: YesCT commentedI think I got it. patch coming soon.
Comment #92
YesCT CreditAttribution: YesCT commentedadds label class="element-invisible" to the table for the column that has the name of the bundle type or the name of the field.
Comment #93
YesCT CreditAttribution: YesCT commentedtests passing locally now.
adding back tags that disappeared.
Comment #94
plachFor the next reroll: missing spaces around =
Comment #94.0
plachUpdated issue summary.
Comment #95
YesCT CreditAttribution: YesCT commentedI 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.
Comment #97
plach#93: et-wizard-1810386-93.patch queued for re-testing.
Comment #99
falcon03 CreditAttribution: falcon03 commented@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! :-)
Comment #100
YesCT CreditAttribution: YesCT commentedI 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. :)
Comment #101
bforchhammer CreditAttribution: bforchhammer commentedRemove empty call to
t()
.Can we make the bundles bold (font-weight: bold;) to make them more visible in the table?
Missing spaces.
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:
Comment #102
falcon03 CreditAttribution: falcon03 commented@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! :-)
Comment #103
YesCT CreditAttribution: YesCT commentedI see. I'll tackle this later unless someone else gets to it first.
Comment #104
YesCT CreditAttribution: YesCT commentedThis 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.
Comment #105
YesCT CreditAttribution: YesCT commentedI 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.
Comment #106
Gábor Hojtsy@falcon03 what do you think of the updated patch? :)
Comment #107
YesCT CreditAttribution: YesCT commentedupdated 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.
Comment #108
webchickIs 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?
Comment #109
YesCT CreditAttribution: YesCT commentedI'm working on getting a test site up now. I'll post with the address when I'm done.
Comment #110
Gábor HojtsyHere 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.
Comment #111
XanoIf 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.
Comment #112
YesCT CreditAttribution: YesCT commentedHere we go! (Thanks @chx)
[edit: It's rackspace in case someone is curious.]
http://i18n.negyesi.net/
admin
d8wizard
Comment #113
YesCT CreditAttribution: YesCT commentedunassigning myself.
ok. now a review should be easier.
Comment #114
mgifford@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
Comment #114.0
mgiffordUpdated issue summary with followup issue numbers and to reflect some accessibility feedback was encorporated
Comment #115
YesCT CreditAttribution: YesCT commentedI 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
Comment #116
falcon03 CreditAttribution: falcon03 commented@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.
Comment #117
Gábor Hojtsy@falcon03: I'm sure we can solve it if we get some guidance on how best to do it :)
Comment #118
YesCT CreditAttribution: YesCT commented#1833020: Polish help page for Entity translation UI postponed on this issue.
Comment #119
falcon03 CreditAttribution: falcon03 commentedOk, 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:
IMO the markup should look like this instead:
In short, my proposal consists of:
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!
Comment #120
Bojhan CreditAttribution: Bojhan commentedCan 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"
Comment #121
YesCT CreditAttribution: YesCT commentedsummary 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
and I dont know how to do
Q1: @Bojhan, can you link me to an issue or example?
Q2: Also, can it be a follow-up?
screenshot
snippet of html to see how the label for= goes with the checkbox id
one row for entity, one row for a field
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)
Comment #121.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary added the label for = follow up
Comment #121.1
YesCT CreditAttribution: YesCT commentedupdated with new next steps
Comment #123
YesCT CreditAttribution: YesCT commentedThe server is updated with the patch from #121
admin
d8wizard
http://i18n.negyesi.net/admin/config/regional/translation_entity
Comment #124
YesCT CreditAttribution: YesCT commentedoops. forgot to change the save button name in the tests.
this passes tests locally.
Comment #124.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary with recent screenshot
Comment #125
plachLooking 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.
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.
http://groups.drupal.org/node/271918 has some good indications for these.
Comment #126
YesCT CreditAttribution: YesCT commentedThe 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
Comment #127
Gábor HojtsyI 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.
Comment #128
plachThen 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 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?
Comment #129
Gábor HojtsyWell, 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 :)
Comment #130
Bojhan CreditAttribution: Bojhan commentedCould anyone hop by IRC on this one? I am having trouble following this.
Comment #131
Gábor Hojtsy@Bojhan: have been looking for you on IRC all day unsuccessfully.
Comment #132
YesCT CreditAttribution: YesCT commentedI'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.
Comment #133
Dave ReidHave 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?
Comment #134
YesCT CreditAttribution: YesCT commented@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.
Comment #135
falcon03 CreditAttribution: falcon03 commented@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?
Comment #136
webchickWell, 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.
Comment #137
plachI 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.
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.
Comment #138
Gábor HojtsyOnce 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).
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.
Comment #139
plach@YesCT:
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.
Comment #140
Gábor Hojtsy@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.
Comment #141
Gábor HojtsyBTW 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).
Comment #142
Bojhan CreditAttribution: Bojhan commentedI 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.
Comment #143
plach@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.
Comment #144
Gábor HojtsyWell, 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.
Comment #145
plachThe 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.
Comment #147
chx CreditAttribution: chx commentedPer 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.Comment #148
plachIt'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.
Comment #149
Gábor HojtsyRelated: #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.
Comment #150
plachOk, 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.
Comment #152
YesCT CreditAttribution: YesCT commentedwow! I'll try and test this tonight.
Comment #153
tim.plunkettIt 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
Missing trailing commas
AFAIK, you shouldn't have to be calling drupal_render here, it will handle render arrays.
This would likely be improved by doing
var $bundles = $(context).find('table .bundle-settings .translatable :input');
And then doing
$bundles.find(':not(:checked)')
later.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.Comment #154
YesCT CreditAttribution: YesCT commentedI 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.
Comment #155
YesCT CreditAttribution: YesCT commentedhaving the bot check to see if this has the goto failure too.
Comment #156
YesCT CreditAttribution: YesCT commentedTesting:
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:
Leaving it as needs review since the change in #150 was a big one.
Comment #157
plachI think we are missing a screenshot with ET disabled an Language enabled.
Will look into @tim's review ASAP.
Comment #158
tim.plunkettI attempted to review 150, not 148
Comment #159
plachHere is a new one addressing #153. The attached screenshot shows how the page appears when ET is disabled.
Until it's more clear what we are actually able to support we are skipping both non-fieldable entities and config ones.
Comment #161
YesCT CreditAttribution: YesCT commented#159: et-wizard-1810386-159.patch queued for re-testing.
Comment #162
YesCT CreditAttribution: YesCT commentedsorry @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.
Comment #162.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary to point out that it works with language alone, update the resent screenshots, update remaining tasks.
Comment #162.1
YesCT CreditAttribution: YesCT commentedUpdated issue summary fixing img src
Comment #163
Gábor HojtsyFantastic 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:
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.
Comment #164
YesCT CreditAttribution: YesCT commentedA 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.
http://drupal.org/node/1354#hookimpl
http://drupal.org/node/1354#themeable
Comment #165
klonosYeah, "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:
Comment #166
Gábor HojtsyYes, 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.
Comment #167
plachOk, 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.
Comment #168
Gábor Hojtsy@plach: I don't see any other problems that would hold this up.
Comment #169
plachI just run an analysis through the WAVE toolbar and in the relevant page area it found:
'item'
form element, not something related to this patch;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.
Comment #170
plachLatest patch review + fixes.
This comment needs to be updated.
Comment not wrapping correctly.
Wrong search/replace.
Actually this permission should be used to limit access to the translation settings.
Comment #172
plach#170: et-wizard-1810386-170.patch queued for re-testing.
Comment #173
Gábor HojtsyLooks good to me with these updates, the data loss bug has been fixed and accessibility concerns have been resolved as far as I see.
Comment #174
YesCT CreditAttribution: YesCT commented#170: et-wizard-1810386-170.patch queued for re-testing.
Comment #175
falcon03 CreditAttribution: falcon03 commentedSorry, 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...
Comment #176
YesCT CreditAttribution: YesCT commentedThe test server does not have the most recent #170 patch. @falcon03 should we get it live?
Comment #177
falcon03 CreditAttribution: falcon03 commented@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).
Comment #178
plach@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.
Comment #179
falcon03 CreditAttribution: falcon03 commented@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:
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.
Comment #180
YesCT CreditAttribution: YesCT commented@falcon03
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.
Comment #181
YesCT CreditAttribution: YesCT commentedSo 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.Comment #182
YesCT CreditAttribution: YesCT commentedsetting 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.
Comment #183
plachHere it is! Hopefully we should be done :)
Comment #184
YesCT CreditAttribution: YesCT commentedThe code looks clean, and it does the fix for 1. and 2. :)
here are some updated screen shots.
With no entity translation enabled:
Comment #184.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary still fixing img link
Comment #184.1
YesCT CreditAttribution: YesCT commentedUpdated issue summary with recent screenshots and updated remaining tasks!
Comment #184.2
YesCT CreditAttribution: YesCT commentedcorrected comment that screenshots are for.
Comment #185
webchickYEAH!!!! Awesome work, everyone! :D
Committed and pushed to 8.x. YEEHAW!
Comment #186
Gábor HojtsyWoot, woot! Thanks everyone, fabulous work!
Comment #187
nod_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.
Comment #188
Gábor HojtsyRemoving sprint tag.
Comment #189
Gábor HojtsyAny accessibility issues remaining? #1855036: ARIA and accessibility improvements in entity (content) translation settings page
Comment #190
YesCT CreditAttribution: YesCT commentedThis might have used an api that would be an example useful in #1446382: Need a reliable way to determine if a specific bundle for an entity type is translatable
Comment #192
YesCT CreditAttribution: YesCT commented#1977784: Content language settings configuration page needs to determine what entities and bundles to include is looking at how to decide what is listed on this page.
Comment #192.0
YesCT CreditAttribution: YesCT commentednit: updating comment number for screenshot
Comment #193
Gábor Hojtsy@alexpott asked for a change notice on a followup, so added one at http://drupal.org/node/1987978 and included this issue on it.
Comment #194
YesCT CreditAttribution: YesCT commented#2019485: 500 Internal server error, during batch enable of translation from the content language settings page
Comment #194.0
YesCT CreditAttribution: YesCT commentedadded new follow up for deciding how this page is build
Comment #194.1
YesCT CreditAttribution: YesCT commentedadding new issue that relates to this. might be a follow-up. at least it's linked now.
Comment #195
Gábor HojtsyNow 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
Comment #196
plachComment #197
Gábor HojtsyIn #2355909: language.settings config is not scalable, @chx argues this screen will not work in real life.
Comment #198
mgifford