Problem/Motivation

Currently, language_css_alter() checks every CSS file that is added to the page for it's -rtl.css counterpart and, if it exists, adds it as well. Due to CSS weight (the -rtl.css stylesheets are added after the original styles) the RTL rules override the LTR rules.

This is a total anti-pattern and pretty much a Drupalism. It requires you to maintain another file and keep it in sync with the changes in your main stylesheet. It involves PHP logic to dynamically detect and inject the right .css files on run-time including quite a few file_exists() checks for every page RTL page that is loaded.

It also makes it much harder to leverage Sass or any other preprocessor to automatically generate the RTL output in your stylesheets which would otherwise be a big win for maintainability.

Proposed resolution

Let's leverage the 'dir' attribute in our stylesheets. Basically, this would look like this:

// CSS
.foo {
  float: left;
}

[dir="rtl"] .foo {
  float: right;
}

// Plain Sass
.foo {
  float: left;

  [dir="rtl] &{
    float: right;
  }
}

// Or even with cool mixins that do all the magic for you
.foo {
  // Mixin that inverts the float for RTL languages.
  @include float(left);
}

This increases the maintainability of our stylesheets and makes them somewhat self-contained. It takes away the burden of synchronizing changes across multiple files when changing something as simple as a float.

Considerations

This slightly increases the size of the CSS for LTR pages aswell as both, the RTL and the LTR styles are served for everyone. However, it normally comes down to just a few lines of extra CSS that are even minified and compressed on production environments and therefore absolutely negligible.

Files: 
CommentFileSizeAuthor
#69 2015789-69.patch122.14 KBechoz
PASSED: [[SimpleTest]]: [MySQL] 57,945 pass(es).
[ View ]
#68 2015789-66.patch121.54 KBLewisNyman
PASSED: [[SimpleTest]]: [MySQL] 56,741 pass(es).
[ View ]
#68 interdiff.txt2.71 KBLewisNyman
#63 2015789-63.patch119.42 KBechoz
PASSED: [[SimpleTest]]: [MySQL] 58,124 pass(es).
[ View ]
#56 2015789-55.patch119.32 KBrteijeiro
PASSED: [[SimpleTest]]: [MySQL] 56,784 pass(es).
[ View ]
#48 2015789-48.patch116.54 KBrteijeiro
PASSED: [[SimpleTest]]: [MySQL] 58,327 pass(es).
[ View ]
#44 2015789-44.patch118.46 KBechoz
PASSED: [[SimpleTest]]: [MySQL] 57,763 pass(es).
[ View ]
#42 2015789-42.patch118.34 KBechoz
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#40 2015789-40.patch117.84 KBrteijeiro
PASSED: [[SimpleTest]]: [MySQL] 58,243 pass(es).
[ View ]
#39 2015789-39.patch119.5 KBpenyaskito
PASSED: [[SimpleTest]]: [MySQL] 56,130 pass(es).
[ View ]
#36 2015789-36.patch118.75 KBLewisNyman
PASSED: [[SimpleTest]]: [MySQL] 57,192 pass(es).
[ View ]
#24 2015789-24.patch124.38 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2015789-24.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#20 2015789-20.patch123.06 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 55,597 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#19 2015789-19.patch123.52 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 55,463 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#17 2015789-17.patch123.9 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/language/lib/Drupal/language/Tests/LanguageUrlRewritingTest.php.
[ View ]
#16 2015789-16.patch123.37 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/language/lib/Drupal/language/Tests/LanguageUrlRewritingTest.php.
[ View ]

Comments

Snugug’s picture

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.

fubhy’s picture

Another bonus: Testing RTL styles without page reloads/constantly switching languages by simply changing the dir attribute in your favorite debugging tool.

ry5n’s picture

+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.

sdboyer’s picture

@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

rupl’s picture

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

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

Issue tags:+D8MI, +language-base
tsi’s picture

Issue tags:-D8MI, -language-base

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?

tsi’s picture

Issue tags:+D8MI, +language-base

stupid tags... sorry...

Gábor Hojtsy’s picture

@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.

tsi’s picture

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.

fubhy’s picture

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.

Okay, but we are only talking about a handful of bytes here. All it would do, basically, is inverting floats, margins, paddings, border radius, etc. which is not that much. Also, due to things like Sass, with proper @mixins and @extends the footprint will be even smaller as many things can be aggregated into shared rules during preprocessing anyways. So yeah, we should not have to argue about a few bytes. Also, another downside of RTL stylesheets is that they require an additional CSS aggregation derivative which means that, when switching the language you have to re-download the entire package (granted, that should not happen very often but still, RTL stylesheets double the amount of hosted CSS aggregates).

tsi’s picture

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 ?

LewisNyman’s picture

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.

LewisNyman’s picture

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.

fubhy’s picture

Status:Active» Needs review
StatusFileSize
new123.37 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/language/lib/Drupal/language/Tests/LanguageUrlRewritingTest.php.
[ View ]

Holy moly there is a lot of crap in the stylesheets. I think this is going to serve us well in uncovering A LOT of RTL/LTR bugs in core. I found at least 20 spots where I could fix small bugs immediately and many more where the RTL styles are either completely missing or there are RTL styles for things that don't even exist anymore (sometimes even the selectors are no longer there).

Anyways... Here is a first patch.

fubhy’s picture

StatusFileSize
new123.9 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/language/lib/Drupal/language/Tests/LanguageUrlRewritingTest.php.
[ View ]

Forgot a direct call to language_css_alter(). Urghs. Removing uglyness.

Status:Needs review» Needs work

The last submitted patch, 2015789-17.patch, failed testing.

fubhy’s picture

Status:Needs work» Needs review
StatusFileSize
new123.52 KB
FAILED: [[SimpleTest]]: [MySQL] 55,463 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Accidentely added a whitespace before <?php

fubhy’s picture

StatusFileSize
new123.06 KB
FAILED: [[SimpleTest]]: [MySQL] 55,597 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Duh... Removing a few merge relicts.

Head -> Table.

mortendk’s picture

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.

fubhy’s picture

Yes exactly. I can see this being a big win for maintainability of RTL styles as everything is self-contained and close together. So if you change a LTR style you directly see that there is a RTL style that has to be updated too.

Once this gets in I will open an issue to dig through all of core CSS to find missing RTL styles (I already saw quite a few while doing this patch... mostly paddings/margins as floats are more obvious apparently). This is really going to help with fixing Core CSS.

Status:Needs review» Needs work

The last submitted patch, 2015789-20.patch, failed testing.

fubhy’s picture

StatusFileSize
new124.38 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2015789-24.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Ah, there you are RTL CSS test! Bye bye test!

fubhy’s picture

Status:Needs work» Needs review
Snugug’s picture

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?

tsi’s picture

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.

LewisNyman’s picture

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.

tsi’s picture

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

Snugug’s picture

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).

JohnAlbin’s picture

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

fubhy’s picture

As mentioned previously, this is basically a straight cut&paste patch except for the Views CSS where I could not match up certain things due to inconsistencies between the LTR and the RTL styles (due to them being outdates/incomplete). So the most important part to take a look at is the Views UI. Everything else really is just cut&paste including some minor fixes.

However, we really need follow-ups to look at all the stylesheets once more to identify missing RTL styles as I fear there are quite a few. Floats seem to be covered mostly, but so many paddings/margins/borders/etc. are not touched in the RTL styles and not marked as LTR styles in the stylesheets. Furthermore in some places when LTR for example sets a margin-left the RTL style sets a margin-right but without unsetting the margin-left. So, yeah... That definitely requires some follow-ups.

Anyways, for now I'd be happy for some reviews!

LewisNyman’s picture

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.

LewisNyman’s picture

Issue tags:-D8MI, -language-base

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

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

The last submitted patch, 2015789-24.patch, failed testing.

LewisNyman’s picture

Status:Needs work» Needs review
StatusFileSize
new118.75 KB
PASSED: [[SimpleTest]]: [MySQL] 57,192 pass(es).
[ View ]

The patch got knocked out of whack by commit b3b93ac4246304d18984799a23add7cd7c1d5cdf

rteijeiro’s picture

Status:Needs review» Needs work

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.

echoz’s picture

Plus openid module is now gone from core.

penyaskito’s picture

Status:Needs work» Needs review
StatusFileSize
new119.5 KB
PASSED: [[SimpleTest]]: [MySQL] 56,130 pass(es).
[ View ]

Rerolled #36.

rteijeiro’s picture

StatusFileSize
new117.84 KB
PASSED: [[SimpleTest]]: [MySQL] 58,243 pass(es).
[ View ]

Removed OpenID module references.

echoz’s picture

Status:Needs review» Needs work

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

echoz’s picture

Status:Needs work» Needs review
StatusFileSize
new118.34 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 2015789-42.patch, failed testing.

echoz’s picture

Status:Needs work» Needs review
Issue tags:+Avoid commit conflicts
StatusFileSize
new118.46 KB
PASSED: [[SimpleTest]]: [MySQL] 57,763 pass(es).
[ View ]

Status:Needs review» Needs work
Issue tags:-D8MI, -language-base, -Avoid commit conflicts

The last submitted patch, 2015789-44.patch, failed testing.

echoz’s picture

Status:Needs work» Needs review
Issue tags:+D8MI, +language-base, +Avoid commit conflicts

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

echoz’s picture

Issue tags:+RTL

tagging

rteijeiro’s picture

StatusFileSize
new116.54 KB
PASSED: [[SimpleTest]]: [MySQL] 58,327 pass(es).
[ View ]

Re-rolled with the latest changes.

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

echoz’s picture

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?

JohnAlbin’s picture

Status:Needs review» Needs work
Issue tags:-RTL, -language-base

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

rteijeiro’s picture

@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.

JohnAlbin’s picture

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
rteijeiro’s picture

Assigned:Unassigned» rteijeiro

Working on it ;)

fubhy’s picture

Thanks for working on this, guys. Everyone++

LewisNyman’s picture

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

rteijeiro’s picture

Status:Needs work» Needs review
StatusFileSize
new119.32 KB
PASSED: [[SimpleTest]]: [MySQL] 56,784 pass(es).
[ View ]

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

Feel free to blame on me :P

echoz’s picture

*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.

echoz’s picture

Status:Needs review» Reviewed & tested by the community
Pancho’s picture

Status:Reviewed & tested by the community» Needs work

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.

fubhy’s picture

Assigned:rteijeiro» Unassigned
Status:Needs work» Reviewed & tested by the community

@Pancho: There are _many_ missing RTL styles. But this issue is not about fixing those. We are just moving what's already there. If we had to fix it all in here we would never get it done. Let's just get the straight conversion (from -rtl.css to [dir=rtl]) done and then clean up and fix the CSS somewhere else.

Setting back to RTBC (although I did not check the last patch).

rteijeiro’s picture

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.

echoz’s picture

Status:Reviewed & tested by the community» Needs work

#59 is correct that

#palette .hook {
  float: right;
}

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

echoz’s picture

Status:Needs work» Needs review
StatusFileSize
new119.42 KB
PASSED: [[SimpleTest]]: [MySQL] 58,124 pass(es).
[ View ]

Added

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

to color.admin.css

rteijeiro’s picture

Status:Needs review» Reviewed & tested by the community

Patch applies well. Go for RTBC!!

JohnAlbin’s picture

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.

LewisNyman’s picture

Status:Reviewed & tested by the community» Needs work

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

Bad news: we need a re-roll.

rteijeiro’s picture

Assigned:Unassigned» rteijeiro

Working on it :'(

LewisNyman’s picture

Assigned:rteijeiro» Unassigned
Status:Needs work» Needs review
StatusFileSize
new2.71 KB
new121.54 KB
PASSED: [[SimpleTest]]: [MySQL] 56,741 pass(es).
[ View ]
echoz’s picture

StatusFileSize
new122.14 KB
PASSED: [[SimpleTest]]: [MySQL] 57,945 pass(es).
[ View ]

#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.

rteijeiro’s picture

Status:Needs review» Reviewed & tested by the community

Patch applies well. Go for RTBC!!

alexpott’s picture

Title:Remove language_css_alter() (RTL stylesheets) in favor of HTML 'dir' attribute» Change notice: Remove language_css_alter() (RTL stylesheets) in favor of HTML 'dir' attribute
Priority:Normal» Critical
Status:Reviewed & tested by the community» Active
Issue tags:-Avoid commit conflicts+Needs change record

Great work...

Committed bfba7b8 and pushed to 8.x. Thanks!

JohnAlbin’s picture

Woo-hoo! Great work, everyone!

jessebeach’s picture

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.

ry5n’s picture

Yay! +1 to #73

rteijeiro’s picture

Okay, what do you suggest to fix this?

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

Gábor Hojtsy’s picture

I think a quick followup issue would be good.

echoz’s picture

jessebeach’s picture

Title:Change notice: Remove language_css_alter() (RTL stylesheets) in favor of HTML 'dir' attribute» Remove language_css_alter() (RTL stylesheets) in favor of HTML 'dir' attribute
Status:Active» Closed (fixed)
echoz’s picture

Issue tags:-Needs change record

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

hass’s picture

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

echoz’s picture

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

Gábor Hojtsy’s picture

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.

hass’s picture

Added extra D6/D7 section to docs.

YesCT’s picture

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.

echoz’s picture

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

Wim Leers’s picture