Follow-up to #2349653: Copy color templates to Classy

The lock/unlock link is just an empty div, this means it is not accessible to screenreaders and is not useable without CSS.

As nod_ mentioned, this will be removed, but only in 8.1.x-dev, depending on #1651344: Use color input type in the color.module.
So as a consensus, lets make this icon accessible without restyling it.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because accessibility
Issue priority Not critical because minor accessibility defect
Unfrozen changes Unfrozen because it only changes markup
Prioritized changes The main goal of this issue is accessibility
Files: 
CommentFileSizeAuthor
#14 color-module-accessible-link-2409653-14.patch1.57 KBMathieuSpil
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,402 pass(es). View
#13 Screenshot 2015-04-13 14.33.30.jpg476.49 KBLewisNyman
#11 color-module-accessible-link-2409653-11.patch2.16 KBMathieuSpil
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,402 pass(es). View

Comments

MathieuSpil’s picture

Issue tags: -Twig
FileSize
2.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,100 pass(es). View

So here are my changes explained (for all the changes, you should still check patch)

-  var lock = $('<div class="lock"></div>').on('click', function () {
+  var lock = $('<a href="#" class="lock">' + Drupal.t('Unlock') + '</a>').on('click', function (e) {
+    e.preventDefault();

Replaced the clickable div by a actual link with linktext and a href-attribute so it is focusable.

-  $(this).addClass('unlocked');
+  $(this).addClass('unlocked').html(Drupal.t('Lock'));

Added Some logic so that the link text is being replaced on click.

-  cursor: pointer;
+  text-indent: -9999px;

Removed cursor: pointer in css because this is now a actual link.
Added text-indent: -9999px because when css is being used, it already have a background-image, and it shouldn't have visible text (But now it is accessible for screen readers)

Only remaining issue I spotted here, is that the module css file was full of unnecessary css inside media queries, so I removed a bunch of it (Just stating the obvious, that is highly unlikely that this code was ever used.)

And last but not least: Am I the only one who doesn't like those icons (If Duke Nukem would have a manbag, those would look like that)
So is it an option to replace those icons with some better ones, or remove them all together?

LewisNyman’s picture

Component: theme system » color.module
Status: Needs review » Needs work
Issue tags: -banana, -cssbanana +JavaScript, +Needs screenshots

Nice, I didn't find much to improve but we could do with nod_ giving this a quick JS review to make sure.

+++ b/core/modules/color/color.js
@@ -210,9 +210,10 @@
-            var lock = $('<div class="lock"></div>').on('click', function () {
+            var lock = $('<a href="#" class="lock">' + Drupal.t('Unlock') + '</a>').on('click', function (e) {

I think for accessibility reasons this should actually be a element?

Only remaining issue I spotted here, is that the module css file was full of unnecessary css inside media queries, so I removed a bunch of it (Just stating the obvious, that is highly unlikely that this code was ever used.)

That's cool, but we will need some before/after screenshots at various screen sizes just to make sure we haven't broken anything. I am happy to do this.

And last but not least: Am I the only one who doesn't like those icons (If Duke Nukem would have a manbag, those would look like that)
So is it an option to replace those icons with some better ones, or remove them all together?

Haha yeah they look like something out of minecraft or something. Let's open a new issue and replace them with Libricons like we have in #1356602: [META] Clean up icons in misc

MathieuSpil’s picture

Hey Lewis,

What do you mean with: "I think for accessibility reasons this should actually be a element?"?
For accessibility's sake, a clickable element schouldn't be a div. Also a 'a'-element is not focusable when there isn't a href-attribute.

Just not really sure what you meant by this :)

LewisNyman’s picture

Ah sorry, I think I forgot to use the code tag. I meant a <button> element

LewisNyman’s picture

Seven has a .link class that you can use the reset the styles of the button tag

nod_’s picture

Didn't we talk about getting rid of them altogether because it was really unclear what they did? I know Bojhan mentioned that and I agreed :p

MathieuSpil’s picture

@nod_ Can you provide a link to that issue?

mgifford’s picture

MathieuSpil’s picture

Assigned: Unassigned » MathieuSpil
MathieuSpil’s picture

Issue summary: View changes
MathieuSpil’s picture

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

Altered the patch so instead of using <a> elements, it now uses <button> elements.

MathieuSpil’s picture

Assigned: MathieuSpil » Unassigned
LewisNyman’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs screenshots
FileSize
476.49 KB

I tested this patch and it works as expected. Here's a screenshot of the functionality working without any JS errors:

+++ b/core/modules/color/color.js
--- a/core/modules/color/css/color.admin.css
+++ b/core/modules/color/css/color.admin.css

+++ b/core/modules/color/css/color.admin.css
+++ b/core/modules/color/css/color.admin.css
@@ -62,7 +62,7 @@

@@ -62,7 +62,7 @@
   width: 20px;
   height: 19px;
   background: url(../images/lock.png) no-repeat 50% 0;
-  cursor: pointer;
+  text-indent: -9999px;
 }

@@ -87,12 +87,10 @@
   .color-form label {
     float: left; /* LTR */
-    clear: left; /* LTR */

I think we should move these CSS improvements to a follow up issue, so we can keep this issue focused.

MathieuSpil’s picture

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

New patch now only has the neccessary css change: text-indent: -9999px; because I added text to the button, so people who use screenreaders can actually use these buttons for now.

There should be a new issue where we do a cleanup of the CSS (or we can ignore this, and it will be deleted eventually when this functionality is being deleted, as mentioned before)

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community
LewisNyman’s picture

LewisNyman’s picture

Issue summary: View changes
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 4b0b2c0 and pushed to 8.0.x. Thanks!

  • alexpott committed 4b0b2c0 on 8.0.x
    Issue #2409653 by MathieuSpil: The color module lock/unlock link is not...

Status: Fixed » Closed (fixed)

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