Content translation overview should add destination query parameter

Thanks for the patch. I did a review for clarity and formatting and didn't see anything wrong except one comment could be added to the original code (see below). I'll do some testing.

1. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationLocaleTest.php
   @@ -376,24 +383,26 @@ public function testLanguageIndependentContentModeration() {
   +    $add_translation_fr_path = $node3->toUrl('drupal:content-translation-add')
   +      ->setRouteParameter('source', 'en')
   +      ->setRouteParameter('target', 'fr')
   +      ->setOption('language', $language_fr);
   

   Nitpick: I know this is from the original code, but it would be good to add comment to make it clear why only French is tested here since Italian is tested for the other tests.

I have tested as follows:

1. Enabled language and content translation modules
2. Added Spanish
3. Configured content types to be translatable
4. Added node 1
5. Went to overview page and saw destination for edit and add links
6. Tested edit and add links and they take you back to overview page after submitting form

Given this is working and the code seems fine (except minor nitpick with original code in comment #7 which could be addressed), I'm going to mark this RTBC.

I've tested this manually and I think this makes a lot of sense. But I have one concern. At the moment when you add a new translation to an existing node you can taken to the view the translation once you are finished. This patch changes that behaviour - either taking you to admin/content or node/NID/translate depending on how you started. That's quite a big change that I'm not sure is correct because adding is very different from editing something that exists already. I think that this needs discussion.

Ah, good catch, @alexpott. I was only testing from the translate page. I've tested from the admin/content page and added some screenshots to show the issue. I think that UX is ok myself since it gets you back to where you started but I can see why someone might argue to redirect to the translation overview if you did a translation. For the latter, the destination should get altered on the links on the translate page to swap out the admin/content path?

Patch from #6 applies cleanly to 9.1.x. Kicking off tests.

Applied patch #6 successfully on v9.1.3, but it no longer applies on v9.1.4

Per #18, needs reroll.

Applied reroll, but when trying to do interdiff i was getting the following error

1 out of 1 hunk FAILED -- saving rejects to file /var/folders/f3/zj16k4ps65xf1dvqsd469dq00000gn/T//interdiff-1.6EaWYN.rej
interdiff: Error applying patch1 to reconstructed file

does anyone know why this is happening.

Patch for 9.2.x

A small typo in the previous patch (3025384-21_0.patch)

// Add a French translation (revision 3).;

Needs re-roll against 9.3.x branch so moving to Needs work.

Re-rolled patch against 9.3.x. Please review

Tried to fix custom failures.

Tagging for testing.

Bringing up #11 from @alexpott again as no one besides myself has chimed in on that. All the other comments after that were for version changes and re-rolls.

But I have one concern. At the moment when you add a new translation to an existing node you can taken to the view the translation once you are finished. This patch changes that behaviour - either taking you to admin/content or node/NID/translate depending on how you started. That's quite a big change that I'm not sure is correct because adding is very different from editing something that exists already. I think that this needs discussion.

No need for manual testing until we decide that this new behavior is what we want.

Re-rolled #27 for 9.4

#31 works for me; manually tested on Drupal 9.4.8, with 2 languages enabled; tested both explicit and implicit destination.

Tagging for usability review for the issue brought up in #11

As I learn more realized this should move to NW for best approach for #11. Then when a decision is made should it be sent to usability.

Decision should be documented in issue summary with reason why.

Usability review

We've discussed this issue at #3358491: Drupal Usability Meeting 2023-05-12. The recording of the meeting can be found under the following link: https://youtu.be/Z22Hbe3yQtg?t=552

For the record, the attendees at the usability meeting were @aaronmchale, @benjifisher, @rkoller, and @shaal.

At first apologies that it took a while. :( The issue was on the meetings shortlist for a few weeks now, but we only got around to discussing it about three weeks ago. During the write-up I’ve stumbled across a few more details as well as an issue with multilingual in Drupal 10.1.x-dev that delayed things further plus the last one or two weeks were a plain nightmare making me unable to finish the write-up. Sorry again.

During the meeting we've compared the current with the patched state in the different contexts and tasks at hand on Drupal 10.1.x-dev with three languages, English, German, and French, installed. On the Detection and selection-page, Account administration pages is checked and on the user profile page site language and administration pages language is set to English and the translations for the content type Article is activated. In the first table, I have listed the destination parameters of each context and it’s tasks.

In the second table the destinations for the different contexts are listed. That way it was easier to compare the behavior.

Without the patch applied on save, in every context and for every task, the user is getting redirected to the View-tab page of a node, except in example C2 where the user is getting redirect back to admin/content. That way the destination was sort of consistent and predictable but the downside was that the user's flow was interrupted by getting redirected to the node’s page most of the time.

With the patch applied the behavior gets a bit more diverse and potentially confusing across the different contexts and tasks (C1-C6). The user is getting redirected back to admin/content</code in the context of <code>admin/content where you’ve started off (C1-C3) while getting redirected in most cases “half way” to the translation overview page (C4&C6) instead of the nodes view-tab page (C5) in the context of nodes, as pointed out in #11.
In general it is nearly close to impossible based on the potential different scenarios ("are you building out a site and its content" or "are you just governing already existing content"), contexts (admin/content or the node page), and the tasks at hand (add a node, add a translation after saving a node, editing a node, editing a translation after saving a node and so on) to manage everything with a single Save button and at the same time challenging for the user to remember all the different destinations based on the current context and task at hand.

There was an agreement it would be clearer to add a secondary save button, a pattern that is utilized on the Add term-page. There you have a Save and a Save and go to list-button. In the context of the current patch it would make sense adopting the aforementioned pattern for C1, C3, C4 and C6. With save the user would be redirect to the node’s view page while with the other button the user could be directed back to the translation overview page.

In regards of the button label one worry the group had was coming up with a brief and concise label with two to three words tops. Save and go to list is already too long for example. One idea that came up was Save and translate which would concise enough but is way too ambiguous.

So the suggestion, not a clear recommendation, for the next steps would be adding a secondary button to C1, C3, C4 and C6 with the following labels:

Save (this translation) (no destination parameter - the user gets redirected, depending on the context, to admin/content or the View-tab of the corresponding node)
Save and go to list (use the translation overview page as the destination parameter - user gets redirected to the translation overview page of the current node)

Then opening a follow-up issue about shortening and clarifying the button label. Save and go to list is too long and “list” might also be confused with the list of content on admin/content.

The other option for a follow-up issue might be to extend the adoption of the secondary button pattern to C2 and C5 but using another destination parameter there. But all that would be out of the scope for this issue.

During the write-up for #38, when i was testing and also collecting feedback in a meetup, I ran into two more details to take into consideration:

1. @rocketeerbkw noted during the Drupal Dojo Austin that the current patch doesn’t preserve the filter settings you were using on admin/content. If filter for example for Basic page and then edit or add a translation the destination you are getting redirected to in the end is again /admin/content instead of /admin/content?title=&type=page&status=All&langcode=All. it would be reasonable that the user is returned to the page and filter setting that one left off.

2. In the context of the translation overview page for individual nodes having the translation overview as the destination causes another problem. It is sort of difficult to grasp the language context you are in on the translation overview page. In the environment i had set up i have three languages (english, german, french) installed and the administration language is set to english.

in the current screenshot the admin language changed to german even though the admin language is set to english. i was editing the german node. when being redirected to the translation overview page the status message says that the german article was updated, the h1 says these are the translations for the french article, while in the language list you have english bolded as the original language. if you have a short working memory, or you get distracted or are away from your computer for a while and return you will have to at least think for a moment in which context you actually are and which version you will get to when you hit the edit tab (bearbeiten). It is sort of difficult due to #2313309: Admin toolbar, Navigation and contextual links should always be rendered in the admin language (if set) and #2864972: Content and interface translation don't clearly separate to reproduce the setting i illustrate in the screenshot consistently. But nevertheless the edit button is sort of problematic with the translation overview page as the destination. Definitely out of the scope of this issue but i think it would be reasonable to have at least a discussion.