Updated: Comment #19

Problem/Motivation

The HEX codes in the color module fields display the # at the end of the numbers in RTL languages, and they should still be shown at the front. @neo1691 reproduced the bug by enabling language module, changing language to RTL, and then editing theme color settings.

Proposed resolution

Correct the CSS in color.admin.css - which will set the direction every time the color changes.
Changing via js was also proposed but CSS approach preferred.
Patch at #17 by aparnakondala123 has been reviewed by Mark Carver, mradcliffe & kattekrab.
Patch at #25 appears to have resolved the issue by replacing the css selector with an #attributes array.
Before applying patch #25:
Before #25
After applying patch #25:
After #25

Remaining tasks

Possible Follow-ups
1. bug report should be posted regarding issue in #1 "(When I clicked settings, I got a page not found message, workaround was to disable the overlay module and enable it again, maybe another bug)"
2. Some strings stand out in manual test screenshot as not translated.

Original report by djbobbydrake

The RTL hexadecimal color code formatting is incorrect. The "#" should always be on the left, even in RTL. Image below:

color-rtl.png

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

neo1691’s picture

I managed to reproduce this bug on drupal-8.0-alpha2
Enabled the language module in extend->multilingual->language

Changed the language to RTL
Configure->Languages->English->Edit->Direction->RTL

I went to Appearance->Bartik->Settings->Color Scheme
(When I clicked settings, I got a page not found message, workaround was to disable the overlay module and enable it again, maybe another bug)

vineet.osscube’s picture

Assigned: Unassigned » vineet.osscube
vineet.osscube’s picture

Status: Active » Needs review
FileSize
869 bytes

I have fixed this issue. I am attaching a patch for the same.

tstoeckler’s picture

Sorry for not trying this out, but is this markup produced somewhere without JS? Otherwise patch looks great, could use a comment, however.

markhalliwell’s picture

Status: Needs review » Needs work

This bug should be fixed in color.admin.css, not in JS. This method sets the direction every time the color changes, we only need to do this once.

markhalliwell’s picture

Assigned: vineet.osscube » Unassigned
Issue tags: +CSS novice
aparnakondala1’s picture

Status: Needs work » Needs review
FileSize
802 bytes

Created a patch for thr RTL issue using CSS.

Status: Needs review » Needs work

The last submitted patch, drupal8.color-module.2003570-7.patch, failed testing.

kattekrab’s picture

@aparnakondala123
Thanks for submitting a patch! :)

diff --git "a/C:\\Users\\APARNA~1\\AppData\\Local\\Temp\\TortoiseGit\\col5AED.tmp\\color.admin-cd15b23-left.css" "b/C:\\wamp\\www\\drupal\\core\\modules\\color\\css\\color.admin.css"
index 96de7b9..472b37b 100644
--- "a/C:\\Users\\APARNA~1\\AppData\\Local\\Temp\\TortoiseGit\\col5AED.tmp\\color.admin-cd15b23-left.css"

You will need to reroll this and correct the slashes and directory path. This is an easy mistake when using windows for development.
Make sure the path is relative to the drupal root.

+++ "b/C:\\wamp\\www\\drupal\\core\\modules\\color\\css\\color.admin.css"undefined
@@ -122,3 +123,23 @@
\ No newline at end of file

Check the newline at end of file - something's wrong there too.

aparnakondala1’s picture

Patch to solve this issue using CSS

aparnakondala1’s picture

Status: Needs work » Needs review

Someone please review.

aparnakondala1’s picture

Someone please review.

markhalliwell’s picture

Status: Needs review » Needs work

Thanks for the patch @aparnakondala123! I like the simpler approach. Here is a few minor coding standard nit picks:

+++ b/core/modules/color/css/color.admin.css
@@ -122,3 +122,8 @@
+input:not([type]), input[type="text"]
+{
+direction:ltr
+}

The curly brackets should be on the same line per https://drupal.org/node/1887862#rule-sets. Also, read the "Styling for Right-To-Left Languages" section right above that.

I would also prefer to keep at least one class (or ID if no class exists) before the input portion to target just the fields in the color palette.

aparnakondala1’s picture

Status: Needs work » Needs review
FileSize
368 bytes

Patch to solve this issue using CSS

Thank you Mark Carver

mradcliffe’s picture

Status: Needs review » Needs work
+++ b/core/modules/color/css/color.admin.cssundefined
@@ -122,3 +122,7 @@
+
+#palette input:not([type]),#palette input[type="text"]{
+direction:ltr; /*LTR*/
+}

It looks like standards for CSS suggest that there must be a space after "{" and ":".

Good work so far.

markhalliwell’s picture

+++ b/core/modules/color/css/color.admin.css
@@ -122,3 +122,7 @@
+#palette input:not([type]),#palette input[type="text"]{
+direction:ltr; /*LTR*/
+}

Should be:

#palette input:not([type]), #palette input[type="text"] {
  direction: ltr; /*LTR*/
}
aparnakondala1’s picture

Status: Needs work » Needs review
FileSize
369 bytes

Updated patch

mradcliffe’s picture

Patch looks good, aparnakondala123!

kattekrab’s picture

Manual tested with simplytestme - seems to work!

Here's a screenshot.

2003570-rtl-hex-manualtest.png

removed "needs manual testing" tag.

kattekrab’s picture

Issue summary: View changes

updated issue summary.

kattekrab’s picture

-Needs issue summary update

markhalliwell’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update
  1. +++ b/core/modules/color/css/color.admin.css
    @@ -122,3 +122,7 @@
    +#palette input:not([type]),#palette input[type="text"] {
    

    There needs to be a space after the comma (,).

  2. +++ b/core/modules/color/css/color.admin.css
    @@ -122,3 +122,7 @@
    +direction: ltr; /*LTR*/
    

    There needs to be a two space indention at the beginning, it's nested inside { }.

markhalliwell’s picture

Removed tag that was added back...

tstoeckler’s picture

Wouldn't another approach be to set the direction attribute directly as part of the form array in color_scheme_form()? I would find that more intuitive than via CSS. Don't want to rain on anyone's parade, though, the current approach is totally fine as well! Just wanted to ask if there are any reasons for either approach.

markhalliwell’s picture

Hmm, tbh either is fine really (ie: I have no preference). I just didn't like that it was doing CSS in JS.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
452 bytes

Here's that approach. Hope it's OK for me to jump on this.
Tested this and get more or less the same screenshot as in #19.

Tim Bozeman’s picture

FileSize
43.06 KB
42.75 KB

This is a test of #25 before applying the patch:
before.png
and after:

after.png

Tim Bozeman’s picture

Issue summary: View changes

Updated issue summary - link to latest comment

Tim Bozeman’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me!
#25++

Tim Bozeman’s picture

Title: incorrect RTL hexadecimal color code format » Enabling RTL and then changing theme colors moves the hexadecimal # to the right

Changed issue title from "incorrect RTL hexadecimal color code format" to "Enabling RTL and then changing theme colors moves the hexadecimal # to the right."

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 882c6f0 and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

I made before and after pics of applying patch #25 and updated the proposed resolution.