Remove language_css_alter() (RTL stylesheets) in favor of HTML 'dir' attribute

I one million perent agree with this. This is a terrible Drupal anti-pattern that must be stopped. In fact, the best practice in the wider world of the web is to do what's proposed (using the dir attribute selector). There is support back to IE7 for this and all major modern browsers. We already have everything in place to do this, we just need to make it the standard and update our CSS.

+1 from me as well. The maintenance and testing advantages outweigh a few extra lines of CSS in the LTR stylesheet. Theoretically at least, primarily RTL themes could assume RTL and use [dir="ltr"] for the overrides.

@Snugug and discussed this a while back in the context of bringing in Assetic, and came to strong agreement that it should be chucked. big +1

A separate RTL stylesheet is another HTTP request too. The proposed method of inlining RTL is the way to go.

Re @rupl and others: the RTL sheet should not be a separate HTTP request. You are using CSS aggregation on your site, aren't you? I mean extra HTTP request maybe in development, but not on your live site?

@all: one of the selling points of the RTL sheet as a separate file is that those sites that will only ever appear in LTR will not need to pass on extra CSS selectors and potentially substantial more styling to the client that will never be needed on the site. This can be avoided with external CSS build tools that are not Drupalisms. I assume contrib themes will not rely on those either in the Drupal 8 cycle but I may be mistaken.

Either way, I'm fine getting rid of this Drupalism but more wider feedback would be great. Especially as this related to front end performance. As said I'm not at all concerned about HTTP requests, because you *do* have CSS aggregation turned on on your site. I'm concerned about the extra parsing overhead for unused selectors on LTR sites. We usually optimise Drupal for LTR/English first and try to avoid overhead on LTR/English sites if possible. I've been called out before on several occasions when introducing multilingual features/capabilities that taxed the original LTR/English base use case.

Yep.
What about adding a 'directionality' argument to drupal_add_css so if anyone wants they can achieve the same conditional loading without the overhead?

stupid tags... sorry...

@tsi: I don't think making people work more to achieve the same in Drupal 8 can be solved as an improvement :D Also multiple conflicting ways to do the same does not make a consistent contrib space.

Well, I do see a case where someone would like to add a separate stylesheet based on directionality (or language), it can already be done today but will require *a lot* more work, I'm suggesting that simplifying that (work less, not more) while removing language_css_alter() is an improvement while simply removing language_css_alter() might be seen as a regression in these cases.

Example use case: I have two fontello font sets that should be included based on directionality/language.

I do agree with @Gábor about the performance issue - on production, with aggregation enabled you're getting less lines of CSS today, whether it's a handful of bytes or more depends on the implementation.

A point to consider, if we do decide that those extra lines are negligible, is that inline RTL rules (right next to the LTR ones) will probably give us much more solid support for RTL css in core (it will simply be harder to neglect) and better maintainability. On the other hand, that will require a major change to almost all of core CSS - can this still be done for 8.x ?

I think we should be upfront about this and admit that this is a performance regression, and not an insignificant one. Before we start making assumptions about "just a couple of lines" can we verify the impact here?

Bartik alone has 320 lines of RTL CSS, compressed this weighs about 4KB. I haven't spent any time checking all the core or even contrib module RTL CSS.

I think we should be upfront about this and admit that this is a performance regression, and not an insignificant one. Before we start making assumptions about "just a couple of lines" can we verify the impact here?

Bartik alone has 320 lines of RTL CSS, compressed this weighs about 4KB. I haven't spent any time checking all the core or even contrib module RTL CSS.

yup the Drupalism's like this must die die die die die (die)
will argue for having those few X amount of lines (even if its a ton) is not gonna be an issue compared to the maintainability for the frontend.

To get back to the size issue, let's break down some actual numbers from properly optimized CSS (not CSS by committee). In creating a responsive grid system with five different grids ranging from 4 to 16 columns wide, the CSS for the grid system in LTR 7715 bytes. The total sum of all of the RTL components of the grid (mind you, this is a grid that can describe each and every single potential layout on the site) is 828 bytes. When talking about performance vs regression, while it may be technically true that if I were to serve the combined file to LTR it would be a regression of performance for LTR (by 828 bytes), in most frontender's estimation, the worse thing is what we currently have; different CSS packages per language direction. I'm going to go out on a limb and say that an extra 828 bytes for everyone and a solid CSS cache for all directions is better than two different CSS packages, one for each direction, of equal size plus a little more for the RTL direction.

Even if we were to take this to what Bartik currently has, what we're really talking about is an extra 4kb for everyone or an extra 44kb for RTL users. Now when talking performance and regression, which would you take?

Sam, I don't follow your argument, why would you assume an *extra* 44kb for RTL users?
Following your example the situation would be 40kb for LTR and 44kb for RTL pages - every page delivers what is needed for it to display properly, as it should.

Saying that, I do support the maintainability gain - as an RTL dev that had a lot of hard time bringing RTL into core, it is very hard to maintain and get patches committed when styles live in a separate file, mainly due to lack of public interest.
It feels weird to bring all the RTL css into the default stylesheets just because we couldn't maintain them, but we couldn't, so maybe we should.

There's some flawed logic here, only the inverse styles from the LTR stylesheets are redundant for RTL to users. They still need every other line. Assuming the CSS matches line for line, it would be 4kb waste for LTR and a 4kb waste for RTL.

Just for reference - probably one of the worst examples for RTL maintainability nightmare, would never happen if it would've been inline with the LTR styles; it took around 16 months to get Seven to support RTL, months after D7 was released we still had to deal with broken admin theme on RTL sites #766458: Seven theme lacks rtl styling

The 44k additional comes from multilingual sites switching languages needing to download an entire new CSS package for the RTL vs the LTR. It's only 4k if you're exclusively making an RTL site, but I was under the impressions we're talking about multilingual sites where language swapping is a possibility (and probability).

I'm in favor of this change as well. But let's see some reviews now! :-)

fubhy's patch/audit of RTL styles is enough to convince me that the current way of working is not effective. Mistakes using the current process have slipped through the review into core.

This new process will make it easier to implement, test, and review LTR/RTL styling. In the light of the inconsistencies fubhy has found, I think this is something we need, rather than something we want.

Performance should always be a consideration. looking forward towards Drupal 9, we'll start using modern CSS3 layout techniques that will remove or reduce the need for separate RTL styling.

#24: 2015789-24.patch queued for re-testing.

The patch got knocked out of whack by commit b3b93ac4246304d18984799a23add7cd7c1d5cdf

Patch doesn't apply well with latest 8.x branch. Conflicts with the following files:

core/modules/system/css/system.admin.css
core/modules/system/css/system.admin-rtl.css

Maybe it needs re-roll.

Plus openid module is now gone from core.

Rerolled #36.

Removed OpenID module references.

The changes in system.admin.css committed in #1861702: Add a responsive grid to the Appearance page are not properly in #40 patch

This re-does system.admin.css and also adjusts tour.css since #1363112: Simplify names of "element-x" helper classes was committed.

added new change to vertical-tabs.css in #2018913: Vertical tab styling for tabs should not be applied when JavaScript is disabled and no tabs are present
tagging with awesome new tag!

#44: 2015789-44.patch queued for re-testing.

tagging

Re-rolled with the latest changes.

@echoz: How the "Avoid commit conflicts" tag works exactly?

How the "Avoid commit conflicts" tag works exactly?

Heh, I only assumed this was helpful for the core committers when other rtbc patches exist that touch the same files, they can make the call to commit those first, rather than cause them to need re-rolls.

---
I didn't review all, but only caught the following typo while comparing the 2 latest patches.

views_ui.admin-rtl.css had ".views-display-columns > *" and when copied to views_ui.admin.css, it is mispelled as "[dir=rtl] .views-display-colums > *" (columns is missing the "n").

Were there other places that you hand typed rather than copy pasted?
Any tips how to review this beast?

I'm reviewing this in detail now. But I see that you forgot to remove the core/themes/bartik/css/style-rtl.css file.

@echoz: I only fixed conflicts by hand in the following files:

core/modules/views_ui/css/views_ui.admin.css
core/modules/views_ui/css/views_ui.admin.theme.css
core/themes/seven/style.css

I know it was better to copy and paste but I thought there were less changes :(

Will try to re-roll this beast this afternoon but feel free if you want to do that.

I checked every single selector in this patch.

And this patch should be RTBC after these changes:

1. The latest patch mistakenly includes changes to these files, but shouldn't:
   • core/modules/user/lib/Drupal/user/Tests/UserPermissionsTest.php
   • core/modules/user/user.module
2. Add a space before the { in this ruleset: [dir=rtl] form .field-multiple-table .field-multiple-drag .tabledrag-handle{
3. In this ruleset
   
   

   [dir=rtl] .toolbar .toggle-orientation [value="vertical"]:before {
     background-image: url('../images/icon-toggle-vertical-rtl.png'); /* LTR */
   }

   remove the /* LTR */

4. Change the selector
   
   .js[dir=rtl] .toolbar .tour-toolbar-tab.tab to:
   .js[dir=rtl] .toolbar .bar .tour-toolbar-tab.tab
5. Change the selector
   
   [dir=rtl] .views-display-colums > * to:
   [dir=rtl] .views-display-columns > *
6. Change the selector
   
   

   [dir=rtl] .container-inline > * + *,
   .container-inline .details-wrapper > * + *

   to:

   [dir=rtl] .container-inline > * + *,
   [dir=rtl] .container-inline .details-wrapper > * + *

7. Remove the ruleset
   
   

   [dir=rtl] #user-profile-form input#edit-submit {
     margin-left: 0;
   }

8. Move the ruleset
   
   

   [dir=rtl] .password-suggestions ul li {
     margin-right: 1.2em;
     margin-left: 0;
   }

   to just below the .password-suggestions ul li ruleset.

9. In seven's stye.css, add a space before the { in this selector [dir=rtl] ul.primary li.active a{
10. Change the selector
    
    [dir=rtl] html.js .vertical-tabs-panes to:
    html.js[dir=rtl] .vertical-tabs-panes
11. Remove the core/themes/bartik/css/style-rtl.css file

Working on it ;)

I can't wait for this commit! Ping me if there's anything I can do to help.

Just commited all the changes. Hope it's now RTBC ;)

Feel free to blame on me :P

*All* of JohnAlbin's changes listed in #52 have been implemented, nice work guys!
I jumped the gun and tested, patch applies cleanly, RTL language works, when this goes green it's RTBC for me.

Note: we need RTL admin testing, found lots of padding corrections needed.

RTBC!
BTW, @JohnAlbin, this resolves #1997630: RTL stylesheets not recognized for vertical tabs

At least this RTL override is missing in color.admin.css:

-#palette .hook {
-  float: right;
-}

Also, would it be an option to explicitely use [dir=ltr], so we wouldn't override the ltr rules for rtl, but would have either the ltr selector or the rtl one match?
So instead of:

.dblog-filter-form .form-item-type,
 .dblog-filter-form .form-item-severity {
   display: inline-block;
   margin: .1em .9em .1em .1em; /* LTR */
   max-width: 30%;
 }
[dir=rtl] .dblog-filter-form .form-item-type,
 [dir=rtl] .dblog-filter-form .form-item-severity {
  margin: .1em .1em .1em .9em;
}

it would look like this:

.dblog-filter-form .form-item-type,
 .dblog-filter-form .form-item-severity {
   display: inline-block;
   max-width: 30%;
 }
[dir=ltr] .dblog-filter-form .form-item-type,
 [dir=ltr] .dblog-filter-form .form-item-severity {
   margin: .1em .9em .1em .1em;
 }
[dir=rtl] .dblog-filter-form .form-item-type,
 [dir=rtl] .dblog-filter-form .form-item-severity {
  margin: .1em .1em .1em .9em;
}

This would make more obvious, which rules need (and have) an RTL counterpart and which ones are generic.
Don't know how it would work out performance-wise, though.

Sure, in fact there are some CSS clean up issues here #1921610: [Meta] Architect our CSS

I'm moving them to the Drupal 8 Mobile Initiative sandbox as branch per issue: https://drupal.org/sandbox/johnalbin/1488942

Don't hesitate to comment that changes in the corresponding issue ;)

Thanks for the feedback.

#59 is correct that

#palette .hook {
  float: right;
}

is in and removed from color.admin-rtl.css but missing in color.admin.css

Added

[dir=rtl] #palette .hook {
  float: right;
}

to color.admin.css

Patch applies well. Go for RTBC!!

I double checked the latest patch. It has all the changes I requested plus the [dir=rtl] #palette .hook ruleset. This is RTBC from me as well.

Good news: #1337554: Develop and use separate branding for the installer just landed.

Bad news: we need a re-roll.

Working on it :'(

#68 missed 3 .task-list RTL rulesets from seven/style.css probably because the style-rtl.css hadn't dropped the ol qualified selector that had been dropped in style.css in the installer commit.

Patch applies well. Go for RTBC!!

Great work...

Committed bfba7b8 and pushed to 8.x. Thanks!

Woo-hoo! Great work, everyone!

Please include the quotation marks when you write this attribute selector. We should be doing this as a standard. So.

Non-standard

[dir=rtl]

Standard

[dir="rtl"]

Here's a good article explaining why we want to make this our standard: http://mathiasbynens.be/notes/unquoted-attribute-values

Although it is valid to leave out the quotes for values of an attribute that are alphanumeric, it can lead to problems for non-alphanumeric values and I'd rather just see everyone be in the habit of using quotations so it's not something we need to consider.

Yay! +1 to #73

Okay, what do you suggest to fix this?

Maybe a new patch that replaces all [dir=rtl] sentences by [dir="rtl"]?

I think a quick followup issue would be good.

Created followup #2030925: quote rtl attribute selector

Change notice: https://drupal.org/node/2032405.

Removing "Needs change notification" tag.
Docs had been updated at CSS formatting guidelines

https://drupal.org/node/1887862 is not clear that this is ONLY a D8 rule. D7 beginners will be confused.

#80, thanks @hass, docs now edited branching RTL for Drupal versions.

The RTL orientation of the sidebar somehow got lost between this issue and #1337554: Develop and use separate branding for the installer.

See: #2033137: New installer sidebar not properly RTL.

Added extra D6/D7 section to docs.

Is the comment on the change notice https://drupal.org/node/2032405#comment-7637701 correct?

I thought the same thing, that /* LTR */ should be on things that need to have a RTL counterpart.
If the comment is right, we should update the actual change notice.

#84, yep and has been corrected, good catch.

Another follow-up filed: #2085673: Remove all remaining "-rtl.css" references.