I have carefully gone through all the pages in a normal Drupal 8 installation, and identified the remaining RTL issues (some minor, some clearly broken).

These issues could not have been fixed earlier, since there were heavy changes and refactoring in the CSS and base themes up to the RC freeze.

The changes are mostly non-disruptive. They are only in the CSS, and clearly only affect RTL languages.

I have uploaded screenshots for LTR, RTL (before patch), and RTL (after patch) for every fix, but to prevent the issue summary from getting too long, will only embed the RTL (Before patch) ones in the summary.

Two notes for reviewers:

  • You don't need to add a separate language, or modify any settings to test the fixes (except the ckeditor toolbar one). All you have to do is modify the dir="ltr" on the <html> tag to "rtl" (using firebug or the browser's developer tools).
  • When a CSS specificity issue happens between a module's CSS and a theme's CSS, the fix is added to the theme. So, some of the rules might seem out-of-place (not adjacent to their equal LTR rule), but this is a more appropriate fix.

So, here are the issues (starting with a weird one):

1. Display Tabs in Views Admin

2. Link Icons in Module Descriptions in Modules Admin Page

3. Requirements List in Modules Uninstall Page

4. List of Links in Module Help Pages

5. Content Type Manage Display

6. Notification Messages

7. Content Types' Manage Form Display

8. Add Article (Save and Publish Button)

9. Password Strength Notes in User Create/Add

10. Bartik Main Menu (Front Page) (460px < width < 900px)

11. Bartik Secondary Menu

12. Text Formats - CKEditor Toolbar Settings

13. Action Buttons in Views Admin Page

14. Views Admin Modal Dialogs

15. Filters in Views Admin Modal Dialogs

CommentFileSizeAuthor
#13 remaining_rtl_fixes_for-2587549-13.patch12.22 KBherom
#13 interdiff-2587549-2-13.txt1.32 KBherom
#9 remaining-rtl-issues-15.png26.65 KBprestonso
#9 remaining-rtl-issues-14.png19.05 KBprestonso
#9 remaining-rtl-issues-13.png10.56 KBprestonso
#9 remaining-rtl-issues-12.png52.05 KBprestonso
#9 remaining-rtl-issues-11.png19.21 KBprestonso
#9 remaining-rtl-issues-10.png26.82 KBprestonso
#9 remaining-rtl-issues-9.png27.31 KBprestonso
#9 remaining-rtl-issues-8.png45.77 KBprestonso
#9 remaining-rtl-issues-7.png38.47 KBprestonso
#9 remaining-rtl-issues-6.png43.65 KBprestonso
#9 remaining-rtl-issues-5.png25.33 KBprestonso
#9 remaining-rtl-issues-4.png31.92 KBprestonso
#9 remaining-rtl-issues-3.png25.89 KBprestonso
#9 remaining-rtl-issues-2.png32.77 KBprestonso
#9 remaining-rtl-issues-1.png14.04 KBprestonso
#8 remaining-rtl-issues-11.png30.73 KBprestonso
#8 remaining-rtl-issues-9.png42.83 KBprestonso
#8 remaining-rtl-issues-8.png69.45 KBprestonso
#8 remaining-rtl-issues-7.png62.44 KBprestonso
#8 remaining-rtl-issues-6.png53.96 KBprestonso
#8 remaining-rtl-issues-5.png32.93 KBprestonso
#8 remaining-rtl-issues-4.png46.6 KBprestonso
#8 remaining-rtl-issues-3.png39.15 KBprestonso
#8 remaining-rtl-issues-2.png47.7 KBprestonso
#8 remaining-rtl-issues-1.png20.82 KBprestonso
#2 drupal-8-rtl-fixes.patch12.22 KBherom
manage-form-display-rtl-after.png10.16 KBherom
manage-form-display-rtl-before.png17.54 KBherom
manage-form-display-ltr.png10.6 KBherom
views-admin-tabs-rtl-after.png2.02 KBherom
views-admin-tabs-rtl-before.gif30.76 KBherom
views-admin-tabs-ltr.png2.06 KBherom
views-admin-inline-form-rtl-after.png4.14 KBherom
views-admin-inline-form-rtl-before.png7.04 KBherom
views-admin-inline-form-ltr.png4.25 KBherom
views-admin-dialog-rtl-after.png3.21 KBherom
views-admin-dialog-rtl-before.png4.69 KBherom
views-admin-dialog-ltr.png3.05 KBherom
views-admin-actions-rtl-after.png2.2 KBherom
views-admin-actions-rtl-before.png3.82 KBherom
views-admin-actions-ltr.png2.21 KBherom
text-formats-rtl-after.png13.01 KBherom
text-formats-rtl-before.png19.75 KBherom
text-formats-ltr.png12.95 KBherom
secondary-menu-rtl-after.png6.34 KBherom
secondary-menu-rtl-before.png8.92 KBherom
secondary-menu-ltr.png6.46 KBherom
primary-menu-rtl-after.png9.96 KBherom
primary-menu-rtl-before.png10.5 KBherom
primary-menu-ltr.png9.01 KBherom
password-notes-rtl-after.png4.85 KBherom
password-notes-rtl-before.png6.6 KBherom
password-notes-ltr.png4.91 KBherom
node__add__article-rtl-after.png6.68 KBherom
node__add__article-rtl-before.png11.2 KBherom
node__add__article-ltr.png7.55 KBherom
manage-fields-rtl-after.png5.56 KBherom
manage-fields-rtl-before.png7.87 KBherom
manage-fields-ltr.png5.81 KBherom
manage-display-rtl-after.png5.45 KBherom
manage-display-rtl-before.png8.38 KBherom
manage-display-ltr.png5.4 KBherom
help-pages-rtl-after.png2.42 KBherom
help-pages-rtl-before.png3.73 KBherom
help-pages-ltr.png2.43 KBherom
admin__modules__uninstall-rtl-after.png3.89 KBherom
admin__modules__uninstall-rtl-before.png5.65 KBherom
admin__modules__uninstall-rtl-before.png5.65 KBherom
admin__modules__uninstall-ltr.png4.12 KBherom
admin__modules-links-rtl-after.png4.97 KBherom
admin__modules-links-rtl-before.png7.54 KBherom
admin__modules-links-ltr.png5.03 KBherom
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

herom created an issue. See original summary.

herom’s picture

herom CreditAttribution: herom commented
Status: Active » Needs review
FileSize
drupal-8-rtl-fixes.patch12.22 KB

And here is the patch.

herom’s picture

herom CreditAttribution: herom commented
Issue summary: View changes

Added numbering to the issues referenced in the summary.

herom’s picture

herom CreditAttribution: herom commented
Issue tags: -i18n +D8MI

Fixed a tag. Also, the uploaded screenshots did not show up in the issue summary (Files section), but they are available in the Files block at the bottom of the page.

herom’s picture

herom CreditAttribution: herom commented
Priority: Normal » Major

Raising to Major as in "Major for RTL (Farsi/Hebrew/Arabic) Sites". Feel free to revert if not appropriate.

prestonso’s picture

prestonso CreditAttribution: prestonso commented

Thanks for the comprehensive IS, @herom! I was able to reproduce all of these issues on a clean install at HEAD.

Reviewing the patch now.

prestonso’s picture

prestonso CreditAttribution: prestonso commented

These are extraordinarily minor points so I will move straight into testing.

  1. +++ b/core/themes/seven/css/components/dropbutton.component.css
@@ -18,10 +18,11 @@
+[dir="rtl"].js .dropbutton .dropbutton-action > button {

    Nice catch! 

  2. +++ b/core/themes/seven/css/components/dropbutton.component.css
@@ -18,10 +18,11 @@
+  margin-left: 0; /* This is required to win over specificty of [dir="rtl"] .dropbutton-multiple .dropbutton .dropbutton-action > * */

    Spelling: "specificity" instead of "specificty" 

  3. +++ b/core/themes/seven/css/components/tabs.css
@@ -282,11 +293,26 @@ li.tabs__tab a {
+/* This is required to win over specificty of [dir="rtl"] .tabs.secondary a */

    Spelling: "specificity" instead of "specificty"

prestonso’s picture

prestonso CreditAttribution: prestonso as a volunteer commented
Issue summary: View changes
FileSize
remaining-rtl-issues-1.png20.82 KB
remaining-rtl-issues-2.png47.7 KB
remaining-rtl-issues-3.png39.15 KB
remaining-rtl-issues-4.png46.6 KB
remaining-rtl-issues-5.png32.93 KB
remaining-rtl-issues-6.png53.96 KB
remaining-rtl-issues-7.png62.44 KB
remaining-rtl-issues-8.png69.45 KB
remaining-rtl-issues-9.png42.83 KB
remaining-rtl-issues-11.png30.73 KB

Results of testing: All good to go apart from the minor points mentioned in #7.

For bullet no. 12 below, replacing class cke_ltr with cke_rtl, as will normally happen to CKE on a RTL localization, eliminated the text indent/overflow issue, though the screenshot does not reflect this.

1. Display tabs in Views admin

Display tabs in Views admin after patch

2. Link icons in module descriptions in modules admin page

Link icons in module descriptions in modules admin page after patch

3. Requirements list in modules uninstall page

Requirements list in modules uninstall page after patch

4. List of links in module help pages

List of links in module help pages after patch

5. Content type manage display

Content type manage display after patch

6. Notification messages

Notification messages after patch

7. Content types manage form display

Content types manage form display after patch

8. Add article save and publish button

Add article save and publish button after patch

9. Password strength notes in user creation

Password strength notes in user creation after patch

10. Bartik main menu on front

Bartik main menu on front after patch

11. Bartik secondary menu

Bartik secondary menu after patch

12. CKEditor toolbar settings

CKEditor toolbar settings after patch

13. Views admin page action buttons

Views admin page action buttons after patch

14. Views admin modal dialogs

Views admin modal dialogs after patch

15. Filters in Views admin modal dialogs

Filters in Views admin modal dialogs after patch

prestonso’s picture

prestonso CreditAttribution: prestonso as a volunteer commented
FileSize
remaining-rtl-issues-1.png14.04 KB
remaining-rtl-issues-2.png32.77 KB
remaining-rtl-issues-3.png25.89 KB
remaining-rtl-issues-4.png31.92 KB
remaining-rtl-issues-5.png25.33 KB
remaining-rtl-issues-6.png43.65 KB
remaining-rtl-issues-7.png38.47 KB
remaining-rtl-issues-8.png45.77 KB
remaining-rtl-issues-9.png27.31 KB
remaining-rtl-issues-10.png26.82 KB
remaining-rtl-issues-11.png19.21 KB
remaining-rtl-issues-12.png52.05 KB
remaining-rtl-issues-13.png10.56 KB
remaining-rtl-issues-14.png19.05 KB
remaining-rtl-issues-15.png26.65 KB
prestonso’s picture

prestonso CreditAttribution: prestonso as a volunteer commented
prestonso’s picture

prestonso CreditAttribution: prestonso as a volunteer commented
Status: Needs review » Needs work

Passing back to @herom.

prestonso’s picture

prestonso CreditAttribution: prestonso as a volunteer commented
Issue summary: View changes
herom’s picture

herom CreditAttribution: herom commented
Status: Needs work » Needs review
FileSize
interdiff-2587549-2-13.txt1.32 KB
remaining_rtl_fixes_for-2587549-13.patch12.22 KB

Thanks for the great review, @prestonso.
The points are fixed.

prestonso’s picture

prestonso CreditAttribution: prestonso as a volunteer commented
Status: Needs review » Reviewed & tested by the community

No regressions in the code or in functionality; RTBC. Nice work @herom!

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: -rc target triage +rc target

Discussed with the D8 committers and we agreed with making this an rc target.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

WOW! I just want to say a HUGE thanks for the extremely thorough job on screenshots. Makes this patch hugely easier to review.

This is changing CSS, which is technically frozen, but it's fixing legit bugs, and will help translators, so woop woop.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 23236e0 on 8.0.x 
    Issue #2587549 by herom, prestonso: Remaining RTL fixes for Drupal 8
herom’s picture

herom CreditAttribution: herom commented

YAAAAAAAYYYYY!!! Thanks everyone.

Let's Party ‮ Let's Party

Status: Fixed » Closed (fixed)

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