BUEditor accessibilty regression - no way to discover control keys

I'll have to get a second opinion on this from the Accessibility team. I was just reporting what they said in their review.

http://webaim.org/techniques/forms/controls#image_button says: "If you use an image button ( rather than a standard button, the input must have appropriate alt text." and suggests markup like:

<input type="image" name="submitbutton" alt="search" src="submit.png">

http://www.w3.org/WAI/GL/techniques.htm#A.1.4 says the same thing (more authoritative source).

But... The individual buttons already do have an "alt" attribute on them. For instance, the first button I am seeing here has alt="Bold(B)".

So the alt text is there. Are the browsers you are using not seeing that alt text, or do you just think the module should be putting in better alt text (maybe the same as the title attribute)?

By the way, the code that generates the alt/title text is around line 138 in
http://cgit.drupalcode.org/bueditor/tree/bueditor.js

Good explanation!

This patch doesn't look right to me:

-    alt = title + (key ? '('+ key +')' : '');
+    alt = access && key ? title + ' (Access Key: '+ access +' + '+ key +')' : title + (key ? '('+ key +')' : '');
+    title += access && key ? ' ('+ access +' + '+ key +')' : '';
     title += access && key ? ' ('+ access +' + '+ key +')' : '';

- In the alt line, it looks like it is using the key variable even if it is possibly not defined?
- The title line looks like it is now in the file twice?

Regarding your suggestion for additional text, it seems like that could be added to the loop inside

BUE.theme = function (tplid) {

in http://cgit.drupalcode.org/bueditor/tree/bueditor.js

The way I would do it would be to build up a variable called "explanation", and initialize it to

'<a class="element-invisible element-focusable" href="#">BUEditor commands: Use (Shift + Alt + COMMAND CHARACTER). Available commands: '

initially.

Then each time through the loop, in the area of the lines you are adding to the file in this patch, you should be able to do something like:

if (key) {
  explanation += title + '(' + key + '),'
}

although you'd probably want to remove the final , at the end.

Ummm...

+    if (key) {
+      if (explanation_count == 0) {
+        explanation += title + '(' + key + ')';
+      }
+      else {
+        explanation += ', ' + title + '(' + key + ')';
+      }
+    }

After adding something to explanation, you need to set explanation_count to a non-zero value so you get the comma next time?

And then here:

+    alt = access && key ? title + ' (Access Key: '+ access +' + '+ key +')' : title + (key ? '('+ key +')' : '');

This is really convoluted, with two ? : if statements. I think the code would be a lot clearer if it was a regular if() statement.

Also I see here the button is only printed out if the 'access' variable is also TRUE, but up above in explanation, you are only checking if 'key' evaluates to TRUE? Probably the whole thing could be put into one if() statement, like:

if (access) {
   if (key) {
      // do explanation, alt, title with key
   } else {
     // do explanation, alt, title without key
  }
  // add alt/title to the html variable
}

Although this would change the output slightly, since for whatever reason, the buttons are not currently being output if they have no key defined, which seems a bit odd to me.

One more thing: I forgot to mention it before but at the end of the function you need to close the A tag in explanation, plus actually output the explanation. As it is now, you're building it up but it never gets finished or printed out.

For readability, I think there should be a space between the title and the (key)? Also, it's not really good form to put a <br /> at the end of an A tag, and shouldn't be necessary. Why did you do that? Maybe needs a CSS update instead to put it on its own line and/or make it actually invisible?

Oh, I just assumed if you uploaded a patch and marked it "needs review" that you actually wanted a review. How about if we leave this issue at "needs work" until you have what you think is a viable patch then?

And by the way... Why is this explanatory text a link? It doesn't link to anything.

I'm not much of an accessibility expert, but I do remember about links being tabbable (if that's a word), so that makes sense.

Just so we're clear... I'm not one of the maintainers of this module. I just want to make sure the tools we have on Drupal.org are usable by everyone, even if they're tools I personally don't use. :)

Anyway, I think the text looks good now, and I don't see any issues in the patch (aside from needing to hide the link from sighted users presumably).

At some point, applying this patch to a d.o test site so that both regular-browser-sighted and various adaptive-browser users can try it out might be a good idea? It might also be good to test it out with a non-sighted user to find out if the text makes sense to them too?

"Some people cannot use bueditor" seems like a strong reason. The Drupal community does have accessibility as a core value, right?