All the core styles use the value of "inherit" to try and reset what is inside the base LTR style. I noticed this after looking over this issue and it looks like it's not used properly.

According to the W3C:

6.2.1 The 'inherit' value

Each property may also have a specified value of 'inherit', which means that, for a given element, the property takes the same computed value as the property for the element's parent. The inherited value, which is normally only used as a fallback value, can be strengthened by setting 'inherit' explicitly.

So, according to that this is what's happening.

This is inside default(-rtl).css.

LTR base:

th {
  text-align: left; /* LTR */
  padding-right: 1em; /* LTR */
  border-bottom: 3px solid #ccc;
}

RTL override:

th {
  text-align: right;
  padding-right: inherit;
  padding-left: 1em;
}

Example markup:

<table>
  <tr>
    <th>Table Header</th>
  </tr>
  ...
</table>

Inside a normal LTR layout, the text "Table Header" is right padded but the RTL is inheriting the calculated parent styles.

Lets say we have this inside the theme for whatever reason:

style.css:

.container {
  padding-right: 20em;
}

Markup:

<div class="container">
  <table>
    <tr>
      <th>Table Header</th>
    </tr>
    ...
  </table>
</div>

Now the RTL style for "th" will inherit the computed value of one of the parents. In this case the .container div and not reset what was set inside the LTR file as it seems like it was trying to do.

This is done everywhere in core.

http://www.w3.org/TR/REC-CSS2/cascade.html#value-def-inherit
http://dorward.me.uk/www/css/inheritance/

CommentFileSizeAuthor
#11 drupal-HEAD.rtl_.patch12.61 KBsun
#6 Picture 1_60.png5.81 KBdvessel

Comments

dvessel’s picture

dvessel commented

The table wasn't a good example since they're treated differently but the logic applies for everything else.

yhager’s picture

yhager commented

In the example above, what would be the value of th {padding-right} ? It would be inherited implicitly from the parent. When we switch to an RTL theme, we would want the same to happen for the 'padding-left' value, so we set 'padding-left:inherit'.

Moreover, if you have 

.container {padding-right:20em}

then the RTL style would set

.container {padding-left:20em;padding-right:inherit}

which gives the exact behavior we want!

dvessel’s picture

dvessel commented

And what do you think this does?

.container {padding-left:20em;padding-right:inherit}

Your not getting the issue here. Read.. calculated parent property.

yhager’s picture

yhager commented

No, I am probably not getting to the bottom of this, cause I still think it is right. padding-left of the .container will get the value of the parent anyway in the LTR version, so what's wrong with it being such in the RTL version for padding-right?

From a different angle - can you show a real problem with using this method in the core themes?

dvessel’s picture

dvessel commented

For everyone who's willing to see this for what it is.. copy this and load it in your browser.

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
  "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">

<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
<head>
  <meta http-equiv="Content-Type" content="text/html; charset=utf-8"/>
  <title>inherit test</title>
  
  <style type="text/css" media="all">
    .parent {
      margin-left: 10em;
      border-left: 5px black solid;
      width: 20em;
      background: red;
    }
    .content {
      margin-left: 1em;
      border-left: 2px purple dotted;
      width: 20em;
      background: green;
    }
    /* Make pretend this is inside a rtl style. */
    /* From where does it inherit? */
    .content {
      margin-left: inherit;
      background: inherit;
      color: white;
    }
  </style>
  
</head>

<body>

<div class="parent">
  This is inside "parent".
  <div class="content">
    this is inside "content".
  </div>
</div>

</body>
</html>

There's clearly defined way of using the value. It's like saying it works in IE so what's the problem.? The problem is that if it works, it's by dumb chance.

dvessel’s picture

dvessel commented
StatusFileSize
new Picture 1_60.png5.81 KB

Here's a screen shot, go figure..

dvessel’s picture

dvessel commented
Priority: Critical » Normal

I'll just add that although this doesn't seem like a problem, it's definitely something to watch out for.

The reason i brought this up is because it looked like the inherit value was used to reset it back to it's original condition which it does not.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Additionally, as already mentioned in IRC, inherit is CSS 2 only and not supported by older browsers like IE6. Core themes should not use it at all.

elv’s picture

elv commented

dvessel is right I think.
Paddings are not inherited by default, so I can see no reason why they should be inherited in RTL if they're not in LTR.

Let's suppose the TR has a padding of 4em, for some reason (yeah paddings are not supposed to work on TRs but it's just for testing purpose).
By default the THs inside it have paddings of 0, as paddings are not inherited.

So with the styles describes in the first message we get:
In LTR, a TH with a right padding of 1em.
In RTL, a TH with a right padding of 4em, and a left padding of 1em.

Test file, very much like dvessel's one, but demonstrating paddings:

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
  "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">

<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
<head>
  <meta http-equiv="Content-Type" content="text/html; charset=utf-8"/>
  <title>inherit test</title>
 
  <style type="text/css" media="all">
    tr {
      padding: 4em;
      background: red;
    }
    th {
      text-align: left; /* LTR */
      padding-right: 1em; /* LTR */
      border-bottom: 3px solid #ccc;;
      background: green;
    }
    /* Make pretend this is inside a rtl style. */
    /* From where does it inherit? */
    th.rtl {
      text-align: right;
      padding-right: inherit;
      padding-left: 1em;
      background: orange;
    }
  </style>
 
</head>

<body>

<table>
  <tr>
    <th>LTR cell</th>
    <th class="rtl">RTL cell</th>
  </tr>
</table>

</body>
</html>
sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Just a nitpicking question: Why don't we set

<body class="rtl">

to apply RTL rules and override the styles for LTR? Thus:

- body {
+ body.rtl {
    direction: rtl;
  }
- ol li, ul li {
+ .rtl ol li, .rtl ul li {
    margin: 0.4em .5em 0.4em 0;
  }

That would be much cleaner and would not depend on any stylesheet file inclusion order.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Active » Needs review
StatusFileSize
new drupal-HEAD.rtl_.patch12.61 KB

Attached patch implements what I suggested in my last follow up. Note: My lang is ltr, so this patch basically implements a proper way to override ltr styles but the rtl styles are perhaps incomplete/wrong. However, that's another issue. We should focus on a good way to override ltr styles in this issue.

The patch additionally cleans out some rtl styles because they are needless/duplicated (both style.css/fix-ie.css and style-rtl.css/fix-ie-rtl.css are loaded).

dvessel’s picture

dvessel commented
Status: Needs review » Active

Sun, I forgot to mention IE6 support, thanks for bringing that up. It's spotty at best.

Setting a class wouldn't do much unless we set all the default LTR styles to use those classes and that ultimately depends on the theme.

The only other option I can think of is to explicitly set a value instead of using inherit but then the base style should be set too since not all browsers default to the same value.

elv’s picture

elv commented

There are also cases when the parent element doesn't have the proper values to be inherited. See my comment on the other topic.
(I wrote it there because it referred to an example given in another comment)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Agreed - usage of 'inherit' or a cascading class does not work out. RTL styles need to properly override LTR styles.

Based on the RTL stylesheets in HEAD, I would suggest to re-create them from scratch. The current stylesheets break the theme in many places. They also contain stuff that should not be there (f.e. clear-block styles).

However, I have no use for RTL currently. So this has to be done by someone else talented in theming.

yhager’s picture

yhager commented

@dvessel: What I didn't realize was that some properties are NOT inherited by default (like padding). Of course it's trivial, I just didn't make the connection until elv wrote it specifically. Also, I was not aware that IE6 do not support 'inherit' - I guess this is a real blocker.

dvessel’s picture

dvessel commented

yhager, glad we're seeing the same thing. Thanks elv for clearing that up.

So what do you guys see as a best alternative? Explicitly setting a value for the rtl styles sound best to me. But it should be done by keeping in mind what the style applies to. It looks like most of the elements can be zeroed out when it applies to div's. It'll get trickier with list items or anything that has default padding or margins applied by the browser.

What I think would be ideal is to set default margins and padding for various tags inside "default.css" so it's not dependent on the browsers default which can vary. That may affect too much and we're way too late into beta.

Another thing I think that'll help in the future is to set a tag inside the selector so we have a good idea on what it's being applied to. The good news is that it looks like this is being done already. Some guidelines on this may keep it consistent.

example inside a LTR style:

.parent li.theclass {
  margin-left: 1em; /* LTR */
}

not:

.parent .theclass {
  margin-left: 1em; /* LTR */
}

Not always possible since some classes are used for multiple tags but you get the idea.

All the rtl styles does not have to be redone. We just need to set explicit values.

elv’s picture

elv commented

Yes, setting explicit values is the best option IMHO. I guess there's no perfect solution.
Explicit values mean you have to change both the standard and the RTL stylesheet, if you modify the theme. But I think it's the themer's responsibility to deal with this.

Anyway a good practice is be to always add a "LTR" comment near every overridden style.

LTR:

.gugu li {border-left: 3px solid red} /* LTR */

RTL:

.gugu li {border-left: none; border-right: 3px solid red}
yhager’s picture

yhager commented
Status: Active » Closed (duplicate)

See http://drupal.org/node/195543