Add missing RTL rules to Seven theme CSS

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

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

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

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

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;
   }
   

   

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?

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

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

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

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

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.

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

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

Yup.

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

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

Removed tag by mistake.

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

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.

Is someone planning to work on this one?

It is duplicated #2396473: Add missing RTL rules to System CSS

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

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

Please review

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?

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

Reroll made and more rules of this link were added.

Review, please.

@Karmen Looks like this patch is empty?

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

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

Trying again, sorry.

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

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.

Are you thinking about this change?

Yep that's it. Thanks!

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