Add missing RTL rules to System CSS

1. +++ b/core/modules/system/css/system.admin.css
   @@ -132,7 +132,11 @@ small .admin-link:after {
    .system-modules td {
   -  padding-left: 0;
   +  padding-left: 0; /* LTR */
   +}
   +[dir="rtl"] .system-modules td {
   +  padding-left: 12px;
   +  padding-right: 0;
   

   Where is 12 defined for the default one? Generally for all td?

2. +++ b/core/modules/system/css/system.diff.css
   @@ -71,7 +82,11 @@ table.diff col.diff-content {
    table.diff th {
   -  padding-right: inherit;
   +  padding-right: inherit; /* LTR */
   +}
   +[dir="rtl"] table.diff th {
   +  padding-left: inherit;
   +  padding-right: 9px;
   

   Same question for 9px?

3. +++ b/core/modules/system/css/system.theme.css
   @@ -75,7 +75,12 @@ label.option {
   +  margin-right: 2.4em; ¶
   

   Extra whitespace at end of line.

4. +++ b/core/modules/system/css/system.theme.css
   @@ -235,7 +240,7 @@ summary {
    .collapse-processed > summary:before {
      background: url(../../../misc/menu-expanded.png) 0px 100% no-repeat; /* LTR */
      content: "";
   -  float: left;
   +  float: left; /* LTR */
   

   Already has an RTL rule?

5. +++ b/core/modules/system/css/system.theme.css
   @@ -333,6 +338,11 @@ th.checkbox {
   +  animation-direction: reverse;
   

   Is the LTR one already marked for this one?

Thanks, looks good with those fixes.

If the px is added in Seven, why isn't the RTL version also in Seven?

+++ b/core/modules/system/css/system.admin.css
@@ -129,13 +129,17 @@ small .admin-link:after {
 .system-modules td {
-  padding-left: 0;
+  padding-left: 0; /* LTR */
+}
+[dir="rtl"] .system-modules td {
+  padding-left: 12px;
+  padding-right: 0;
 }

+++ b/core/modules/system/css/system.diff.css
@@ -68,13 +79,17 @@ table.diff col.diff-marker {
 table.diff th {
-  padding-right: inherit;
+  padding-right: inherit; /* LTR */
+}
+[dir="rtl"] table.diff th {
+  padding-left: inherit;
+  padding-right: 12px;
 }

@catch: I think you are concerned for these two.

Looks like those 12px are defined in core/themes/seven/css/components/tables.css:

th {
  text-align: left; /* LTR */
  padding: 10px 12px;
}

...

td {
  padding: 10px 12px;
  text-align: left; /* LTR */
}

However the patch is against core/modules/system/css/system.admin.css.

So it is true that we are adding arbitrary 12pxs here if a theme other than seven is used.

Would it be a good idea though to add 12px into the system theme? Would that not be too specific for what these rules wanted to achieve?

Removed SprintWeekend2015Queue by mistake.

Can someone add some thoughts to the issue summary on how to best test this? Not sure what to do.

@Kristen Pol Have a look at a related issue to see what is expected #2360069: Add missing RTL rules to Seven tabs.css. The basic gist is you switch language direction to Right-to-left (/admin/config/regional/language/edit/en) and do manual testing and take screenshots along the way.

Patch looks great!

All the missing RTL styling has been added. However, there is a rule missing from the automated report from system.theme.css:

form .field-multiple-table .field-multiple-drag .tabledrag-handle {
  padding-right: .5em; /*LTR*/
}
[dir="rtl"] form .field-multiple-table .field-multiple-drag .tabledrag-handle {
  padding-left: .5em;
}

The RTL version should reset the padding-right to 0.

1. +++ b/core/modules/system/css/system.diff.css
   @@ -71,7 +82,10 @@ table.diff col.diff-content {
    table.diff th {
   -  padding-right: inherit;
   +  padding-right: inherit; /* LTR */
   +}
   +[dir="rtl"] table.diff th {
   +  padding-left: inherit;
    }
   

   Would make both paddings inherit, is that intended? Ie. LTR is not undone.

2. +++ b/core/modules/system/css/system.theme.css
   @@ -282,6 +287,11 @@ td.checkbox,
    th.checkbox {
      text-align: center;
    }
   +/* This is required to win over specifity of [dir="rtl"] td */
   +[dir="rtl"] td.checkbox,
   +[dir="rtl"] th.checkbox {
   +  text-align: center;
   +}
   

   Erm, why? The RTL td has different text align so this needs to be more specific?

3. +++ b/core/modules/system/css/system.theme.css
   @@ -385,7 +400,11 @@ ul.menu a.active {
   -  padding-left: 0;
   +  padding-left: 0; /* LTR */
   +}
   +[dir="rtl"] ul.inline,
   +[dir="rtl"] ul.links.inline {
   +  padding-right: 0;
   

   Should redo the left padding no?

4. +++ b/core/themes/seven/css/components/tables.css
   @@ -14,11 +14,14 @@ caption {
   +[dir="rtl"] table.diff th {
   +  padding-right: 12px;
   +}
   

   This does not come right after the LTR counterpart, why?

5. +++ b/core/themes/seven/css/components/tables.css
   @@ -42,12 +45,15 @@ th {
   +[dir="rtl"] .system-modules td {
   +  padding-left: 12px;
   +}
   

   This does not follow the LTR counterpart, why?

There are some clear actions here so tagging novice

Is someone planning to work on this one?

It is duplicated #2396483: Add missing RTL rules to Seven theme CSS

Looks like not being actively worked on, removing from sprint. :/

#14:
1. Reset padding right
2. Tested to remove this and didn't seem to make a difference, removed
3. Redid padding left
4 & 5. No LTR counterpart, removed

+++ b/core/themes/seven/css/components/tables.css
@@ -14,7 +14,7 @@ caption {
 th {
   text-align: left; /* LTR */
-  padding: 10px 12px;
+  padding: 10px 12px; /* LTR */

@@ -52,7 +52,7 @@ th {
 td {
-  padding: 10px 12px;
+  padding: 10px 12px; /* LTR */

These properties aren't LTR specific, also changes to Seven CSS should be made in #2396483: Add missing RTL rules to Seven theme CSS

Removed Seven specific changes.

Thanks @Aunion, changes look great!

However, this patch will need a reroll now that #2430961: Remove leftover CSS from theme_exposed_filters() has been committed:

error: patch failed: core/modules/system/css/system.admin.css:370
error: core/modules/system/css/system.admin.css: patch does not apply

Rerolled

Thanks @Aunion, just a few minor remarks and then we're good to go!

1. +++ b/core/modules/system/css/system.admin.css
   @@ -132,7 +132,10 @@ small .admin-link:after {
   -  padding-left: 0;
   +  padding-left: 0; /* LTR */
   +}
   +[dir="rtl"] .system-modules td {
   +  padding-right: 0;
   

   While technically this is correct, .system-modules td has no padding by default so these selectors can safely be removed.

2. +++ b/core/modules/system/css/system.diff.css
   @@ -14,7 +14,13 @@
   -  margin-right:5px;
   +  margin-right:5px; /* LTR */
   +}
   +
   +[dir="rtl"] .diff-inline-legend span,
   +[dir="rtl"] .diff-inline-legend label {
   

   RTL overrides don't require an additional linebreak

3. +++ b/core/modules/system/css/system.module.css
   @@ -128,9 +128,13 @@ a.tabledrag-handle:focus .handle {
   +[dir="rtl"] .touch a.tabledrag-handle .handle {
   +  background-position: right 40% top 19px;
   +}
   +
   

   This additional line-break is unnecessary and can be removed.

The remarks by @idebr of #31 are processed in this patch.

Modified according to #34

I agree with the changes proposed by b0unty.
There is no need to add the animation-direction: normal;/*LTR*/ line in the CSS.

Patch #35 by Dhorkiy looks fine to me.

I'm surprised that there are no screenshots on this issue?

Patch #35 applies properly. Though, I can't check specifically the effect of it's CSS because it is overriden by superior CSS rules (form.css overrides the change of .form-type-checkbox .description from the patch for instance).
Still, without this patch, admin pages looks good RTL. Here is an example where the above CSS rule applies : it is the same RTL with and without page because form.css already does the job.

LTR of Account settings page :

RTL of Account settings page :

@alexpott could you be more specific about expected screenshots and remaining work ?

@Dom: is this CSS used on a page without the form CSS? As for the screenshots, a few places where this fixes CSS would be nice to screenshot with markings on what is fixed (before/after).

Hi !
The patch applies quasi-exclusively on some form generated items (checkboxes, radio buttons, table, etc..) where form.css always overrides that.
There is however ".system-themes-admin-form" clear:left changed to clear:right. But this is also effectless because even without the patch, the concerned item renders properly (Administrative theme box under /admin/appearance)

@Dom. To test this patch correctly I would enable the Classy theme as the admin theme instead of Seven, otherwise the Seven CSS will override some of the changes here.

@LewisNyman: So true ! How could I forgot that !

Here is full review :

• system-modules td : the padding-left does not have a padding-right equivalent. But anyway it's not applied because normalize.css actually set padding:0px
• system-themes-admin-form : this is actually useless here but ok it has a rtl equivalent !

Otherone seems to apply properly; exampe here :

becomes after patch :

I could find this issue remaining though where parenthesis gone wrong :

Lets get the issue summary updated and embed the most recent screenshots in the summary.

Anyone know where .diff-inline-legend (declared in system.diff.css) is used in the Admin section? My global search isn't finding it and it's not in the Diff mod.

rerolling a patch #35

minor spaces between text and arrow in RTL as compared to LTR :(

@Manjit.Singh This appears to be an issue with head. The issue occurs with both head and with the patch applied in the Safari browser on a mac, but looks fine on Firefox and Chrome in both versions.

Do you mind if we postpone this until #2395853: Split system.module.css and system.theme.css files into SMACSS style components lands? It's a very difficult patch to reroll and we run the risk of losing these improvements easily.

The issue Lewis mentioned in #53 has been committed.

rerolled patch #48

manually test #57 patch. Arrows looks fine in Seven and stark !! Please check screenshots.

1. +++ b/core/modules/system/css/components/progress.theme.css
   @@ -49,6 +49,11 @@
   +[dir="rtl"] .progress__bar {
   +  margin-left: 0;
   +  margin-right: -1px;
   +  animation-direction: reverse;
   +}
   

   We've added RTL styling but we haven't placed LTR comments on the corresponding lines above.

   Also, if we're using animation-direction we should also be using the same browser prefixes as above. (webkit, moz)

2. +++ b/core/modules/system/css/system.admin.css
   @@ -132,7 +132,7 @@ small .admin-link:after {
    .system-modules td {
   -  padding-left: 0;
   +  padding-left: 0; /* LTR */
   

   This adds an LTR comment but I can't find the corresponding RTL styling?

@lewis I have made the changes as suggested in #59.
Also i have removed the LTR comment, As there is nor RTL style for the same. Please verify.

system-missing-rtl-2396473-62.patch

Trigger testbot

Also i have removed the LTR comment, As there is nor RTL style for the same. Please verify.

I think instead we should add the RTL styling. Sorry I wasn't very clear.

Also, it helps me review your changes if you also create interdiffs between patches. You can find out how to create them here: https://www.drupal.org/documentation/git/interdiff

Reviewed each change in the patch on a code level. Where there is no matching LTR/RTL change in the patch, there is already in the existing code. Looks good.

Nice catch.

Committed and pushed to 8.0.x. Thanks!