Just looking at my site through http://wave.webaim.org/toolbar

Came obvious that there are accessibility problems with the Masquerade form (see image attached).

All forms need to have a label with them. These don't need to be visible, but they do need to be there to provide semantics.

#3 1502804-accessibility-3.patch3.12 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 16 pass(es), 2 fail(s), and 0 exception(s). View
#3 masq_acessibility.jpg7.61 KBandypost
#1 1502804-accessibility-1.patch926 bytesandypost
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in sites/default/modules/masquerade/masquerade.module. View
Masquerade-accessibility-nolabel.png13.76 KBmgifford
Members fund testing for the Drupal project. Drupal Association Learn more


andypost’s picture

Status: Active » Needs review
926 bytes
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in sites/default/modules/masquerade/masquerade.module. View

Does this patch make a sense? Maybe title should be shorter?

mgifford’s picture

That would work, but would be a bit of a duplication. as the screen reader user would hear the title twice. I do wonder why you added <div class="description"></div>. I'm also not sure why you couldn't just remove $markup_value completely & use the title (without the invisible title of course).

andypost’s picture

Version: 7.x-1.0-rc4 » 7.x-1.x-dev
7.61 KB
3.12 KB
FAILED: [[SimpleTest]]: [MySQL] 16 pass(es), 2 fail(s), and 0 exception(s). View

@mgifford We can't use this long string as title because it related to both input and submit. This ugly fix just tried to implement (mimic) standard element description... but this anyway breaks a form flow... so

here's a new patch, changes
- added title Username, probably we should hide it with '#title_display' => 'invisible'
- reordered a code and added some comments

-also changed a way to display title for quick switches to more accessible

I'd like to get review of @deekayen on this because this changes the form a bit


mgifford’s picture

Patch still seems to work well when I ran it in http://simplytest.me

Status: Needs review » Needs work

The last submitted patch, 1502804-accessibility-3.patch, failed testing.

deekayen’s picture

Traditionally, if you're going to include a description, it's associated with an input field. If you're going to break from that model, I'd think you'd want the instructions to precede the form, not append to it, then again, I'm not blind. If we're really going to consider the impacts of blind people, the Go description on the submit button isn't very helpful either.

andypost’s picture

+++ b/masquerade.moduleundefined
@@ -555,8 +555,37 @@ function masquerade_block_1() {
+      '#prefix' => '<div class="form-item"><div class="description">',
+      '#markup' => $markup_value,

@@ -578,33 +607,15 @@ function masquerade_block_1() {
-    '#prefix' => '<div class="form-item"><div class="description">',
-    '#markup' => $markup_value,
-    '#suffix' => '</div></div>',

on a one hand this needs better accessible attributes and render

+++ b/masquerade.moduleundefined
@@ -555,8 +555,37 @@ function masquerade_block_1() {
+    if (masquerade_menu_access('autocomplete')) {
+    }
+    // Add instructions for form above. Don't use description for input because
+    // this used for input and submit both.
+    $form['masquerade_desc'] = array(

Anyway this description should use same rules as inputs does

+++ b/masquerade.moduleundefined
@@ -555,8 +555,37 @@ function masquerade_block_1() {
+      $form['masquerade_user_field'] = array(
+        '#prefix' => '<div class="container-inline">',

on a second - this sould be done with proper container wrapper as 8.x-2.x makes.
So description could be attached to container

deekayen’s picture

I'm not highly opinionated on this issue and don't have any background in blind people stuff, nor am I really interested in getting one.

Melissamcewen’s picture

"blind people stuff"

Err, let's just call it Accessibility, since it applies to a wide range of people who use screen readers and other accessibility devices, not just "blind people." Also it is of wide interest because a lot of us work in fields like government or education where we are legally required to produce accessible websites. But that can be a bit hard to work out- should a dev do the bare minimum which is to make it readable in a screen reader and adhere to basic web best practices, or should they be doing usability testing with a screen reader? Probably the former is best for modules like this.

deekayen’s picture

Yeah, I get it. I don't work for the government and I've turned down jobs where I found their portfolio had govt work I might have to participate in. There are too many government departments that have no legitimate constitutional existence and I won't participate willingly in their success.

#8 is simply me letting Andrey take lead on this issue. I don't really care about accessibility, nor would I attempt to maintain its compatibility in the future, but this module has quickly grown to be completely out of my control anyway. Andrey, Mike, and Daniel are good devs and I don't feel the need to clash on this issue.