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

Files: 
CommentFileSizeAuthor
#26 before.png42.75 KBTim Bozeman
#26 after.png43.06 KBTim Bozeman
#25 2003570-25-color-scheme-ltr.patch452 byteststoeckler
PASSED: [[SimpleTest]]: [MySQL] 58,071 pass(es).
[ View ]
#19 2003570-rtl-hex-manualtest.png72.51 KBkattekrab
#17 drupal8.color-module.2003570-17.patch.patch369 bytesaparnakondala123
PASSED: [[SimpleTest]]: [MySQL] 58,109 pass(es).
[ View ]
#14 drupal8.color-module.2003570-14.patch.patch368 bytesaparnakondala123
PASSED: [[SimpleTest]]: [MySQL] 58,355 pass(es).
[ View ]
#10 drupal8.color-module.2003570-10.patch.patch344 bytesaparnakondala123
PASSED: [[SimpleTest]]: [MySQL] 58,155 pass(es).
[ View ]
#7 drupal8.color-module.2003570-7.patch802 bytesaparnakondala123
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.color-module.2003570-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#3 incorrect_hexadecimal_color_code-2003570-3.patch869 bytesvineet.osscube
PASSED: [[SimpleTest]]: [MySQL] 57,190 pass(es).
[ View ]
color-rtl.png65.44 KBdjbobbydrake

Comments

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)

Assigned:Unassigned» vineet.osscube

Status:Active» Needs review
StatusFileSize
new869 bytes
PASSED: [[SimpleTest]]: [MySQL] 57,190 pass(es).
[ View ]

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

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

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.

Assigned:vineet.osscube» Unassigned
Issue tags:+css-novice

Status:Needs work» Needs review
StatusFileSize
new802 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.color-module.2003570-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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

StatusFileSize
new344 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,155 pass(es).
[ View ]

Patch to solve this issue using CSS

Status:Needs work» Needs review

Someone please review.

Someone please review.

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.

Status:Needs work» Needs review
StatusFileSize
new368 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,355 pass(es).
[ View ]

Patch to solve this issue using CSS

Thank you Mark Carver

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.

+++ 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*/
}

Status:Needs work» Needs review
StatusFileSize
new369 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,109 pass(es).
[ View ]

Updated patch

Patch looks good, aparnakondala123!

Issue tags:-Needs manual testing
StatusFileSize
new72.51 KB

Manual tested with simplytestme - seems to work!

Here's a screenshot.

2003570-rtl-hex-manualtest.png

removed "needs manual testing" tag.

Issue summary:View changes

updated issue summary.

-Needs issue summary update

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 { }.

Removed tag that was added back...

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.

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

Status:Needs work» Needs review
StatusFileSize
new452 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,071 pass(es).
[ View ]

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.

StatusFileSize
new43.06 KB
new42.75 KB

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

after.png

Issue summary:View changes

Updated issue summary - link to latest comment

Status:Needs review» Reviewed & tested by the community

Looks good to me!
#25++

Title:incorrect RTL hexadecimal color code formatEnabling 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."

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.

Issue summary:View changes

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