Search results of multi-lingual pages fail Language of Parts

Thanks for the issue and patch. The patch still applies to 9.1.x.

It looks like the quotes (”) are not correct and need to be updated so moving back to needs work.

1. +++ b/core/themes/stable/templates/content/search-result.html.twig
   @@ -57,13 +57,13 @@
   +<h3{{ title_attributes }} lang=”{{ langcode }}”>
   

2. +++ b/core/themes/stable/templates/content/search-result.html.twig
   @@ -57,13 +57,13 @@
   +  <p{{ content_attributes }} lang=”{{ langcode }}”>{{ snippet }}</p>
   

3. +++ b/core/themes/stable/templates/content/search-result.html.twig
   @@ -57,13 +57,13 @@
   +  <p lang=”{{ langcode }}”>{{ info }}</p>
   

working on this.

Addressed #8 please review.

Thanks for the update.

1) Patch applies cleanly.

2) Tests pass.

3) New patch addresses the quote issues from #8.

4) Unlikely tests need to be added.

5) Not sure this can be manually tested since this is stable.

6) Also, I am wondering if this change would even be allowed. Normally stable can't be changed.

7) These are all the search-result.html.twig files. None of the other twig files have lang added. Seems like it should be except for probably stable9. It would be good to get direction from @mgifford on this.

./core/profiles/demo_umami/themes/umami/templates/classy/content/search-result.html.twig
./core/modules/search/templates/search-result.html.twig
./core/themes/stable/templates/content/search-result.html.twig
./core/themes/stable9/templates/content/search-result.html.twig
./core/themes/bartik/templates/classy/content/search-result.html.twig
./core/themes/claro/templates/classy/content/search-result.html.twig
./core/themes/classy/templates/content/search-result.html.twig
./core/themes/seven/templates/classy/content/search-result.html.twig

Re. #11.7 - This change should go into all of the search-result.html.twig copies, including stable and stable9.

This sort of change is permitted in the stable themes. It's machine-readable accessibility metadata, and mostly non-disruptive as it doesn't affect the element names or nesting. There may be some custom themes with [lang] attribute selectors in their CSS, but if they are styling things according to language then they likely want to target the search results anyway. We'll need a change record to notify theme and distro maintainers, and advise them to follow suit.

Thanks to you both for the feedback. Back to needs work to fix all the files.

Working on this.

Fixed all the files mentioned in #11.7. Please review.

Resolving the failed test cases from #16

#18 LGTM and Applied successfully.

Unrelated test failure. Moving back to Needs review

1) Drupal\Tests\quickedit\FunctionalJavascript\QuickEditIntegrationTest::testArticleNode
Field node/1/body/en/full did not match its expectation selector (.cke_editable_inline), actual HTML: Hello world!I do not know what to say…I wish I were eloquent.
Failed asserting that 'Hello world!I do not know what to say…I wish I were eloquent.' is true.

I think we should also modify olivero templates. So far looks good to me.

Patch created with the changes mentioned in #24, Please have a look.

Thanks for the update. Based on the following and the previous couple comments, marking this RTBC.

1) Patch still applies cleanly.

2) Tests passed.

3) These are all the search-result.html.twig files:

./core/profiles/demo_umami/themes/umami/templates/classy/content/search-result.html.twig
./core/modules/search/templates/search-result.html.twig
./core/themes/stable/templates/content/search-result.html.twig
./core/themes/stable9/templates/content/search-result.html.twig
./core/themes/bartik/templates/classy/content/search-result.html.twig
./core/themes/claro/templates/classy/content/search-result.html.twig
./core/themes/classy/templates/content/search-result.html.twig
./core/themes/seven/templates/classy/content/search-result.html.twig
./core/themes/olivero/templates/content/search-result.html.twig

These are the files changed by patch which cover all of the files above for all themes:

	modified:   core/modules/search/templates/search-result.html.twig
	modified:   core/profiles/demo_umami/themes/umami/templates/classy/content/search-result.html.twig
	modified:   core/tests/Drupal/KernelTests/Core/Theme/ConfirmClassyCopiesTest.php
	modified:   core/themes/bartik/templates/classy/content/search-result.html.twig
	modified:   core/themes/claro/templates/classy/content/search-result.html.twig
	modified:   core/themes/classy/templates/content/search-result.html.twig
	modified:   core/themes/olivero/templates/content/search-result.html.twig
	modified:   core/themes/seven/templates/classy/content/search-result.html.twig
	modified:   core/themes/stable/templates/content/search-result.html.twig
	modified:   core/themes/stable9/templates/content/search-result.html.twig

ConfirmClassyCopiesTest change is modifying the hash for the search-result.html.twig file. I didn't find any other place using a hash like this.

4) All changes besides for olivero are for h3 (title) and p (snippet and info).

olivero uses div instead of p and doesn't have {{ info }} in the file for some reason.

core/modules/search/templates/search-result.html.twig
+<h3{{ title_attributes }} lang="{{ langcode }}">
+  <p{{ content_attributes }} lang="{{ langcode }}">{{ snippet }}</p>
+  <p lang="{{ langcode }}">{{ info }}</p>

core/profiles/demo_umami/themes/umami/templates/classy/content/search-result.html.twig
+<h3{{ title_attributes.addClass('search-result__title') }} lang="{{ langcode }}">
+    <p{{ content_attributes.addClass('search-result__snippet') }} lang="{{ langcode }}">{{ snippet }}</p>
+    <p class="search-result__info" lang="{{ langcode }}">{{ info }}</p>

core/themes/bartik/templates/classy/content/search-result.html.twig
+<h3{{ title_attributes.addClass('search-result__title') }} lang="{{ langcode }}">
<p{{ content_attributes.addClass('search-result__snippet') }} lang="{{ langcode }}">{{ snippet }}</p>
<p class="search-result__info" lang="{{ langcode }}">{{ info }}</p>

core/themes/claro/templates/classy/content/search-result.html.twig
+<h3{{ title_attributes.addClass('search-result__title') }} lang="{{ langcode }}">
+    <p{{ content_attributes.addClass('search-result__snippet') }} lang="{{ langcode }}">{{ snippet }}</p>
+    <p class="search-result__info" lang="{{ langcode }}">{{ info }}</p>

core/themes/classy/templates/content/search-result.html.twig
+<h3{{ title_attributes.addClass('search-result__title') }} lang="{{ langcode }}">
+    <p{{ content_attributes.addClass('search-result__snippet') }} lang="{{ langcode }}">{{ snippet }}</p>
+    <p class="search-result__info" lang="{{ langcode }}">{{ info }}</p>

core/themes/olivero/templates/content/search-result.html.twig
+  <h3{{ title_attributes.addClass('search-result__title') }} lang="{{ langcode }}">
+  <div{{ content_attributes.addClass('search-result__snippet', layout, 'text-content') }} lang="{{ langcode }}">{{ snippet }}</div>

core/themes/seven/templates/classy/content/search-result.html.twig
+<h3{{ title_attributes.addClass('search-result__title') }} lang="{{ langcode }}">
+    <p{{ content_attributes.addClass('search-result__snippet') }} lang="{{ langcode }}">{{ snippet }}</p>
+    <p class="search-result__info" lang="{{ langcode }}">{{ info }}</p>

core/themes/stable/templates/content/search-result.html.twig
+<h3{{ title_attributes }} lang="{{ langcode }}">
+  <p{{ content_attributes }} lang="{{ langcode }}">{{ snippet }}</p>
+  <p lang="{{ langcode }}">{{ info }}</p>

core/themes/stable9/templates/content/search-result.html.twig
+<h3{{ title_attributes }} lang="{{ langcode }}">
+  <p{{ content_attributes }} lang="{{ langcode }}">{{ snippet }}</p>
+  <p lang="{{ langcode }}">{{ info }}</p>

We should have test coverage. Also we need to work out why the code in template_preprocess_search_result()

  if (isset($result['language']) && $result['language'] != $language_interface->getId() && $result['language'] != LanguageInterface::LANGCODE_NOT_SPECIFIED) {
    $variables['title_attributes']['lang'] = $result['language'];
    $variables['content_attributes']['lang'] = $result['language'];
  }

is not working for us.

Plus that code implies that we should only have a lang attribute when it doesn't match the page. @catch said:

If we print it for one search result snippet, we'll need to print it for the next to return to the original language.

so maybe this should always be there but I do think that we need to fix the preprocess so it works how we want it to work.

Ah... $result['language'] does not exist! It's $result['langcode']...

+++ b/core/modules/search/templates/search-result.html.twig
@@ -59,13 +59,13 @@
 {{ title_prefix }}
-<h3{{ title_attributes }}>
+<h3{{ title_attributes }} lang="{{ langcode }}">
   <a href="{{ url }}">{{ title }}</a>
 </h3>

Also - why can't lang be included in title_attributes - it's just an attribute. If so, we wouldn't need all the template changes here at all.

@catch
By adding langcode directly into title_attributes does the trick.
Apart from that we need to add langcode variable so inline_template can use it.
I've updated the patch accordingly and added a few test cases as well.

Fixing test cases and updating md5 hash.

Tests are passing on local:

vendor/bin/phpunit -c core --filter=ConfirmClassyCopiesTest
PHPUnit 8.5.13 by Sebastian Bergmann and contributors.

Testing
.....                                                               5 / 5 (100%)

Time: 1.2 minutes, Memory: 808.00 MB

OK (5 tests, 2481 assertions)

1. +++ b/core/modules/search/search.pages.inc
   @@ -30,9 +30,11 @@ function template_preprocess_search_result(&$variables) {
   +    // Adding additional variable to be used for inline_template.
   +    $variables['langcode'] = $result['langcode'];
   

   Due to reverting all the changes to the templates this is no longer necessary.

2. +++ b/core/modules/search/templates/search-result.html.twig
   @@ -67,5 +67,5 @@
   -  <p>{{ info }}</p>
   +  <p lang="{{ langcode }}">{{ info }}</p>
   
   +++ b/core/profiles/demo_umami/themes/umami/templates/classy/content/search-result.html.twig
   @@ -67,6 +67,6 @@
   -    <p class="search-result__info">{{ info }}</p>
   +    <p class="search-result__info" lang="{{ langcode }}">{{ info }}</p>
   
   +++ b/core/tests/Drupal/KernelTests/Core/Theme/ConfirmClassyCopiesTest.php
   @@ -795,7 +795,7 @@ protected function getClassyHash($type, $file) {
   -        'search-result.html.twig' => '5676e5f62d82fcb1a2588da2197d1455',
   +        'search-result.html.twig' => '239b1d65540366846aaf74eb00a71c1a',
   
   +++ b/core/themes/bartik/templates/classy/content/search-result.html.twig
   @@ -67,6 +67,6 @@
   -    <p class="search-result__info">{{ info }}</p>
   +    <p class="search-result__info" lang="{{ langcode }}">{{ info }}</p>
   
   +++ b/core/themes/claro/templates/classy/content/search-result.html.twig
   @@ -67,6 +67,6 @@
   -    <p class="search-result__info">{{ info }}</p>
   +    <p class="search-result__info" lang="{{ langcode }}">{{ info }}</p>
   
   +++ b/core/themes/classy/templates/content/search-result.html.twig
   @@ -67,6 +67,6 @@
   -    <p class="search-result__info">{{ info }}</p>
   +    <p class="search-result__info" lang="{{ langcode }}">{{ info }}</p>
   
   +++ b/core/themes/seven/templates/classy/content/search-result.html.twig
   @@ -67,6 +67,6 @@
   -    <p class="search-result__info">{{ info }}</p>
   +    <p class="search-result__info" lang="{{ langcode }}">{{ info }}</p>
   
   +++ b/core/themes/stable/templates/content/search-result.html.twig
   @@ -65,5 +65,5 @@
   -  <p>{{ info }}</p>
   +  <p lang="{{ langcode }}">{{ info }}</p>
   
   +++ b/core/themes/stable9/templates/content/search-result.html.twig
   @@ -65,5 +65,5 @@
   -  <p>{{ info }}</p>
   +  <p lang="{{ langcode }}">{{ info }}</p>
   

   Are we sure that the info is in the language of the node?

   I'm not sure it is. At least $info['date'] = \Drupal::service('date.formatter')->format($result['date'], 'short'); is in the language of the page. And the only implementation in core - comment_node_search_result() - that adds extra info is in the page language and not the node language.

   I think all these changes should be reverted.

3. +++ b/core/modules/search/tests/src/Functional/SearchLanguageTest.php
   @@ -148,4 +148,27 @@ public function testLanguages() {
   +    $this->assertSession()->elementExists('xpath', '//h3[contains(@lang, "es")]');
   +    $result = $this->xpath('//h3[contains(@lang, "es")]/a');
   +    $this->assertEquals($node->getTitle(), $result[0]->getText());
   +    $this->assertSession()->elementExists('xpath', '//p[contains(@lang, "es")]');
   ...
   +    $this->assertSession()->elementExists('xpath', '//h3[contains(@lang, "en")]');
   ...
   +    $result = $this->xpath('//h3[contains(@lang, "en")]/a');
   +    $this->assertEquals($node->getTitle(), $result[0]->getText());
   +    $this->assertSession()->elementExists('xpath', '//p[contains(@lang, "en")]');
   

   These asserts are asserting on the whole page. I think we should make them only check html inside the search result.

@alexpott
I cross-checked about the "result__info" attributes. They aren't translatable.
So we can remove those changes.
I'll update the patch and remove un-necessary changes.

Updating patch with the suggestions from #33

Hello, I have reviewed patch #36. The un-necessary changes pointed by #33 where removed from the patch and the tests asserts where fixed. Since the search worked perfectly with my manual tests and the code seems fine for me, I'm moving the issue to RTBC.

// Visit the search form in spanish language.

We need to include Spanish with using Capital letter
https://www.ef.com/wwen/english-resources/english-grammar/capitalisation...

Suggestion:
// Visit the search form in the Spanish language.

;

Thanks for pointing out the capitalization! I'm lost with the patchs now(sorry for the question, still new to the Drupal), are we considering patch #36, or should we made a new patch with the grammar fix? I'm asking this because patch #39 was deleted.

Yeah, please disconsider my patch #38. It was a mistake

So yeap, are we considering the patch #36

My only consideration is to update this patch with this grammar fix on the comment. My suggestion on patch 36 is to change this line:

// Visit the search form in spanish language.

to
// Visit the search form in the Spanish language.

For this reason updating as "Needs Work"

Ok, I took the liberty to make the grammar change in the following patch.

It works

Committed and pushed bd882e9b7b to 9.2.x and cdf61eea8d to 9.1.x. Thanks!