All of the "add content" functions in the Panels IPE have two A tags: one for the icon, and one for the text of the link. The icons do not have any alt text. This can cause screenreaders to read out the destination path instead, which is garbled and meaningless.
Icons used in links should have either empty alt text (if the icon duplicates the meaning of text within the same link tag) or meaningful alt text (if the icon is the only content of the link tag, or if it adds to the meaning of the link text).
Suggested solution: Combine the icon and the text inside one A tag - since the icon duplicates the meaning of the text in the link, it can have an empty alt attribute.
| Comment | File | Size | Author |
|---|---|---|---|
| #22 | ctools-icon-text-renderer-2280875-22.patch | 1.49 KB | cboyden |
| ipe-duplicate-links.png | 781.04 KB | cboyden |
Comments
Comment #1
cboyden commentedPart of this issue comes from ctools: it provides a renderer for a text link and a renderer for an image link but not for a link that includes both an icon and text.
Comment #2
cboyden commentedComment #3
cboyden commentedMoving to ctools: will patch to add an ajax renderer that can handle an icon with text inside the same link tag.
Comment #4
cboyden commentedMoving to correct ctools project.
Comment #5
cboyden commentedThis patch adds an Ajax renderer that creates a link with an icon and text within the same A tag. It is based on the existing renderers for image buttons and text buttons. It adds no new variables and will not break backward compatibility.
Comment #6
cboyden commentedThis patch sets up Panels to use the new icon text renderer for Add Content links in the panelizer. It depends on the ctools patch in the previous comment.
Comment #8
japerryAfter talking to Caroline about this at Drupalcon, I think this solution makes sense for the architecture we live in. Its not perfect, but the structure of adding icons would have to be fairly re-written in order to provide a clean solution to icons and links.
The only other concern I have with screen-readers and accessibility, what about javascript/ajax? This solution won't work if JS is disabled. But I'm a bit ignorant when it comes to screen reading technology, maybe this isn't an issue? I suppose since its already being displayed in a panels modal, JS is going to enabled ;-)
I'll give it a test this week, and as long as its backwards compatible (aka panels doesn't break w/o the panels patch), and no one has a problem with it only working with JS enabled.
Comment #9
dsnopek@japerry: Most screenreaders are browser plugins, so Javascript/AJAX will work! This issue actually comes from testing that Caroline and her team did with Panopoly using JAWS, the most popular screenreader (which works as a plugin for Internet Explorer).
We'll start testing this in Panopoly soon!
Comment #10
cboyden commentedScreenreaders are a bit more complicated than that - but most Javascript is handled fine by recent screenreaders. It's a bit of an arms race. New standards and techniques are introduced, the screenreaders catch up. As a developer I find that keyboard accessibility is a good proxy for screenreader accessibility and speech input, and other alternative input modes.
Comment #11
cboyden commentedDo you think we need to add some logic in the Panels patch to cover the case where a site may update Panels and not update ctools?
Comment #12
mgiffordNice, so where is ctools_ajax_icon_text_button() defined?
Comment #13
cboyden commentedThe renderer function lives in /includes/ajax.inc in the ctools module, along with the original text and image button renderers. So that has to be in ctools for Panels to take advantage of it.
In Panels, I added a line to template_preprocess_panels_add_content_link() in the add-content.inc file that builds an icon/text link at the same time as the separate image button and text buttons were already being built. So if anyone's relying on the image or text button in some custom context, they'll still be available. There's an addition to the stylesheet to make the display match. And then I rewrote the tpl.php in Panels to use the new link instead of the two old ones.
Comment #14
japerry@cboyden:
Yup, I think that'd be a good idea. We tried to do a dependency with the last version of ctools/panels, which didn't work out so well. Instead its probably best to just put a wrapper around the function name since we're making an API addition.
Gonna mark needs work so we can get a patch for that change. Otherwise I think its good to go once thats in.
Comment #15
cboyden commentedOK, I've changed the Panels patch so that in add-content.inc we check that the new function exists before trying to set the variable, and in the tpl.php we check to see if the $icon_text_button variable is set, and if not, the image and text buttons are output separately as before. Tested with both patched and unpatched versions of ctools. It works OK in both situations, and the visual display is the same regardless.
Comment #16
cboyden commentedComment #18
mgiffordWhy did you add a span to the CSS if you didn't add a span in panels-add-content-link.tpl.php?
Was the patch rolled against the git version of the module? Trying to figure out why it failed.
Comment #19
cboyden commentedThe patch is failing because it's for Panels, not ctools. japerry suggested I post it here, but I will create a sub-issue in Panels so the testbot will run there.
The span is added in the ctools function. It's meant to be a complete package with text and link, so the tpl.php doesn't insert any more markup.
Comment #20
cboyden commentedMoved Panels patch to child issue for testbot. Updated issue title.
Comment #21
dsnopekWorked great in manual testing! Just committed to Panopoly. I have one minor code review comment, though:
There are two random '###' comments. They should probably be removed!
Comment #22
cboyden commentedRemoved empty comments.
Comment #23
dsnopekLooks good to me! Marking RTBC. :-)
Comment #25
japerryLooks good to me, I think you're right, this should be committed before panels is committed and a different issue should be made for it.
Now off to look at that issue ;-)