Language direction missing in <html> [#208768]

Comment	File	Size	Author
#41	theme_language_direction-208768-41.patch	9.01 KB	cburschka
#37	theme_language_direction-208768-37.patch	9 KB	cburschka
#35	theme_language_direction-208768-35.patch	8.96 KB	cburschka
#34	theme_language_direction-208768-34.patch	8.96 KB	cburschka
#23	theme_language_direction-208768-23.patch	8.16 KB	cburschka
#8	html.dir_.attr_.patch	8.12 KB	dvessel

Comment #2

dvessel commented 11 January 2008 at 13:20

This should have been added long ago. It's perfectly valid even with xhtml strict.

I think it would help rtl conditions and make it more feature complete.

Log in or register to post comments

Comment #3

hass commented 11 January 2008 at 19:54

Ups, maybe this is my theme that missing this... i will take a look...

Log in or register to post comments

Comment #4

hass commented 12 January 2008 at 01:30

So - i've checked all core themes and the direction does not exists in page.tpl.php, yet.

Log in or register to post comments

Comment #5

vladimir.dolgopolov commented 15 January 2008 at 10:25

@hass: your suggestion is to add $language->direction variable into template page.tpl.php?:

<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="<?php print $language->language ?>" lang="<?php print $language->language ?>" dir="<?php print $language->direction ?>">

But as said in http://drupal.org/node/132442
"$language->direction being an intereger (0 or LANGUAGE_LTR for left to right and 1 or LANGUAGE_RTL for right to left)"
So we need a string representation of it.

Log in or register to post comments

Comment #6

gábor hojtsy

he/him

Hungarian

Hungary

commented 15 January 2008 at 10:34

Then we probably need a $direction variable generated from the page template preprocessor.

Log in or register to post comments

Comment #7

hass commented 15 January 2008 at 12:08

@vladimir.dolgopolov: Yep :-)

Log in or register to post comments

Comment #8

dvessel commented 15 January 2008 at 17:49

Status:

Active

» Needs review

Status	File	Size
new	html.dir_.attr_.patch	8.12 KB

This should cover it.

Log in or register to post comments

Comment #9

vladimir.dolgopolov commented 16 January 2008 at 02:25

I think it's better to use "direction" value as variable of $language object, not as plain var (so we use $language->language):

$language->dir
$language->direction_dir
$language->direction_value
$language->direction_text
$language->language_dir

and so on

// $language is now an object
// http://drupal.org/node/132442#language-var

Log in or register to post comments

Comment #10

dvessel commented 16 January 2008 at 02:33

It's already been established what $language contains. Within it *->dir exists and anyone who needs that data knows it's there. A separate variable is being used here specific to templates so $direction is fine.

Log in or register to post comments

Comment #11

vladimir.dolgopolov commented 16 January 2008 at 11:09

My only suggestion here: let's use "language related stuff" in templates one way - either as object or as variables.

Log in or register to post comments

Comment #12

cburschka

they

commented 16 January 2008 at 13:25

"$language->direction being an intereger (0 or LANGUAGE_LTR for left to right and 1 or LANGUAGE_RTL for right to left)"
So we need a string representation of it.

Is there a design policy that forbids the following?

<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="<?php print $language->language ?>" lang="<?php print $language->language ?>" dir="<?php print $language->direction == LANGUAGE_LTR ? 'ltr' : 'rtl' ?>">

Sorry if there are more than these two possibilities for direction; if so, the design does have to be more complex.

Log in or register to post comments

Comment #13

gábor hojtsy

he/him

Hungarian

Hungary

commented 16 January 2008 at 13:42

Arancaytar: the policy is that we'd like to have simpler templates then that. The whole preprocess magic is there to get off this burden from the themers. I see that using $direction for this and using $language->... for all others is a bit inconsistent, but I am not sure how should we stuff up the language object for the theme. What should be a good name for the direction string attribute?

Log in or register to post comments

Comment #14

cburschka

they

commented 16 January 2008 at 14:50

$language->direction_attribute or $language->dir_att? Just going by instinct...

Edit: Although using abbreviations would make it somewhat harder to follow.

Log in or register to post comments

Comment #15

hass commented 16 January 2008 at 17:03

"$language->dir" sounds good to me.

Log in or register to post comments

Comment #16

vladimir.dolgopolov commented 17 January 2008 at 07:38

So we have 2 options concerning 'dir' variable:

a) update only "includes\theme.inc:template_preprocess_page()" and use it only for themes, like this:

  $variables['language']        = $GLOBALS['language'];
  $variables['language']->dir   = $GLOBALS['language']->direction == LANGUAGE_LTR ? 'ltr' : 'rtl';

b) set the variable on early stages and use it in scripts (core, modules) too.

I'd prefer (a).

Log in or register to post comments

Comment #17

gábor hojtsy

he/him

Hungarian

Hungary

commented 17 January 2008 at 11:40

I think it is fine to have it in the page theme only for now. AFAIK, no other theme functions expose the language so far. If they would, we would need to do this somewhere more central to avoid code repetition, but language support in Drupal 6 is on the page level.

Log in or register to post comments

Comment #18

hass commented 17 January 2008 at 22:21

@Gabor: does "page level" means - only:

<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="<?php print $language->language ?>" lang="<?php print $language->language ?>" dir="<?php print $language->direction == LANGUAGE_LTR ? 'ltr' : 'rtl' ?>">

or a $direction variable and this code:

<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="<?php print $language->language ?>" lang="<?php print $language->language ?>" dir="<?php print $direction ?>">

Log in or register to post comments

Comment #19

gábor hojtsy

he/him

Hungarian

Hungary

commented 17 January 2008 at 23:03

hass: my comment on page level language handling has no relation to any source code whatsoever, it just means we don't have a feature to specify a language of a paragraph or a table. We do have node language support, but we don't specify it's direction on the node template; our CSS also sets a global direction attribute for the whole page.

Log in or register to post comments

Comment #20

hass commented 17 January 2008 at 23:09

Hmmm... and what are we doing now? :-)

Log in or register to post comments

Comment #21

gábor hojtsy

he/him

Hungarian

Hungary

commented 18 January 2008 at 00:04

$language->dir from #16/2 above looks logical to me.

Log in or register to post comments

Comment #22

cburschka

they

commented 18 January 2008 at 14:33

Status:

Needs review

» Needs work

Unless somebody else wants to, I could make the new patch.

Edit: Am I right in assuming that the previous patch of chameleon.theme should not be changed? It uses a different variable set than those provided in theme.inc, setting language to be a string rather than an object, which helps it in placing the variables directly into quotes.

Log in or register to post comments

Comment #23

cburschka

they

commented 18 January 2008 at 14:44

Status	File	Size
new	theme_language_direction-208768-23.patch	8.16 KB

This modifies patch #8 to replace $variables['direction'] and $direction with $variables['language']->dir and $language->dir. The documentation comment in page.tpl.php is also updated. Chameleon continues to use $language and $direction as strings.

Log in or register to post comments

Comment #24

cburschka

they

commented 18 January 2008 at 14:44

Status:

Needs work

» Needs review

Marking CNR.

Log in or register to post comments

Comment #25

hass commented 19 January 2008 at 11:33

Status:

Needs review

» Needs work

You change here doesn't look correct:

-  $language = isset($GLOBALS['language']) ? $GLOBALS['language']->language : NULL;
+  $language = $GLOBALS['language']->language;
+  $direction = $GLOBALS['language']->direction ? 'rtl' : 'ltr';

Log in or register to post comments

Comment #26

hass commented 19 January 2008 at 11:44

Status:

Needs work

» Needs review

Hmmm, no. The language object should exist every time and NULL makes no sense inside HTML.

Log in or register to post comments

Comment #27

hass commented 19 January 2008 at 12:33

Status:

Needs review

» Reviewed & tested by the community

Tested all core themes + my own in LTR and RTL. Looks good.

Log in or register to post comments

Comment #28

hass commented 20 January 2008 at 02:23

Status:

Reviewed & tested by the community

» Needs work

I get the following notice on page update.php?op=results if i'm running an update.

notice: Undefined property: stdClass::$dir in C:\Inetpub\wwwroot\drupal6\themes\garland\maintenance-page.tpl.php on line 16.

Log in or register to post comments

Comment #29

cburschka

they

commented 21 January 2008 at 13:00

It /is/ bad code; not E_NOTICE-safe. I copied the previous patch without checking anything aside from the $language->dir change, sorry. I'll bring in the corrected patch in a moment.

Log in or register to post comments

Comment #30

cburschka

they

commented 21 January 2008 at 13:24

Wait, that particular error indicates that $language is an object, but $language->dir isn't set. I'm not sure how that works... isn't the variable assignment in theme.inc always run?

I stand ready to replace the theme.inc code with


  $variables['language']          = is_object($GLOBALS['language']) ? $GLOBALS['language'] : new stdClass();
  $variables['language']->dir     = !empty($GLOBALS['language']->direction) ? 'rtl' : 'ltr';

which would prevent warnings (or even fatals) if $GLOBALS['language'] failed to be an object. But the error specified seems to be caused by something else... hass, could you var_dump or print_r the variables in maintenance-page.tpl.php and in theme.inc before and after the language variable is supposed to be set? I'd do it, but it'd have to wait until tonight...

Log in or register to post comments

Comment #31

cburschka

they

commented 22 January 2008 at 17:02

$language->dir isn't set on maintenance-page.tpl.php, but is set in page.tpl.php

$language in update.php?op=info when var_dumped in maintenance-page.tpl.php
object(stdClass)#2 (11) {
  ["language"]=>
  string(2) "de"
  ["name"]=>
  string(6) "German"
  ["native"]=>
  string(7) "Deutsch"
  ["direction"]=>
  string(1) "0"
  ["enabled"]=>
  int(1)
  ["plurals"]=>
  string(1) "2"
  ["formula"]=>
  string(7) "($n!=1)"
  ["domain"]=>
  string(0) ""
  ["prefix"]=>
  string(2) "de"
  ["weight"]=>
  string(1) "0"
  ["javascript"]=>
  string(0) ""
}

$language in / when var_dumped in page.tpl.php
object(stdClass)#2 (12) {
  ["language"]=>
  string(2) "de"
  ["name"]=>
  string(6) "German"
  ["native"]=>
  string(7) "Deutsch"
  ["direction"]=>
  string(1) "0"
  ["enabled"]=>
  int(1)
  ["plurals"]=>
  string(1) "2"
  ["formula"]=>
  string(7) "($n!=1)"
  ["domain"]=>
  string(0) ""
  ["prefix"]=>
  string(2) "de"
  ["weight"]=>
  string(1) "0"
  ["javascript"]=>
  string(0) ""
  ["dir"]=>
  string(3) "ltr"
}

I'll try to figure out what is going on here.

Log in or register to post comments

Comment #32

cburschka

they

commented 22 January 2008 at 17:06

maintenance-page.tpl.php does not use includes/theme.inc to populate its variables. I'm figuring out where it does get populated so I'll know what file to update. As good as fixed.

Log in or register to post comments

Comment #33

cburschka

they

commented 22 January 2008 at 17:09

grep 'variables\['"'"'language'"'"'\]' -R *

includes/theme.inc: $variables['language'] = is_object($GLOBALS['language']) ? $GLOBALS['language'] : new stdClass();
includes/theme.inc: $variables['language']->dir = !empty($GLOBALS['language']->direction) ? 'rtl' : 'ltr';
includes/theme.inc: var_dump("CALLED", $variables['language']);
includes/theme.maintenance.inc: $variables['language'] = $GLOBALS['language'];
modules/book/book.module: $variables['language'] = $language;

I feel a bit silly for missing it now. But hey. Patch follows shortly.

Log in or register to post comments

Comment #34

cburschka

they

commented 22 January 2008 at 17:30

Assigned:	Unassigned	» cburschka
Status:	Needs work	» Needs review

Status	File	Size
new	theme_language_direction-208768-34.patch	8.96 KB

Finally! Notice is gone on my update pages now.

Log in or register to post comments

Comment #35

cburschka

they

commented 22 January 2008 at 17:42

Status	File	Size
new	theme_language_direction-208768-35.patch	8.96 KB

2 commits on includes/theme.inc since last checkout (#210479 and #212409). Patch applies with 8 lines offset; not critical but it's no bother to re-roll.

Log in or register to post comments

Comment #36

dvessel commented 22 January 2008 at 17:48

Status:

Needs review

» Needs work

Minor coding style nitpick. Inside chameleon.theme:

  $language = isset($GLOBALS['language']) ? $GLOBALS['language']->language : NULL;
+  $direction = !empty($GLOBALS['language']->direction) ? 'rtl' : 'ltr';

How about structuring it like the rest by removing the isset() and !empty()?

Log in or register to post comments

Comment #37

cburschka

they

commented 22 January 2008 at 17:55

Status:

Needs work

» Needs review

Status	File	Size
new	theme_language_direction-208768-37.patch	9 KB

You're right. The existing code already makes no sense - if the other themes don't need to isset()-wrap, then chameleon doesn't need to either. Both checks removed here.

Log in or register to post comments

Comment #38

dvessel commented 22 January 2008 at 18:35

Status:

Needs review

» Reviewed & tested by the community

Looks good.

It wouldn't hurt to have a followup patch after this gets in to remove the direction from style-rtl.css from all the themes.

Log in or register to post comments

Comment #39

gábor hojtsy

he/him

Hungarian

Hungary

commented 23 January 2008 at 09:52

Status:

Reviewed & tested by the community

» Needs work

The chameleon code seems to be broken. You did:

-  $language = isset($GLOBALS['language']) ? $GLOBALS['language']->language : NULL;
+  $language = $GLOBALS['language'];

And then used $language later as if it was a language code. Not good.

Log in or register to post comments

Comment #40

hass commented 23 January 2008 at 12:19

@dvessel:

remove the direction from style-rtl.css from all the themes

never heard about this possibility... are you sure? Any reference?

Log in or register to post comments

Comment #41

cburschka

they

commented 23 January 2008 at 13:49

Status:

Needs work

» Needs review

Status	File	Size
new	theme_language_direction-208768-41.patch	9.01 KB

@Gabor: Oh my, how silly. Fixed.

Log in or register to post comments

Comment #42

dvessel commented 23 January 2008 at 19:40

Status:

Needs review

» Reviewed & tested by the community

ugh, me bad review. It's ready now. :)

@hass, direction is done through the markup with the patch. I'd argue that it's not a style, it's more integral to the structure of the page. Might not make much practical difference but it makes more sense.

Log in or register to post comments

Comment #43

gábor hojtsy

he/him

Hungarian

Hungary

commented 24 January 2008 at 09:43

Status:

Reviewed & tested by the community

» Fixed

Thanks, committed.

Log in or register to post comments

Comment #44

Anonymous (not verified) commented 7 February 2008 at 09:51

Status:

Fixed

» Closed (fixed)

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

Log in or register to post comments

Language direction missing in <html>

Comments