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

The list of missing RTL rules is attached.

Files: 
CommentFileSizeAuthor
#49 seven_missing_rtl_rules-2396483-49.patch9.19 KBKarmen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,410 pass(es). View
#46 seven_missing_rtl_rules-2396483-46.patch9.31 KBKarmen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,405 pass(es). View
#43 seven_missing_rtl_rules-2396483-43.patch0 bytesKarmen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,405 pass(es). View
#38 2396483-rule-RTL-38.patch3.68 KBjoginderpc
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,785 pass(es). View
#30 seven_missing_rtl_rules-2396483-30.patch10.09 KBbalagan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,581 pass(es). View
#24 tour-he.jpg186.47 KBbalagan
#24 css-manual-test.zip1.15 MBbalagan
#20 print.css_.jpg139.34 KBbalagan
#14 seven_missing_rtl_rules-2396483-14.patch9.83 KBsaki007ster
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,991 pass(es). View
#8 2396483-6-after.png37.22 KBidebr
#6 seven_missing_rtl_rules-2396483-6.patch9.72 KBSoumyaDas
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,496 pass(es). View
seven-missing-rtl-rules.txt8.21 KBherom

Comments

LewisNyman’s picture

Component: CSS » Seven theme
SoumyaDas’s picture

Assigned: Unassigned » SoumyaDas
lauriii’s picture

I actually started this one day and did some progress. You can use my changes if you want :)

SoumyaDas’s picture

@lauriii Thanks, I'll take a look on your changes :) .

Gábor Hojtsy’s picture

Status: Active » Needs work
+++ b/core/themes/seven/css/components/views-ui.css
@@ -247,9 +247,12 @@ details.fieldset-no-legend {
+[dir="ltr"] .views-ui-rearrange-filter-form tr td:last-child {

@@ -281,10 +284,14 @@ details.fieldset-no-legend {
+[dir="ltr"] .views-query-info table tr td:last-child {

We don't use dir="ltr" in core by convention, instead we only provide override styles in dir="rtl" so all other styles are properly inherited from LTR and we don't need to repeat any. While here there may be only one style, that may not be true with future modifications.

SoumyaDas’s picture

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

Please find the patch file (seven_missing_rtl_rules-2396483-6.patch) for the corresponding missing rtl rules. Needs review. Thanks.

SoumyaDas’s picture

Assigned: SoumyaDas » Unassigned
idebr’s picture

Status: Needs review » Needs work
Issue tags: +Needs manual testing
FileSize
37.22 KB
  1. +++ b/core/themes/seven/css/components/dialog.theme.css
    @@ -31,13 +31,16 @@
    +[dir="rtl"] .ui-dialog .ui-dialog-titlebar-close {
    +  left: 20px;
    +}
    

    When overriding the position to left: 20px, you should also reset the right value with right: auto;

  2. +++ b/core/themes/seven/css/components/dialog.theme.css
    @@ -91,6 +94,9 @@
    +[dir="rtl"] .ui-dialog .ajax-progress-throbber {
    +  right: 49%;
    +}
    

    Same here

  3. The automated RTL test also seems to have missing the dialog title:
    .ui-dialog .ui-dialog-titlebar {
      background: #6b6b6b;
      border-top-left-radius: 5px;
      border-top-right-radius: 5px;
      padding: 15px 49px 15px 15px;
    }
    

LewisNyman’s picture

We don't use dir="ltr" in core by convention, instead we only provide override styles in dir="rtl" so all other styles are properly inherited from LTR and we don't need to repeat any. While here there may be only one style, that may not be true with future modifications.

I don't see why we shouldn't use dir="ltr" when it makes sense. Why apply a style to both ltr and rtl if you're only going to undo it again for rtl?

SoumyaDas’s picture

@idebr your observation is correct. I have tested the point 1 manually and found that "right: auto;" also needs to be added to behave it properly. Thus Needs manual testing.

Gábor Hojtsy’s picture

@LewisNyman: using dir="ltr" for some things would IMHO open the floodgates to not use the cascading system of CSS and would make it much easier to define independent styles for RTL and LTR making maintenance much harder. Using the cascade, it is trivial to figure out for example which styles you need to override, while otherwise you need to dig up styles that don't apply in RTL to see what you need to do in RTL.

Gábor Hojtsy’s picture

Issue tags: +SprintWeekend2015Queue
saki007ster’s picture

Assigned: Unassigned » saki007ster
saki007ster’s picture

FileSize
9.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,991 pass(es). View

Re-rolling the patch and updated the css for issues mentioned in the comment #8

saki007ster’s picture

Assigned: saki007ster » Unassigned
Status: Needs work » Needs review
Gábor Hojtsy’s picture

Issue tags: +SprintWeekend2015
saki007ster’s picture

Issue tags: +#punedrupalgroup
balagan’s picture

Assigned: Unassigned » balagan
emanuelrighetto’s picture

Status: Needs review » Reviewed & tested by the community

I followed the check list and I can tell the patch resolve the issue: there are all the missing rules now.

balagan’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
FileSize
139.34 KB

When printing the page, there are no border on the right. When I use

[dir="rtl"] .messages {
    border-right-color: #999;
	border-width: 1px;
  }

, then it appears properly. It seems it is not inherited from LTR styles.

idebr’s picture

+++ b/core/themes/seven/css/components/dropbutton.component.css
@@ -16,10 +16,18 @@
 .js .dropbutton-action.last {
-  border-radius: 0 0 0 1em;
+  border-radius: 0 0 0 1em; /* LTR */
+}
+[dir="rtl"] .js .dropbutton-action.last {
+  border-radius: 0 0 1em 0;

This conversion is incorrect. Also I believe this css is not applied any since #theme => links no longer adds 'first' and 'last' classes, so it can be removed entirely.

balagan’s picture

+++ b/core/themes/seven/css/components/form.css
@@ -148,6 +148,20 @@ textarea.form-textarea {
+[dir="rtl"] input.form-autocomplete,
+[dir="rtl"] input.form-text,
+[dir="rtl"] input.form-tel,
+[dir="rtl"] input.form-email,
+[dir="rtl"] input.form-url,
+[dir="rtl"] input.form-search,
+[dir="rtl"] input.form-number,
+[dir="rtl"] input.form-color,
+[dir="rtl"] input.form-file,
+[dir="rtl"] input.form-date,
+[dir="rtl"] input.form-time,
+[dir="rtl"] textarea.form-textarea {
+  padding: .3em .5em .3em .4em;
+}

Should not we add /* LTR */ to all the LTR equivalents as in all other cases?

Gábor Hojtsy’s picture

Yup.

balagan’s picture

FileSize
1.15 MB
186.47 KB

I have uploaded a zip file containing the screenshots I have made using Hungarian and Hebrew languages. They seem to be all right, although I have found an error displaying a tour with RTL. The close button is on the right, although it should be on the left, as in the ui-dialog-titlebar-close-he.jpg file (see zip file).

balagan’s picture

Assigned: balagan » Unassigned
balagan’s picture

I cannot reproduce the missing border I have noted before, so I think we can forget about that.

develCuy’s picture

Issue tags: -SprintWeekend2015Queue
develCuy’s picture

Issue tags: +SprintWeekend2015Queue

Removed tag by mistake.

balagan’s picture

Assigned: Unassigned » balagan
balagan’s picture

FileSize
10.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,581 pass(es). View

@idebr: for your comment in #21, I think the conversion is correct. As far as I know the order of the values are: top left, top right, bottom right, and bottom left.

Also I believe this css is not applied any since #theme => links no longer adds 'first' and 'last' classes, so it can be removed entirely.

I have no idea about it, somebody please confirm that this can be removed.
Until then I have added the missing /* LTR */ to the patch.

balagan’s picture

Assigned: balagan » Unassigned
lauriii’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Needs work
  1. +++ b/core/themes/seven/css/components/buttons.theme.css
    @@ -89,13 +89,17 @@
    -  margin-left: -0.2em;
    -  padding-right: 0.2em;
    +  margin-left: -0.2em; /* LTR */
    +  padding-right: 0.2em; /* LTR */
       font-size: 14px;
       font-size: 0.875rem;
       line-height: 16px;
       -webkit-font-smoothing: auto;
     }
    +[dir="rtl"] .button-action:before {
    +  margin-right: -0.2em;
    +  padding-left: 0.2em;
    +}
    

    This will end up having margins/paddings on both sides in RTL, right?

  2. +++ b/core/themes/seven/css/components/dialog.theme.css
    @@ -77,7 +81,8 @@
    -  left: 49%;
    +  left: 49%; /* LTR */
    +  right: auto;
       position: fixed;
       top: 48.5%;
       z-index: 1000;
    @@ -91,6 +96,9 @@
    
    @@ -91,6 +96,9 @@
       padding: 4px;
       width: 24px;
     }
    +[dir="rtl"] .ui-dialog .ajax-progress-throbber {
    +  right: 49%;
    +}
    

    Why did right auto end up in LTR? Also this will end up with both left and right at 49% in RTL no?

  3. +++ b/core/themes/seven/css/components/dropbutton.component.css
    @@ -204,9 +212,12 @@
       border-top-color: #333;
    -  right: 35%;
    +  right: 35%; /* LTR */
       top: 54%;
     }
    +[dir="rtl"] .dropbutton-arrow {
    +  left: 35%;
    +}
    

    Same thing.

  4. +++ b/core/themes/seven/css/components/tour.theme.css
    @@ -34,17 +34,19 @@
     .joyride-tip-guide .joyride-nub.right {
       border-top-color: transparent;
    -  border-right-color: transparent;
    +  border-right-color: transparent; /* LTR */
       border-bottom-color: transparent;
     }
    +[dir="rtl"] .joyride-tip-guide .joyride-nub.right {
    +  border-left-color: transparent;
    +}
     .joyride-tip-guide .joyride-nub.left {
       border-top-color: transparent;
    -  border-left-color: transparent;
    +  border-left-color: transparent; /* LTR */
       border-bottom-color: transparent;
     }
    -.joyride-tip-guide .joyride-nub.top-right {
    -  border-top-color: transparent;
    -  border-left-color: transparent;
    +
    +[dir="rtl"] .joyride-tip-guide .joyride-nub.left {
       border-right-color: transparent;
     }
    

    Will leave the right and left color transparent in the first and second case in RTL respectively, no?

  5. +++ b/core/themes/seven/css/components/views-ui.css
    @@ -248,7 +248,10 @@ details.fieldset-no-legend {
     .views-ui-rearrange-filter-form tr td:last-child {
    -  border-right: medium none;
    +  border-right: medium none; /* LTR */
    +}
    +[dir="rtl"] .views-ui-rearrange-filter-form tr td:last-child {
    +  border-left: medium none;
     }
    

    Should the right border be undone?

  6. +++ b/core/themes/seven/css/components/views-ui.css
    @@ -283,7 +286,11 @@ details.fieldset-no-legend {
    -  border-right: 0 none;
    +  border-right: 0 none; /* LTR */
    +}
    +[dir="rtl"] .views-query-info table tr td:last-child {
    +  /* Fixes a Seven style that bleeds down into this table unnecessarily */
    +  border-left: 0 none;
    

    Same.

Gábor Hojtsy’s picture

Is someone planning to work on this one?

juankvillegas’s picture

balagan’s picture

@juankvillegas, no, both issues are part of a meta issue, one refers to the seven theme css, the other the system css files (whatever that means).

Gábor Hojtsy’s picture

Issue tags: -sprint

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

joginderpc’s picture

Assigned: Unassigned » joginderpc
Status: Needs work » Needs review
FileSize
3.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,785 pass(es). View

Please review

LewisNyman’s picture

Status: Needs review » Needs work

I tried applying this patch and I get the error: "fatal: unrecognized input". Did you modify the patch after creating it? I think I've only seen this happen before if I modify a patch in a text editor that likes to auto correct whitespace etc.

+++ b/core/themes/seven/css/components/dialog.theme.css
@@ -81,7 +81,7 @@
-  top: 48.5%;
+  top: 49%; /* for both side having same position. */

Also, why are changing the vertical position? How does this affect RTL?

joginderpc’s picture

There position updated is older one from other style sheet, not mine.

Gábor Hojtsy’s picture

Issue tags: +Novice
Karmen’s picture

Assigned: joginderpc » Karmen
Karmen’s picture

Status: Needs work » Needs review
FileSize
0 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,405 pass(es). View

Reroll made and more rules of this link were added.

Review, please.

LewisNyman’s picture

Status: Needs review » Needs work

@Karmen Looks like this patch is empty?

Also, did you use a tool to generate that text? It looks useful

Karmen’s picture

OMG! Yes, I'm going to review it.

Karmen’s picture

FileSize
9.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,405 pass(es). View

Trying again, sorry.

And no, i didn't use any tool :P

Karmen’s picture

Status: Needs work » Needs review
LewisNyman’s picture

Status: Needs review » Needs work

Great thanks for working on this.

+++ b/core/themes/seven/css/base/print.css
@@ -47,7 +47,11 @@
   .messages {
     border-width: 1px;
-    border-left-color: #999;
+    border-left-color: #999; /* LTR */
+  }
+  [dir="rtl"] .messages {
+    border-left-color: transparent;
+    border-right-color: #999;

We can make this simpler by just changing "border-left-color" to "border-color" and then we don't need any special RTL styling

Apart from that, it looks good.

Karmen’s picture

Status: Needs work » Needs review
FileSize
9.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,410 pass(es). View

Are you thinking about this change?

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

Yep that's it. Thanks!

Karmen’s picture

Assigned: Karmen » Unassigned
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

CSS is not yet frozen in beta. Committed 28a68ac and pushed to 8.0.x. Thanks!

  • alexpott committed 28a68ac on 8.0.x
    Issue #2396483 by Karmen, balagan, SoumyaDas, saki007ster, lauriii,...
fran seva’s picture

Issue tags: +drupaldevdays

Status: Fixed » Closed (fixed)

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