As discussed here #821672: Forum icons not accessible to screen-reader users CSS background images should not be used instead of real images as they pose accessibility problems. You cannot assign alt text to a CSS background image to display the meaning that it conveys.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Not really. It's not a question of using the <img> tag or not, but of the presence or absence of a tag that has a text alternative.

Generally, we should use *more* CSS background images and *less* <img> tags, because the former can be easily overridden in the theme while the latter cannot. Plus:

<span class="hot">Hot topic</span>

is much more elegant and semantically powerful then:

<img class="hot" src="images/hot.png" alt="Hot topic"/>

The markup should make minimal assumptions about how the elements will be displayed to the user (sighted or not).

In the case of the forum containers, the only meaning the icon convey is "I'm a forum container". Not sure there is a need to pass this information to the non-sighted users. But it is needed, we should add an invisible label before the name of the forum container.

Damien Tournoud’s picture

By invisible label, I mean a:

<span class="element-invisible">Forum container:</span>
Everett Zufelt’s picture

@damien

Isn't what you are proposing the opposite of semantic markup?

mgifford’s picture

@damien with so much of Drupal core being accessible via theme over-rides I don't know that having the images in CSS makes it that much easier.

In the case of the forums there is inconsistency that would be really annoying to have to deal with. There is an actual image or a css background in the forum to express the same image. Inconsistency is a problem.

I also think that going through and adding <span class="element-invisible">Hot topic</span> everywhere that images/hot.png is going to be a real hassle too.

I do think it's much more consistent to just add an image with proper alt tags. It's much more common for elements like this.

Jeff Burnz’s picture

Well, hmmm, from my perspective as themer I like damien's proposal, sort of, I would use text-image-replacement rather than hiding it with element-invisible and probably use the strong or em element and get rid of the image element altogether. This means we could have a sprite to replace the 6 or so images used by Forum module which is better performance also.

This is basically as simple as <strong class="<?php print $icon ?>"><?php print t($icon) ?></strong> in forum-icon.tpl.php and a bit more CSS and combine the seperate images into a sprite - 5 minutes work guys.

Damien Tournoud’s picture

Isn't what you are proposing the opposite of semantic markup?

@Everett Zufelt: no, on the contrary. The <img> tag should only be used for actually displaying an image or photography in a page (like in a catalog or gallery). It should not be used for decorative elements, nor for elements that convey meaning by themselves (like our forum icons).

For those elements, we need to proper (non-<img>) markup associated with a form of text-to-image replacement.

lotyrin’s picture

I agree with #6 completely.

We have the idea of something being a "Hot" topic. This should be displayed to both sighted and non-sighted users in the most semantic and simple way, which would be a text label.

If we want to style this text by replacing it with an image, that's great. Those styles belong in CSS.

mgifford’s picture

FileSize
60.7 KB

Ok.. Let's look at a specific example here. Here's a demo forum http://drupal7.dev.openconcept.ca/forum/3

Forums contain no images:

      <tr id="forum-list-7" class="odd">
      <td class="forum">
                          <div class="name"><a href="/forum/7">kecac</a></div>

                      <div class="description">description of kecac</div>
                        </td>
              <td class="topics">
          0                  </td>
        <td class="posts">0</td>
        <td class="last-reply">  n/a</td>

          </tr>

But Topics do:

      <tr class="odd">
      <td class="icon">
<img typeof="foaf:Image" src="http://drupal7.dev.openconcept.ca/misc/forum-default.png" alt="" width="24" height="24" />
</td>
      <td class="title">
        <div>
          <a href="/node/10">Illum Commoveo Neo Quis</a>        </div>

        <div>
            <span class="submitted">
  By <a href="/user/1" title="View user profile." class="username" xml:lang="" about="/user/1" typeof="sioc:UserAccount" property="foaf:name">swirowashos</a> 1 week 6 days ago  </span>
        </div>
      </td>
          <td class="replies">
        1              </td>

      <td class="last-reply">  <span class="submitted">
  By <a href="/user/1" title="View user profile." class="username" xml:lang="" about="/user/1" typeof="sioc:UserAccount" property="foaf:name">openconcept</a> 30 sec ago  </span>
</td>
        </tr>

Visually they contain the same information... however, there is no indication of the image or any meaning it may represent in the Forum example posted above. It's fine in the topics.

I don't think that the use of:

  <strong class="<?php print $icon ?>"><?php print t($icon) ?></strong>

Would produce any disadvantages for accessibility. However, if we've got examples of the text-image-replacement technique we can test it out and determine if there is a problem.

Consistency is the first goal however.

Jacine’s picture

I agree with Damien. I also think a <span> is better than <strong> in this case.

andypost’s picture

+1 to span with a bit of css, also do not forget to set title attribute make hints visible

Jeff Burnz’s picture

Status: Active » Needs review
FileSize
1.84 KB
2.38 KB
24.04 KB

OK, so heres a rudimentary patch to get the ball rolling and allow for testing.

This patch:

  1. Removes the img element and replaces it with a real word - e.g. for "default" forum topics the word "Default" appears in the markup.
  2. Modifies template_preprocess_forum_icon() to use natural language words and phrases, e.g. "Hot new" instead of "hot-new".
  3. Adds markup to forum-icon.tpl.php to support the new text-image-replacement technique used to replace the words with icons - a DIV wrapper to hang the icon on and a SPAN wrapper to hide the text.
  4. Users drupal_html_class to properly format the class for use on the wrapper DIV.
  5. Uses the existing element-invisible class on the SPAN to hide the text because we know from extensive testing this is accessible.
  6. Merges the icons into one sprite (8kb saving!)

The TIR technique is basically brand new and leverages our existing element-invisible class, I suspect its based (very) loosely on the old Gilder/Levin method, but doesn't attempt to cover the text (well it does, except the text is now totally hidden). This method will survive the "CSS Off" test but not the "Images Off" test (visually in any case, of course it will still be accessible).

There's some pretty big advantages to using text-image replacement - for one users can override template_preprocess_forum_icon() and set whatever natural language phrases they like - we could push this one better and extend the function to separate the class from the phrase, in which case we could remove the need for drupal_html_class(). Obviously theres a performance saving here also - less http requests and overall 8kb saved in image weight.

The new forum-icons.png image goes in /misc (note the patch does not attempt to remove the original images).

text-image-replaced-topic-icons.png

Jeff Burnz’s picture

Ops, attached the wrong patch - this is the right one!

Jeff Burnz’s picture

Just to clarify heres a couple of samples of the HTML output:

<td class="icon">
  <div class="topic-status-new">
    <a id="new"><span class="element-invisible">New</span></a>
  </div>
</td>

<td class="icon">
  <div class="topic-status-sticky">
    <span class="element-invisible">Sticky</span>
  </div>
</td>

Status: Needs review » Needs work

The last submitted patch, text-image-replace-topic-icons_851496_v2.patch, failed testing.

Michelle’s picture

I normally never "subscribe" to anything but I'm making an exception since Advanced Forum uses CSS background images for this and does it differently and I want to see what goes into core and update AF accordingly. So my apologies to everyone I just did a useless bump on. :(

Michelle

Jeff Burnz’s picture

Status: Needs work » Needs review
FileSize
2.95 KB

re-roll, project space hiccup... give me a break bot :)

andypost’s picture

FileSize
3.32 KB

Jeff Burnz, great work! This looks exactly as #821672: Forum icons not accessible to screen-reader users
Except this one cares about css.

I think we need TITLE attribute because icons could tell nothing to user but hovering above icon user see a hint.

I incorporate descriptive/translatable titles into your patch from #821672

Once icon is not image and never was...
I changed $icon => $icon_class as it is and add $icon_title seems more descriptive as $icon_alt

andypost’s picture

Fixed css rules - no reason in TD.ICON

-#forum td.icon .topic-status-closed
+#forum .icon .topic-status-closed
Damien Tournoud’s picture

This looks pretty good to me.

Damien Tournoud’s picture

Just one thing: the quality of the images in #11 seems very poor.

Jeff Burnz’s picture

@20, yeah, the images are very poor - the sprite needs improvement, not sure what the hell I was doing with Photoshop but that particular sprite came out real bad, be good if someone has the original PSD for those icons.

andypost’s picture

Suppose patch should include removal of old icons

Also I think we need mark as duplicate #821672: Forum icons not accessible to screen-reader users

andypost’s picture

Jeff Burnz’s picture

This is looking good, the only suggestion I might make remove some use of the word "icon", for example, its not really the icon_title - nor is it the topic title, but more the "topic status", so I would probably go with something more like

  $variables['class'] = $variables['new_posts'] ? 'hot-new' : 'hot';
  $variables['status'] = $variables['new_posts'] ? t('Hot topic, new comments') : t('Hot topic');

Otherwise the changes are great, good stuff.

aspilicious’s picture

If this ever gets committed the icons have to be re-rendered, they are kinda bad on the outside

NaheemSays’s picture

why not do this but without the tiled icons?

That is just a space optimisation (that may or may not exist once the tiles are rerendered to look good in all situations).

Jeff Burnz’s picture

Guys, I did not realise this is being discussed over in this issue #821672: Forum icons not accessible to screen-reader users, so given this has become mostly about forum icons I'm thinking we can close this or mark as duplicate and move the patch and discussion over there.

andypost’s picture

Status: Needs review » Closed (duplicate)

Let's continue in original issue #821672: Forum icons not accessible to screen-reader users

I'm going to re-roll patch but we still need a hq icons sprite

I've ask yoroy via contact form to post a source file of icons to aggregate them into sprites. So let's wait when he does this.

mgifford’s picture

Version: 7.x-dev » 8.x-dev
Status: Closed (duplicate) » Active

I'm re-opening this general issue as the forum isn't the only place that uses this.

Noting #1982236: Markup for: theme_feed_icon()

mgifford’s picture

Issue tags: +sprite

tagging

mgifford’s picture

Status: Active » Needs review
Issue tags: -Accessibility, -sprite

Status: Needs review » Needs work
Issue tags: +Accessibility, +sprite

The last submitted patch, 821672-forum-icon-remove-old-d7.patch, failed testing.

mgifford’s picture

Never mind the patch retest here.. Sorry..

BarisW’s picture

Status: Needs work » Needs review

This seems to be already fixed in D8, right?

<?php
<span class="visually-hidden">{{ icon_title }}</span>
?>
mgifford’s picture

This would work, it's a matter of whether it is consistently applied or not. I re-opened this based on theme_feed_icon(). It was fixed in the forum, but we don't have a generalized (and documented) solution for dealing with non-background issues in an accessible way just yet.

mgifford’s picture

Title: Invalid to use the CSS background to display non-background images » Invalid to use the CSS background to display non-background images in Forum
Issue summary: View changes
Status: Needs review » Closed (fixed)

I'm going to close this. It's just adding confusion and I should have just opened a new issue instead.