Path class on <body> may be language specific, results in CSS bugs [#2404929]

Problem/Motivation

If you visit the /admin page, the tag contains a path-admin class, but if you visit the page in another language, like /fa/admin, the tag will contain a path-fa class and miss the path-admin class, because it uses the first segment after the / symbol (in core/includes/theme.inc -> function template_preprocess_html).
The .path-admin class is now used in a few CSS rules, which means those styles won't be applied if the path has a language prefix.

Proposed resolution

Construct that CSS class based on a non-aliased, non-processed path.

Remaining tasks

Commit.

User interface changes

CSS classes applied to path-admin will work again. Bartik has these styles:

$ git grep path-admin
core/themes/bartik/css/components/admin.css:.path-admin #content img {
core/themes/bartik/css/components/admin.css:.path-admin #content .simpletest-image img {
core/themes/bartik/css/components/admin.css:.path-admin #admin-dblog img {

API changes

None.

Comment	File	Size	Author
#29	wrong-missing-class-on-body-in-paths-with-language-prefix-2404929-28.patch	3.2 KB	segi
#29
#29	interdiff-2404929-28.txt	2.2 KB	segi
#26	interdiff-2404929-26.txt	3.12 KB	segi
#26	wrong-missing-class-on-body-in-paths-with-language-prefix-2404929-26.patch	2.97 KB	segi
#26
#23	wrong-missing-class-on-body-in-paths-with-language-prefix-2404929-20.patch	2.05 KB	prajaankit
#23
#19	wrong-missing-class-on-body-in-paths-with-language-prefix-2404929-19.patch	2.51 KB	dileepmaurya
#19
#17	wrong-missing-class-on-body-in-paths-with-language-prefix-2404929-17.patch	2.44 KB	dileepmaurya
#17
#14	drupal8-d8mi-issue-2404929-works.png	58.95 KB	Kristen Pol
#10	wrong-missing-class-on-body-in-paths-with-language-prefix-2404929-10.patch	1.28 KB	dileepmaurya
#10
#8	wrong-missing-class-on-body-in-paths-with-language-prefix-2404929-8.patch	1.28 KB	dileepmaurya
#8
#5	wrong-missing-class-on-body-in-paths-with-language-prefix-2404929-5.patch	1.25 KB	dileepmaurya
#5
#3	wrong-missing-class-on-body-in-paths-with-language-prefix-2404929-3.patch	716 bytes	dileepmaurya
#3
#1	wrong-missing-class-on-body-in-paths-with-language-prefix-2404929-1.patch	547 bytes	dileepmaurya
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

dileepmaurya CreditAttribution: dileepmaurya commented 11 January 2015 at 14:01

Status:

Active

» Needs review

File	Size
wrong-missing-class-on-body-in-paths-with-language-prefix-2404929-1.patch	547 bytes

Comment #2

11 January 2015 at 14:22

Status:

Needs review

» Needs work

The last submitted patch, 1: wrong-missing-class-on-body-in-paths-with-language-prefix-2404929-1.patch, failed testing.

Comment #3

dileepmaurya CreditAttribution: dileepmaurya commented 11 January 2015 at 17:36

Status:

Needs work

» Needs review

File	Size
wrong-missing-class-on-body-in-paths-with-language-prefix-2404929-3.patch	716 bytes

Comment #4

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 12 January 2015 at 15:46

Status:	Needs review	» Needs work
Issue tags:		+D8MI, +CSS, +language-base, +Novice

I don't think that is a good idea, there may be any number of other things done to the path, eg. path aliases. Does this prefix work with a path alias a /headquarters for /admin for example?

Comment #5

dileepmaurya CreditAttribution: dileepmaurya commented 12 January 2015 at 17:41

File	Size
wrong-missing-class-on-body-in-paths-with-language-prefix-2404929-5.patch	1.25 KB

Comment #6

dileepmaurya CreditAttribution: dileepmaurya commented 12 January 2015 at 17:44

Status:

Needs work

» Needs review

Comment #7

gvso

Spanish

CreditAttribution: gvso commented 13 January 2015 at 02:13

I think we shouldn't use drupal_is_front_page() in order to avoid make this patch again https://www.drupal.org/node/2388627

Comment #8

dileepmaurya CreditAttribution: dileepmaurya commented 13 January 2015 at 04:47

File	Size
wrong-missing-class-on-body-in-paths-with-language-prefix-2404929-8.patch	1.28 KB

drupal_is_front_page() is replaced with \Drupal::service('path.matcher')->isFrontPage()

Comment #9

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 13 January 2015 at 08:37

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

I cannot tell if this is technically correct or not, but it looks promising.

Please watch your code style, you should indent with 2 spaces, not 3 or 4. Also add space after if, else and before {.

Also not sure how best we can test this but it sure would be great to have test for this, since it was broken unnoticed.

Comment #10

dileepmaurya CreditAttribution: dileepmaurya commented 13 January 2015 at 09:29

File	Size
wrong-missing-class-on-body-in-paths-with-language-prefix-2404929-10.patch	1.28 KB

Formating code.

Comment #11

dileepmaurya CreditAttribution: dileepmaurya commented 13 January 2015 at 09:31

Status:

Needs work

» Needs review

Comment #12

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 13 January 2015 at 12:01

Code format looks good :) Given the change is in the theme system this can get tests AFAIS.

Comment #13

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 15 January 2015 at 14:55

Issue tags:

+SprintWeekend2015Queue

Comment #14

Kristen Pol

she/her

English

Santa Cruz, CA, USA

CreditAttribution: Kristen Pol commented 17 January 2015 at 02:49

File	Size
drupal8-d8mi-issue-2404929-works.png	58.95 KB

I tested the patch and it does indeed work as expected so then time for tests. :)

Comment #15

Kristen Pol

she/her

English

Santa Cruz, CA, USA

CreditAttribution: Kristen Pol commented 17 January 2015 at 02:50

Status:

Needs review

» Needs work

Moving back to needs work.

Comment #16

dileepmaurya CreditAttribution: dileepmaurya commented 17 January 2015 at 06:20

If any one on admin page what classes sholud be added ?
I think if we visit on admin page then one of the class should be path-admin and its not depend on url language prefix or any type of prefix and url alias.

Comment #17

dileepmaurya CreditAttribution: dileepmaurya commented 17 January 2015 at 07:00

File	Size
wrong-missing-class-on-body-in-paths-with-language-prefix-2404929-17.patch	2.44 KB

Adding language specific class in body such as lang-[language-id].

Comment #18

dileepmaurya CreditAttribution: dileepmaurya commented 17 January 2015 at 07:00

Status:

Needs work

» Needs review

Comment #19

dileepmaurya CreditAttribution: dileepmaurya commented 17 January 2015 at 07:05

File	Size
wrong-missing-class-on-body-in-paths-with-language-prefix-2404929-19.patch	2.51 KB

Comment #20

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 17 January 2015 at 07:07

Issue summary:	View changes
Status:	Needs review	» Needs work

The root HTML tag already has a lang="XY" attribute. That can already perfectly used in CSS. No need for that body class. Also not in scope for this issue anyway.

@dileepmaurya: please also post interdiffs (https://www.drupal.org/documentation/git/interdiff) so we can review what did you change and explain changes. Updated patches like 19 without comments and interdiffs is very hard to review.

Comment #21

segi CreditAttribution: segi commented 17 January 2015 at 10:33

Assigned:

Unassigned

» segi

Comment #22

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 17 January 2015 at 10:38

Issue tags:

+SprintWeekend2015

Comment #23

prajaankit CreditAttribution: prajaankit commented 17 January 2015 at 10:49

Assigned:

segi

» prajaankit

File	Size
wrong-missing-class-on-body-in-paths-with-language-prefix-2404929-20.patch	2.05 KB

#20

Comment #24

prajaankit CreditAttribution: prajaankit commented 17 January 2015 at 10:51

Status:

Needs work

» Needs review

Comment #25

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 17 January 2015 at 10:57

Status:

Needs review

» Needs work

@prajaankit:

1. Please respect if an issue was assigned to someone recently.
2. You did not post an explanation or an interdiff of what you changed.
3. You did not actually address anything in #20, despite referring to that only in your comment.

Comment #26

segi CreditAttribution: segi commented 17 January 2015 at 13:55

Assigned:	prajaankit	» segi
Status:	Needs work	» Needs review

File	Size
wrong-missing-class-on-body-in-paths-with-language-prefix-2404929-26.patch	2.97 KB

interdiff-2404929-26.txt	3.12 KB

I modified the last #23 patch based on @Gábor Hojtsy feedbacks, I created a new test and I add it to LanguageSwitchingTest test class.

Comment #27

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 17 January 2015 at 14:04

Status:

Needs review

» Needs work

Looking good now, only two notes:

+++ b/core/includes/theme.inc
@@ -1343,8 +1343,17 @@ function template_preprocess_html(&$variables) {
+  //Add a variable for language. This can be used to create a language specific class.
+  $variables['language'] = $language_interface->getId();

You don't use this anymore.

+++ b/core/modules/language/src/Tests/LanguageSwitchingTest.php
@@ -242,6 +241,26 @@ function testLanguageLinkActiveClass() {
+    // Go to admin/config page
+    $this->drupalGet('fr/admin/config');
+    $class = $this->xpath('//body[contains(@class, :class)]', array(':class' => 'path-admin'));
+    $this->assertTrue(isset($class[0]), t('The path-admin class same as on default language'));

I would also test this without the French language, to compare it also works the same on the non-foreign page.

Comment #28

17 January 2015 at 14:44

The last submitted patch, 26: wrong-missing-class-on-body-in-paths-with-language-prefix-2404929-26.patch, failed testing.

Comment #29

segi CreditAttribution: segi commented 17 January 2015 at 14:45

File	Size
interdiff-2404929-28.txt	2.2 KB
wrong-missing-class-on-body-in-paths-with-language-prefix-2404929-28.patch	3.2 KB

Ok, I fixed the patch based on feedbacks.

Comment #30

segi CreditAttribution: segi commented 17 January 2015 at 14:46

Status:

Needs work

» Needs review

Comment #31

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 17 January 2015 at 15:30

Title:	Wrong/missing class on <body> in paths with language prefix	» Path class on <body> may be language specific, results in CSS bugs
Issue summary:	View changes
Status:	Needs review	» Reviewed & tested by the community

Retitled to be more specific. Also updated issue summary. Thanks!

Comment #32

DevElCuy CreditAttribution: DevElCuy commented 17 January 2015 at 17:02

Issue tags:

-SprintWeekend2015Queue

Comment #33

DevElCuy CreditAttribution: DevElCuy commented 17 January 2015 at 17:36

Issue tags:

+SprintWeekend2015Queue

Removed SprintWeekend2015Queue by mistake.

Comment #34

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 18 January 2015 at 09:22

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

Needs a reroll

Comment #35

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 18 January 2015 at 09:35

Status:	Needs work	» Reviewed & tested by the community
Issue tags:	-Needs reroll

Actually I didn't commit the patch that causes the conflict.

Comment #36

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 18 January 2015 at 09:37

Status:

Reviewed & tested by the community

» Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed f8aa6c2 and pushed to 8.0.x. Thanks!

diff --git a/core/includes/theme.inc b/core/includes/theme.inc
old mode 100644
new mode 100755

We need to be careful not to change filemodes. Fixed on commit.

Comment #37

18 January 2015 at 09:38

alexpott committed f8aa6c2 on 8.0.x

Issue #2404929 by dileepmaurya, segi, Kristen Pol, prajaankit: Path...

Comment #38

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 19 January 2015 at 15:02

Yay superb, thanks all!

Comment #39

davidhernandez

English

USA

CreditAttribution: davidhernandez commented 21 January 2015 at 16:14

Not sure what happened with the patches, but there was back and forth with root_path being set TRUE or FALSE and html.html.twig being modified or not modified. In the end that seems to have gotten confused because path-frontpage doesn't work now with that logic.

The template uses not root_path so the preprocess functions should set it FALSE.

Comment #40

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 21 January 2015 at 16:21

How does the test work then?

Comment #41

davidhernandez

English

USA

CreditAttribution: davidhernandez commented 21 January 2015 at 16:29

The test in this issue? It looks to only be visiting admin pages, and looking for 'path-admin'. The switch in logic only affected the class for the frontpage.