Problem/Motivation

Install Drupal in Dutch (Nederlands). Go edit a view. Go to view the tour on the top right of the page. The view has 4 pages instead of the 10 expected.

This is due to how the tour expects class names and IDs to match on the page to place tips. Views however uses the translated name label of the UI section to add a class, see views-ui-display-tab-bucket.html.twig where the class is added and template_preprocess_views_ui_display_tab_bucket where the name is copied from the element and ViewsEditForm::getDisplayDetails() where the #name is taken from a bucket title. The bucket title is only available in a translated form and may be coming from a handler label as well in Views::getHandlerTypes().

Proposed resolution

Add some way to pass on a non-translated value to use for a class so CSS and tour can tie to something predictable.

Remaining tasks

Agree on approach. Implement. Review. Commit.

Files: 
CommentFileSizeAuthor
#59 views_translated_class_names-2409581-59-complete.patch7.63 KBgeertvd
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,990 pass(es). View
#59 views_translated_class_names-2409581-59-tests.patch2.62 KBgeertvd
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,946 pass(es), 6 fail(s), and 0 exception(s). View
#59 interdiff.txt4.01 KBgeertvd
#55 views_translated_class_names-2409581-55-complete.patch8.24 KBgeertvd
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,932 pass(es). View
#55 interdiff.txt676 bytesgeertvd
#53 views_translated_class_names-2409581-53-complete.patch7.58 KBgeertvd
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,953 pass(es). View
#53 interdiff.txt7.09 KBgeertvd
#48 views_translated_class_names-2409581-48-complete.patch7.76 KBgeertvd
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,919 pass(es). View
#48 views_translated_class_names-2409581-48-tests.patch3.23 KBgeertvd
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,912 pass(es), 6 fail(s), and 0 exception(s). View
#48 interdiff.txt1.48 KBgeertvd
#44 views_translated_class_names-2409581-44-complete.patch7.74 KBgeertvd
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,584 pass(es). View
#44 views_translated_class_names-2409581-44-tests.patch3.21 KBgeertvd
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,569 pass(es), 6 fail(s), and 0 exception(s). View
#36 tour-correct-page-number.jpg210.73 KBbalagan
#33 views_translated_class_names-2409581-33.patch4.58 KBgeertvd
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,215 pass(es). View
#33 interdiff.txt579 bytesgeertvd
#28 views_translated_class_names-2409581-28.patch4.58 KBgeertvd
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,208 pass(es), 2 fail(s), and 0 exception(s). View
#28 interdiff.patch6.04 KBgeertvd
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch interdiff_30.patch. Unable to apply patch. See the log in the details link for more information. View
#24 views_translated_class_names-2409581-24.patch3.24 KBgeertvd
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,251 pass(es), 1 fail(s), and 1 exception(s). View
#17 dutch-tour2.jpg394.06 KBbalagan
#11 language.zip20.44 KBbalagan

Comments

balagan’s picture

Just a wild guess, is it related to https://www.drupal.org/node/2363099 ?

balagan’s picture

Issue summary: View changes
balagan’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Tours are not content, so #2363099: Using translated field definition descriptions in entity schema results in different schema definitions, resulting in update.php changes should not be related. I'm surprised if this this happens because we merge translations into original config, so it should not remove items.

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-config
balagan’s picture

I had difficulties to reproduce this issue. I had to reroll to commit 6efd90c8dced579e1624f35e86f75a17ea71cf04, but even then, the problem only appeared after adding the hebrew language, and the Hungarian language added earlier had correct number of pages (10).

balagan’s picture

Steps to reproduce: Install D8, enable language module, enable interface translation, add a language (af), add another language (nl), the tour for the second language I have added only had 4 pages. The first language added had 10 pages. This only occured on the admin/structure/views/view/content tour pages, for example on admin/config/regional/translate the tours had same number of pages.

Gábor Hojtsy’s picture

Please export all your configuration at Admin > Configuration > Configuration management. You get a package download. Unzip that and look at the language directory and the yml files there. Can you post the two overrides for those two languages here?

Gábor Hojtsy’s picture

Status: Active » Postponed (maintainer needs more info)
balagan’s picture

Ok, I have removed the languages, and added back in different order (nl first, then af) the problem did not occured. (I am tearing my hair already). I will export config when I manage to reproduce the problem again.

balagan’s picture

FileSize
20.44 KB

After adding af and nl in this sequence this issue appeared again. I did a full export and zipped the language directory containing the two language overrides.

Gábor Hojtsy’s picture

Status: Postponed (maintainer needs more info) » Active

So the Afrikaans has one item in the override:

tips:
  views-ui-preview:
    label: Voorskou

The Dutch one has three:

tips:
  views-ui-format:
    label: Weergaveformaat
  views-ui-fields:
    label: Velden
  views-ui-preview:
    label: Voorbeeldweergave

Neither has 10 page titles translated. In fact the Afrikaans has less translated. So that that would display 10 and the Dutch would display 4 sounds strange still :) I am glad it is reproducible.

balagan’s picture

Yeah, and as I mentioned above, if I add them in different order (nl first, then af), everything is fine.

Gábor Hojtsy’s picture

Looked into this. Tour steps are generated into the page markup at the end:

<div class="item-list"><ol id="tour" class="hidden"><li ....</div>

When you load the bogus page BEFORE you hit the Tour button, it has 10 steps, ie "4 van 10", "8 van 10" etc. When you hit the tour button, it is reduced to 4 steps visible int the rewritten page as well.

Gábor Hojtsy’s picture

tour.js has a _removeIrrelevantTourItems() method which does the removal of items based on some criteria and the renumbering of steps which is happening in this case.

Gábor Hojtsy’s picture

Title: Non English tours have less pages than English. » Views UI translates class names for buckets (and possibly others)
Priority: Normal » Major
FileSize
363.59 KB

Indeed, what I found stepping into this is that the tour items with ID attachments were kept. The tour items with class attachments are not. The classes in the markup are translated. Should not be:

I think this may lead to bigger problems, therefore major.

balagan’s picture

FileSize
394.06 KB

My classes are not translated. Interestingly, the condition: itemClass && $document.find('.' + itemClass).length will not equal true, because $document.find('.' + itemClass).length is zero. On the correct 'af' page it is 1.

Gábor Hojtsy’s picture

@balagan: those are the class name that are being looked for (as defined in the tour). Go up in the markup and look at the class names in views. The problem is tour looks for class names not in the document, because views translates them.

The bug seems to be in core/modules/views_ui/templates/views-ui-display-tab-bucket.html.twig:

/**
 * ... Available variables:
 * - name: The name of the bucket, e.g. "Fields", "Filter Criteria", etc. ...
 */
#}
{%
  set classes = [
    name ? name|clean_class,
  ]
%}
<div{{ attributes.addClass(classes) }}>
...
</div>
Gábor Hojtsy’s picture

Title: Views UI translates class names for buckets (and possibly others) » Views UI generates translated HTML class names for handlers / buckets

Better title. The translated handler / bucket names come from eg. Views.php (static::t('Sort criteria')), (static::t('Filter criteria')) as well as displays such as DisplayPluginBase ($this->t('Format')).

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.

balagan’s picture

I've got as far, that the name variable in \core\modules\views_ui\templates\views-ui-display-tab-bucket.html.twig is already translated.
Don't know where that is coming from.

balagan’s picture

Component: tour.module » views_ui.module
balagan’s picture

Ok, name comes from here: https://api.drupal.org/api/drupal/core%21modules%21views_ui%21views_ui.t...
Now I know what Gábor was talking about :)

geertvd’s picture

FileSize
3.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,251 pass(es), 1 fail(s), and 1 exception(s). View

Not sure if this is the way since the class names are not so descriptive anymore to go but this fixes it.

geertvd’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 24: views_translated_class_names-2409581-24.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: +sprint

@geertvd: that seems to be failing on this test:

    $areas = array(
      'header' => 'header',
      'footer' => 'footer',
      'empty' => 'no-results-behavior',
    );

    // Assert that the expected text is found in each area category.
    foreach ($areas as $type => $class) {
      $element = $this->xpath('//div[contains(@class, :class)]/div', array(':class' => $class));
      ....
    }
geertvd’s picture

FileSize
6.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch interdiff_30.patch. Unable to apply patch. See the log in the details link for more information. View
4.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,208 pass(es), 2 fail(s), and 0 exception(s). View

Apparently changing those class names wasn't a good idea since this breaks some other tests.
In this one I'm just passing the untranslated title along.

geertvd’s picture

Status: Needs work » Needs review

The last submitted patch, 28: interdiff.patch, failed testing.

geertvd’s picture

ignore interdiff failing, I should use txt extension there.

Status: Needs review » Needs work

The last submitted patch, 28: views_translated_class_names-2409581-28.patch, failed testing.

geertvd’s picture

FileSize
579 bytes
4.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,215 pass(es). View
balagan’s picture

It looks good now, I will try manual testing of the patch.

balagan’s picture

Status: Needs work » Needs review
balagan’s picture

FileSize
210.73 KB

I can confirm that with the patch applied the tour translations display with proper number of pages.

Status: Needs review » Needs work

The last submitted patch, 33: views_translated_class_names-2409581-33.patch, failed testing.

dawehner’s picture

Good catch!

balagan’s picture

+++ b/core/modules/views_ui/src/ViewEditForm.php
@@ -556,7 +556,7 @@ public function getDisplayDetails($view, $display) {
-        $build['columns'][$column][$id]['#name'] = !empty($bucket['title']) ? $bucket['title'] : $id;

I guess this causes the tests to fail.

balagan’s picture

+++ b/core/modules/views_ui/src/ViewEditForm.php
@@ -556,7 +556,7 @@ public function getDisplayDetails($view, $display) {
-        $build['columns'][$column][$id]['#name'] = !empty($bucket['title']) ? $bucket['title'] : $id;
+        $build['columns'][$column][$id]['#name'] = $id;

I guess this causes the tests to fail.

balagan’s picture

Status: Needs work » Reviewed & tested by the community

Since after rerunning the test it is successful, and by testing it manually I see that the problem is solved, I change it to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Nice find. Looks like a test would be good.

geertvd’s picture

FileSize
3.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,569 pass(es), 6 fail(s), and 0 exception(s). View
7.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,584 pass(es). View

Added test to ViewsUITourTest

geertvd’s picture

Status: Needs work » Needs review

The last submitted patch, 44: views_translated_class_names-2409581-44-tests.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests

Looks really good. Some notes:

  1. +++ b/core/modules/views_ui/src/Tests/ViewsUITourTest.php
    @@ -50,4 +51,77 @@ public function testViewsUiTourTips() {
    +    $ln = 'nl';
    

    Let's name this variable $langcode, that is our usual variable naming for this.

  2. +++ b/core/modules/views_ui/src/Tests/ViewsUITourTest.php
    @@ -50,4 +51,77 @@ public function testViewsUiTourTips() {
    +    $this->importPoFile($this->getPoFile(), array(
    +        'langcode' => $ln,
    +      ));
    

    Indentation is off. Should not end up indented twice.

  3. +++ b/core/modules/views_ui/src/Tests/ViewsUITourTest.php
    @@ -50,4 +51,77 @@ public function testViewsUiTourTips() {
    +    $this->assertTourTips();
    

    Wow I did not know this exists. Wow. Huh. Nice.

geertvd’s picture

FileSize
1.48 KB
3.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,912 pass(es), 6 fail(s), and 0 exception(s). View
7.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,919 pass(es). View
  1. Fixed
  2. Fixed
  3. Stumbled upon that one by accident, my test looked totally different before that.
geertvd’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks great, thanks!

The last submitted patch, 48: views_translated_class_names-2409581-48-tests.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/views/src/Views.php
@@ -419,6 +419,7 @@ public static function pluginList() {
+   *   - otitle: The original untranslated title of the handler type.

I'm not sure that adding yet another thing to this array is worth it. How about using the key instead? Yes that will change the class names but we storing the original title for class names feels ott.

geertvd’s picture

FileSize
7.09 KB
7.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,953 pass(es). View

I agree, should have sticked with my thoughts on #24 and just fix the breaking tests.
Hope this one passes.

Gábor Hojtsy’s picture

Now that we are modifying CSS classes, are there CSS selector matching to those classes?

geertvd’s picture

FileSize
676 bytes
8.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,932 pass(es). View

Found one in a js file, this is supposed to hide the "contextual filter"-textfield in preview but that feature seems broken perhaps a new (minor) issue should be opened for that?

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

I also looked around and did not find other CSS/JS affected. This should be all good now.

dawehner’s picture

+++ b/core/modules/views_ui/src/Tests/ViewsUITourTest.php
@@ -50,4 +51,77 @@ public function testViewsUiTourTips() {
+    // Add Dutch language programmatically.
+    ConfigurableLanguage::createFromLangcode($langcode)->save();
+    // Import a .po file.
+    $this->importPoFile($this->getPoFile(), array(
+      'langcode' => $langcode,
+    ));
+

Is there a reason we don't just save the translated strings via an API function?

Gábor Hojtsy’s picture

Yeah that is possible to do, although that test would need to make sure to clear all caches proper to reproduce the bug then which may not be very nice either :) #1785086: Introduce a generic API for interface translation strings provided an API.

geertvd’s picture

FileSize
4.01 KB
2.62 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,946 pass(es), 6 fail(s), and 0 exception(s). View
7.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,990 pass(es). View

Fixed this based based on #1785086: Introduce a generic API for interface translation strings. I didn't have to clear the cache to make this work locally though. Don't know if the testbot is going to do this differently.

geertvd’s picture

Status: Reviewed & tested by the community » Needs review

The last submitted patch, 59: views_translated_class_names-2409581-59-tests.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. Indeed simpler than importing a .po file.

dawehner’s picture

Great, thank you!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed fca50d2 and pushed to 8.0.x. Thanks!

  • alexpott committed fca50d2 on 8.0.x
    Issue #2409581 by geertvd, balagan: Views UI generates translated HTML...
Gábor Hojtsy’s picture

Issue tags: -sprint

Yay and congrats geertvd, balagan!

Status: Fixed » Closed (fixed)

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