The current links are '(-)'. This fails WCAG 2.0 A "Link Purpose (In Context)":
http://www.w3.org/WAI/WCAG20/quickref/#qr-navigation-mechanisms-refs

I also see it as a usability problem, because:

  1. It's not obvious what clicking the link will do
  2. It's a smaller-than-necessary click target
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

daniel.nitsche’s picture

Submitting suggested patch. This version produces more accessible links, but it might be too verbose for the default. I'd suggest hiding the @name using <span class="visuallyhidden"></span> if this is the case.

cpliakas’s picture

Priority: Normal » Minor
Status: Needs review » Needs work

Hi daniel.nitsche.

Thanks for bringing up this very important topic. As a result, I added an "Accessibility" component to which I am assigning this post. I definitely want to resolve this issue, but I am marking this patch as "needs work" for a few reasons.

  • Patches should be created from the root of the module directory, otherwise people will have trouble applying it.
  • The guidelines you mentioned above (section 2.2.4) are for navigation elements, whereas the "(-)" links are more non-text controls IMO. Looking through the guidelines in section 1.1.1, it appears that in order to make non-text controls compliant they must have a textual name. The recommended techniques suggest that using a "title" attribute in the link will satisfy this requirement.
  • This area of code is alterable by overriding the theme function, and the widgets are pluggable as well. Therefore there are enough methods for users to be able to modify the link according to whatever standards they are trying to meet. I am very much interested in adhering to WGAC 2.0 standards, but I don't want to change the UI for 1,000 existing Facet API installations if I don't absolutely have to. If we can find a way to either keep the "(-)" and make it accessible or provide some configuration option for the links widget that would allow for WCAG compliance, that would be ideal.

Thanks again for posting the patch, and I look forward to working together on this issue.
~Chris

Everett Zufelt’s picture

Component: Code » Accessibility
Status: Active » Needs work

I haven't taken a look at the patch, and don't completely understand the problem based on the issue summary.

I can say that using the title attribute is almost never a sufficient method of ensuring accessibility, as most screen-readers, and no keyboard only users, can access @title.

If someone can show an example of current markup, and the problem, I can make more detailed recommendations.

cpliakas’s picture

FileSize
141.41 KB

Thanks, Everett. See the attached image for the current state of things.

~Chris

Everett Zufelt’s picture

Hi Chris

I am actually completely blind and using a screen-reader. Can you please provide a clear and detailed description of the image you attached. Or, an example of current markup and a description of the problem?

Thanks,
Everett

cpliakas’s picture

Sorry for unintentionally making assumptions in my post. I guess that is why there are accessibility issues :-)

In terms of how Facet API works, when you execute a search you are presented with items that you can filter by. These might be taxonomy terms, content types, field data, whatever. The display is configurable via Facet API's widget system, and the most popular method is presenting the items as an unordered list of clickable links. The text inside the link is the facet item's display value, for example the name of the content type, followed by an integer in parenthesis denoting the number of results the facet item is present in. An example of the markup for inactive facets is <li class="leaf"><a href="/search/site/mykeys?f[0]=bundle%3Aarticle" class="facetapi-inactive">Article (77)</a></li>.

Clicking on an item's link activates the filter which narrows the result set by whatever criteria is selected. From a display standpoint, the text is no longer wrapped in a link, but it it preceded by a link with the text "(-)" to remove the filter. An example of the markup for active facets is <li class="leaf first"><a href="/search/site/mykeys" class="facetapi-active">(-) </a> Article</li>.

Thanks for reviewing,
Chris

daniel.nitsche’s picture

Hi Chris,

I'm not sure I agree that the link in question is a control, although I couldn't find anything in WCAG 2 that says a link can't be a control. More importantly however, the current link does pose a real accessibility problem.

Two examples using a screenreader (JAWS):

  1. The user tabs through links until they come across the link in question. JAWS reads out
    Left parent dash right parent link

    (where "parent" is short for parenthesis).

  2. The user brings up a list of links on the page, and moves through them. When they get to the link in question, again they will hear something like
    Left parent dash right parent link

Sighted users have the advantage of seeing the link directly next to it's label, and also may have the advantage of recognising the dash with parenthesises as representing "remove this filter" functionality. Screenreader users will be forced to interpret "left parent dash right parent".

I understand not wanting to change the standard UI unnecessarily, which is why I thought the submitted patch might be a bit to verbose. I'll submit another one shortly which should meet accessibility, but not introduce too dramatic a change.

Dan

Everett Zufelt’s picture

My suggestion would be to.

1. Use a CSS background image for the (-) and then use a span with invisible text that matches the title attribute of the image.

E.g.

<a href="/search/site/mykeys" class="facetapi-active"><span class="element-invisible">Remove Article filter.</span></a>

Alternatively, you can keep the (-) in the markup, but outside of the span.

.element-invisible is a D7 system class to hide content visually, but to keep it available to screen-readers / DOM.

daniel.nitsche’s picture

Here's my second attempt. Visually, it should look the same, but should provide some helpful link text to screenreaders. The "visuallyhidden" class used in the last patch is actually an HTML5 boilerplate class, "element-invisible" is the default drupal one.

Sorry about the last patch not being from the root of the module directory -- it was my first submitted patch. This will also explain if there are any violations of the drupal coding standard in this patch -- I'm still working on getting an IDE sorted locally.

daniel.nitsche’s picture

You just beat me to it :)

Everett Zufelt’s picture

+  $accessible_link_text = '<span class="element-invisible">' . t('Remove @text filter', array('@text' => $variables['text'])) . '</span>';
+  $variables['options']['html'] = true;
+  $variables['text'] = $accessible_link_text . '(-) ';

Looking good. Two thoughts.

1. there should be a space at the end of the content within the span, or it will run into the (.

2. I think that the (-) should preceed the accessible text, at least in English. Can we throw (-) into the t() so that translations can place the (-) appropriately?

daniel.nitsche’s picture

  1. I avoided the space after the span as not to affect the layout. A space could easily be added before the closing span though.
  2. I decided to place the most meaningful text first, otherwise screenreader users will first hear 'parent dash parent'. It can certainly be placed into t() though.

I'll wait for Chris to comment on how he'd like to proceed.

cpliakas’s picture

Priority: Minor » Normal
Status: Needs work » Needs review

Thanks for the feedback and taking the time to help me better understand the issue. I think the approach mentioned in #8 is the way I would like to attack it, keeping the "(-)" in the markup but outside of the span. The patch in #11 looks good and I like the use of t(), I just want to make 100% sure we aren't introducing any security vulnerabilities by always setting the html key to TRUE when building the link. Other than that I think we are getting close. Bumping up the priority and changing status to "needs review".

cpliakas’s picture

Priority: Minor » Normal

The patch in #9 looks good to me, and the values are properly escaped. I think we should incorporate the recommendations made by Everett in #11, and then this issue will be RTBC in my book. Care to cut another patch Dan? Would like to get you in as the 9th committer.

Thanks guys,
Chris

cpliakas’s picture

Also, if you could add a // @see http://drupal.org/node/1316580 comment, that would be helpful for future reference.

cpliakas’s picture

Issue tags: +low hanging fruit

Bumping to the "low hanging fruit" list.

daniel.nitsche’s picture

Here you go. Everett's first point no longer applies, because I've taken his second point and moved the span to the end of the text.

Happy to do another patch if I've missed something.

Everett Zufelt’s picture

Haven't tested, but looks good.

cpliakas’s picture

Status: Needs work » Needs review

Thanks, Daniel!

I am on a mini-vacation and will test early next week, but this looks good to me.

Great work,
Chris

cpliakas’s picture

Status: Needs review » Reviewed & tested by the community

Patch looks great! Marking as RTBC. Great work Everett and Daniel.

cpliakas’s picture

Status: Reviewed & tested by the community » Fixed

Committed at 150f01b.

Congrats on becoming the 10th committer to Facet API! Also looks like this is your first credited commit to Drupal in general, so congrats on that as well.

~Chris

Status: Fixed » Closed (fixed)

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

cpliakas’s picture

Follow-up issue at #1368166: Pass facet deactivation link through t() tying up some of the loose ends.