Part of #2359331: [Meta] Add missing RTL rules in core CSS files.. These missing RTL rules were found using an automated tool.

CommentFileSizeAuthor
#66 system-missing-rtl-2396473-66.patch6.09 KBherom
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,648 pass(es). View
#66 interdiff-2396473-62-66.txt964 bytesherom
#66 admin-modules-rtl-after.png5.2 KBherom
#66 admin-modules-rtl-before.png5.51 KBherom
#66 admin-module-ltr.png5.16 KBherom
#62 system-missing-rtl-2396473-62.patch6.16 KBprabhurajn654
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,454 pass(es). View
#58 rtl-arrows-after-stark.png266.94 KBManjit.Singh
#58 rtl-arrows-after-1.png72.45 KBManjit.Singh
#58 rtl-arrows-after.png173.81 KBManjit.Singh
#57 system-missing-rtl-2396473-57.patch6.06 KBpjbaert
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,462 pass(es). View
#49 rtl-arrows-seven-after-applying-patch.png85.4 KBManjit.Singh
#49 rtl-arrows-classy-after-applying-patch.png122.35 KBManjit.Singh
#48 system-missing-rtl-2396473-48.patch5.15 KBManjit.Singh
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,602 pass(es), 2,658 fail(s), and 1,060 exception(s). View
#43 LTR-wrong.png5.78 KBDom.
#43 contact-before.png6.04 KBDom.
#43 contact-after.png6.19 KBDom.
#41 ar-screen-appearence.png8.89 KBDom.
#39 RTL.png21.07 KBDom.
#39 LTR.png20.39 KBDom.
#35 interdiff.txt644 bytesDhorkiy
#35 system-missing-rtl-2396473-35.patch5.15 KBDhorkiy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,987 pass(es). View
#32 system-missing-rtl-2396473-32.patch5.36 KBpjbaert
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,580 pass(es). View
#32 interdiff-missing-rtl.txt1.29 KBpjbaert
#28 system-missing-rtl-2396473-28.patch5.42 KBAunion
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,428 pass(es). View
#26 system-missing-rtl-2396473-26.patch5.43 KBAunion
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,310 pass(es). View
#22 system-missing-rtl-2396473-22.patch5.98 KBAunion
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,274 pass(es). View
#13 system-missing-rtl-2396473-11.patch6.52 KBb0unty
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,136 pass(es). View
#6 system-missing-rtl-2396473-6.patch6.24 KBherom
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,561 pass(es). View
#6 interdiff-2396473-2-6.txt2 KBherom
#2 system-missing-rtl-2396473-2.patch6.83 KBherom
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,191 pass(es). View
#2 interdiff-2396473-0-2.txt1.24 KBherom
system-missing-rtl.patch5.06 KBherom
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,743 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Gábor Hojtsy’s picture

Status: Needs review » Needs work
  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?

herom’s picture

Status: Needs work » Needs review
FileSize
1.24 KB
6.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,191 pass(es). View

Thanks for the review.

1. yes. there is a td { padding: 10px 12px; } in themes/seven/css/components/tables.css:45.
2. there is a default for all th, but, strangely, it's 12px. fixed. It's in themes/seven/css/components/tables.css:17.
3. fixed.
4. yes, it has. added more context lines to the patch, so it's visible.
5. fixed. marked both the -webkit-animation and -moz-animation as /* LTR */

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, looks good with those fixes.

catch’s picture

Status: Reviewed & tested by the community » Needs review

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

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ 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?

herom’s picture

Status: Needs work » Needs review
FileSize
2 KB
6.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,561 pass(es). View

Well, I was trying to follow the convention of including the RTL version of a CSS rule immediately after the rule itself, but as you pointed out, it didn't work in this case. So, I fixed it and moved the 12px RTL fix into Seven code.

I also noted an RTL issue that was only visible visually (rule specifity issue), and added a fix for that too.

Gábor Hojtsy’s picture

Issue tags: +SprintWeekend2015Queue
develCuy’s picture

develCuy’s picture

Issue tags: +SprintWeekend2015Queue

Removed SprintWeekend2015Queue by mistake.

Kristen Pol’s picture

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

idebr’s picture

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

idebr’s picture

Status: Needs review » Needs work

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.

b0unty’s picture

Status: Needs work » Needs review
FileSize
6.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,136 pass(es). View

added RTL reset of padding 0 as asked in #12,

cant create interdiff properly as there were line changes that it was able to sort out..

patch -p1 < ../system-missing-rtl-2396473-6.patch 
patching file core/modules/system/css/system.admin.css
patching file core/modules/system/css/system.diff.css
patching file core/modules/system/css/system.maintenance.css
patching file core/modules/system/css/system.module.css
patching file core/modules/system/css/system.theme.css
Hunk #1 succeeded at 74 (offset -1 lines).
Hunk #2 succeeded at 239 (offset -1 lines).
Hunk #3 succeeded at 287 (offset -1 lines).
Hunk #4 succeeded at 333 (offset -1 lines).
Hunk #5 succeeded at 400 (offset -1 lines).
Gábor Hojtsy’s picture

Status: Needs review » Needs work
  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?

LewisNyman’s picture

Issue tags: +Novice

There are some clear actions here so tagging novice

Gábor Hojtsy’s picture

Is someone planning to work on this one?

b0unty’s picture

I can take a look at it tomorrow. Didnt have time busy week.

juankvillegas’s picture

b0unty’s picture

No it is not duplicate this is referencing System module and not seven theme.

Gábor Hojtsy’s picture

Issue tags: -sprint

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

Aunion’s picture

Assigned: Unassigned » Aunion
Aunion’s picture

FileSize
5.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,274 pass(es). View

#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

Aunion’s picture

Assigned: Aunion » Unassigned
Status: Needs work » Needs review
LewisNyman’s picture

Status: Needs review » Needs work
+++ 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

Aunion’s picture

Assigned: Unassigned » Aunion
Aunion’s picture

Assigned: Aunion » Unassigned
Status: Needs work » Needs review
FileSize
5.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,310 pass(es). View

Removed Seven specific changes.

idebr’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

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

Aunion’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
5.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,428 pass(es). View

Rerolled

Status: Needs review » Needs work

The last submitted patch, 28: system-missing-rtl-2396473-28.patch, failed testing.

Status: Needs work » Needs review
idebr’s picture

Status: Needs review » Needs work

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.

pjbaert’s picture

FileSize
1.29 KB
5.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,580 pass(es). View

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

pjbaert’s picture

Status: Needs work » Needs review
b0unty’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/css/system.theme.css
@@ -309,15 +314,20 @@ th.checkbox {
-  -webkit-animation: animate-stripes 3s linear infinite;
-  -moz-animation: animate-stripes 3s linear infinite;
+  -webkit-animation: animate-stripes 3s linear infinite; /* LTR */
+  -moz-animation: animate-stripes 3s linear infinite; /* LTR */

These aren't LTR specific as RTL uses em also but just with the animation-direction property, or am i just confused?
If we want to mark this shouldn't we use animation-direction: normal;/*LTR*/

Dhorkiy’s picture

Status: Needs work » Needs review
FileSize
5.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,987 pass(es). View
644 bytes

Modified according to #34

pjbaert’s picture

Status: Needs review » Reviewed & tested by the community

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs screenshots

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

Dom.’s picture

Assigned: Unassigned » Dom.
Dom.’s picture

FileSize
20.39 KB
21.07 KB

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 ?

Gábor Hojtsy’s picture

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

Dom.’s picture

Issue summary: View changes
FileSize
8.89 KB

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)

LewisNyman’s picture

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

Dom.’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
6.19 KB
6.04 KB
5.78 KB

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

fran seva’s picture

Issue tags: +drupaldevdays
Dom.’s picture

Assigned: Dom. » Unassigned
disasm’s picture

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

pbland’s picture

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.

Manjit.Singh’s picture

Status: Needs work » Needs review
FileSize
5.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,602 pass(es), 2,658 fail(s), and 1,060 exception(s). View

rerolling a patch #35

Manjit.Singh’s picture

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

rtl-arrows-seven-after-applying-patch

rtl-arrows-classy-after-applying-patch

pjbaert’s picture

Issue summary: View changes
Status: Needs review » Needs work
kerrymick’s picture

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

b0unty’s picture

Status: Needs work » Reviewed & tested by the community

confirmed what was said in #51, also the patch looks RTBC to me

LewisNyman’s picture

Status: Reviewed & tested by the community » Postponed
Related issues: +#2395853: Split system.module.css and system.theme.css files into SMACSS style components

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.

b0unty’s picture

Status: Postponed » Reviewed & tested by the community

seems like no one is working actively on the issue lewis posted, so unpostponing this, patch does still seem to apply so back to RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 48: system-missing-rtl-2396473-48.patch, failed testing.

davidhernandez’s picture

The issue Lewis mentioned in #53 has been committed.

pjbaert’s picture

Status: Needs work » Needs review
FileSize
6.06 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,462 pass(es). View

rerolled patch #48

Manjit.Singh’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice
FileSize
173.81 KB
72.45 KB
266.94 KB

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

LewisNyman’s picture

Status: Reviewed & tested by the community » Needs work
  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?

LewisNyman’s picture

Issue tags: +Novice
prabhurajn654’s picture

Assigned: Unassigned » prabhurajn654
prabhurajn654’s picture

FileSize
6.16 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,454 pass(es). View

@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

prabhurajn654’s picture

Assigned: prabhurajn654 » Unassigned
Manjit.Singh’s picture

Status: Needs work » Needs review
Issue tags: -Novice

Trigger testbot

LewisNyman’s picture

Status: Needs review » Needs work
Issue tags: -Needs issue summary update

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

herom’s picture

Status: Needs work » Needs review
FileSize
5.16 KB
5.51 KB
5.2 KB
964 bytes
6.09 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,648 pass(es). View

Rerolled. Addressed #65.
Also, found a visible visual issue, and added a fix. Screenshots below:

LTR:



RTL (before):



RTL (after):

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

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.

  • webchick committed 932f2f2 on 8.0.x
    Issue #2396473 by herom, Aunion, pjbaert, Manjit.Singh, Dhorkiy, b0unty...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Nice catch.

Committed and pushed to 8.0.x. Thanks!

Status: Fixed » Closed (fixed)

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

prabhurajn654’s picture

prabhurajn654’s picture