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
CommentFileSizeAuthor
#14 color-module-accessible-link-2409653-14.patch1.57 KBMathieuSpil
#13 Screenshot 2015-04-13 14.33.30.jpg476.49 KBLewisNyman
#11 color-module-accessible-link-2409653-11.patch2.16 KBMathieuSpil
#1 color-module-accessible-link-2409653-1.patch2.15 KBMathieuSpil
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

MathieuSpil’s picture

MathieuSpil CreditAttribution: MathieuSpil commented
Issue tags: -Twig
FileSize
color-module-accessible-link-2409653-1.patch2.15 KB

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

LewisNyman
he/him
CreditAttribution: LewisNyman commented
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

MathieuSpil CreditAttribution: MathieuSpil commented

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

LewisNyman
he/him
CreditAttribution: LewisNyman commented

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

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented

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

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

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

MathieuSpil CreditAttribution: MathieuSpil commented

@nod_ Can you provide a link to that issue?

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented
Related issues: +#1164796: The lock icon attracts unwanted attention, and its purpose is not clear.

I suspect it was #1164796: The lock icon attracts unwanted attention, and its purpose is not clear.. Good catch @LewisNyman

MathieuSpil’s picture

MathieuSpil CreditAttribution: MathieuSpil commented
Assigned: Unassigned » MathieuSpil
MathieuSpil’s picture

MathieuSpil CreditAttribution: MathieuSpil commented
Issue summary: View changes
MathieuSpil’s picture

MathieuSpil CreditAttribution: MathieuSpil commented
Status: Needs work » Needs review
FileSize
color-module-accessible-link-2409653-11.patch2.16 KB

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

MathieuSpil’s picture

MathieuSpil CreditAttribution: MathieuSpil commented
Assigned: MathieuSpil » Unassigned
LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs screenshots
FileSize
Screenshot 2015-04-13 14.33.30.jpg476.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

MathieuSpil CreditAttribution: MathieuSpil commented
Status: Needs work » Needs review
FileSize
color-module-accessible-link-2409653-14.patch1.57 KB

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

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Status: Needs review » Reviewed & tested by the community

Great! Thanks a lot. Follow up issue: #2470069: Refactor color module CSS inline with our CSS standards

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
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.