In looking at this issue, I ran into a problem with core's forum:
http://drupal.org/node/790628#comment-3063574

In modules/forum/forum-icon.tpl.php

There are no alt tags defined in core:

    <?php print theme('image', array('path' => "misc/forum-$icon.png")) ?>

I believe these images to pass along meaning like: 'hot-new', 'hot', 'new', 'default', 'closed', & 'sticky' and I would like to see this conveyed in the alt text here.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
431 bytes

Here is a patch.

I think status-$icon would be better, what do you think?

Dries’s picture

Ideally the alt-tag would be a little bit more explanatory but this seems like a nice win already.

dawehner’s picture

What about "status-icon $icon" ?

Everett Zufelt’s picture

@Dereine

Thanks for working on this issue. The alt text should describe the icon and / or it's meaning. What is the icon communicating to those who can see it, what is it's purpose? Each icon should have a separate and meaningful alt description.

mgifford’s picture

Is there a list somewhere of what the valid $icon values are? We'd need to make sure that the case was correct. Something like "Status $icon" would be better I think.

However, starting with a list of available icons is a critical piece of this. What are they supposed to convey if anything?

dawehner’s picture

'hot-new' 'hot' 'new' 'default' 'closed' 'sticky'

This are the list of icons.

mgifford’s picture

Ok, so we'll want the alt tag to convey this.

- "Status: hot-new"
- "Post status: hot"
- "new"
- "Status icon: default."
- "Status-icon closed"
- "Closed post."

We've got options. I think how we want this to be phrased is going to be mostly in how to make this the most consistent with Drupal's language folks.

Do we have other examples of similar language to model this upon?

dawehner’s picture

I think this is quite confusing if there are many different kind of alt texts.

I don't know much about accessibility, but asume we have every icon on a page, would it be good to have 6 total different texts?

Everett Zufelt’s picture

Having multiple icons along with their alt on a page is not a problem for accessibility

dawehner’s picture

I meant the different kind of alt texts.

Everett Zufelt’s picture

Yes, I would just do 'New' 'Sticky', etc. perhaps 'New icon' 'Sticky icon', but it doesn't need to be complex

jcfiala’s picture

Hm. I'm not sure 'default' works as an alt for the default topic image - it doesn't seem to mean anything. I'd think something like 'Not new' or 'Read' would be a better choice.

However, testing this came across a bug in forum_node_presave().
EDIT: No, it wasn't a bug in forum_node_presave, it was a bug in devel_generate.

mgifford’s picture

How about:
'Hot-new post icon' 'Hot post icon' 'New post icon' 'Default post icon' 'Closed post icon' 'Sticky post icon'

I'd want to change the case in the $icon so that it was upper case using PHP's ucfirst() I think however.

Implementing this should be simple when we know what we want it to say.

mgifford’s picture

FileSize
662 bytes

Here's a patch to insert the icons.

Sadly, this approach only works with the posts themselves. The forums & container icons seem to be defined in CSS as background images.

Everett Zufelt’s picture

Title: Forum Icon alt tags » Forum icons not accessible to screen-reader users

It really is invalid to use the CSS background to display non-background images. Icons are not backgrounds. I recognize that this is likely done to make the images more easily swapped by themers. Perhaps we need to rethink this practice, which is present throughout core, for D8.

dawehner’s picture

+++ modules/forum/forum-icon.tpl.php	6 Jul 2010 16:45:35 -0000
@@ -18,7 +18,7 @@
+<?php print theme('image', array('path' => "misc/forum-$icon.png", 'alt' => t('!icon post icon.', array('!icon' => ucfirst($icon))))) ?>

Is there a reason why icon could be html?

Powered by Dreditor.

mgifford’s picture

@dereine - is there a reason that the icon could or couldn't be HTML?

I don't understand the comment.

Also, if we get the patch in #14 into core we'll have addressed one issue. The issue of css background issues to display non-background images should be another issue that we deal with separately.

Can someone review & RTBC what is there now or expand the patch to include CSS background images if need be?

bleen’s picture

Status: Needs review » Reviewed & tested by the community

#14 looks good.

...I agree with mgifford that the background css images should be made a sperate issue

mgifford’s picture

Damien Tournoud’s picture

Status: Reviewed & tested by the community » Needs work
-<?php print theme('image', array('path' => "misc/forum-$icon.png")) 

+ print theme('image', array('path' => "misc/forum-$icon.png", 'alt' => t('!icon post icon.', array('!icon' => ucfirst($icon)))))
?>

This makes the icon itself not translatable.

mgifford’s picture

Why? In my sandbox here it produces:

<img typeof="foaf:Image" src="http://drupal7.dev.openconcept.ca/misc/forum-new.png" alt="New post icon." width="24" height="24" />
Damien Tournoud’s picture

@mgifford: in this code:

t('!icon post icon.', array('!icon' => ucfirst($icon))

The $icon part cannot be translated. So it will appear the same in every langage. Sadly that's also the most important part.

mgifford’s picture

Ok, Damien can you cut through and tell me what you want to go here.

If not !icon what placeholder do you want for the $icon name?

dawehner’s picture

Status: Needs work » Needs review
FileSize
665 bytes

Here is a new patch

Damien Tournoud’s picture

FileSize
2.04 KB

Here is my suggestion.

mgifford’s picture

That works for me, thanks Damien!

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

I think this is the right approach to this Damien. I'm marking it RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Needs work
+      $variables['icon_alt'] = t('Hot topic, New comments');

should be:

+      $variables['icon_alt'] = t('Hot topic, new comments');
mgifford’s picture

Status: Needs work » Needs review
FileSize
2.04 KB

Sorry about that. Re-rolling the patch with the corrected case.

I've looked at it again to see if there was anything else I could catch that was out of place.

It is in place here - http://drupal7.dev.openconcept.ca/forum/3

andypost’s picture

Status: Needs review » Reviewed & tested by the community

+1, Still applies with offset and works

andypost’s picture

Jeff Burnz’s picture

Status: Reviewed & tested by the community » Needs work

If we're going to make changes here I favor our approach in #851496: Invalid to use the CSS background to display non-background images in Forum - it solves the accessibility issue, improves performance and is actually theme-able just with CSS without having to override a template just to change the icon (imo that is fundamentally wrong).

I respect the work that's been done here but from a designers perspective I will always punt for the option that makes theming easier.

Jacine’s picture

Ditto #32.

Michelle’s picture

Ok, #32 just took us in a complete circle from #19. LOL! I'm getting dizzy here.

I'm not a designer but I can say that switching to putting the images in CSS in Advanced Forum has been a big win for easier styling. I'm watching this discussion to see what core comes up with so I can match core if possible.

Michelle

Jeff Burnz’s picture

Status: Needs review » Needs work

@34 - Yes, the change in Advanced Forum is a big win and I cheered and did a little dance when I first heard about it.

I'm really in two minds about issue that is alluded to in #15, #16 and #19 - I'm not entirely sure the icon conveys any meaning that cant be deduced from the context already (unlike the topic icons which indeed add valuable contextual information and need to be proper labels). Everet can probably answer that with some authority - when navigating the container / forum table structure is it clear what is a container and what is a forum? If yes, then the issue is moot, if not, then we have a serious problem that goes somewhat beyond a small issue of background image verses image element.

andypost’s picture

Status: Needs work » Needs review
FileSize
1.91 KB
4.4 KB

New patch and new icons

- icons joined as they was 24x24 x 6 and processed with www.smushit.com as proposed in #552000: Smush core images
- changed css rules, I think icons should be the same size!!!
- added removal of old icons

@Jeff Burnz, about #851496-24: Invalid to use the CSS background to display non-background images in Forum
I think better user self-explanatory variable names so 'class' and 'status' proposed by you does not useful because:
- 'class' is a part of actual icon class
- 'status' - should be icon_status or forum_status, but this used in theme(forum_icon) so I think better icon_title

PS: glad to see any suggestions about theme variables

mgifford’s picture

It seems the theme folks are very interested in finding a nice way to extend the background images because it makes theming easier.

The crux of this comes down to these two lines of code (essentially):

http://drupal.org/node/851496#comment-3299796

<span class="element-invisible"><?php print t($icon) ?></span>

vs

http://drupal.org/node/821672#comment-3223360

<?php print theme('image', array('path' => "misc/forum-$icon.png", 'alt' => $icon_alt)) ?>

Using images with alt tags is more standardized, but I can't see where the problem would be for screen readers. They should read out the text which conveys the meaning of the icon that others would see simply enough.

Seems like it should be fine either way from an accessibility point.

andypost’s picture

Status: Needs work » Needs review

@mgifford, Please read issue which now marked as duplicate #851496: Invalid to use the CSS background to display non-background images in Forum

Also using t() in template file looks wrong print t($icon)
We have preprocess function for this purpose

Jeff Burnz’s picture

Oh good one - all icons the same size and looking much better - hooray! No bike-shedding from me on variable naming, looks fine.

For testers the forum-icons.png goes in /misc

Yes Mike, precisely right, from an accessibility point of view its the same, so the win/win for accessibility and themers is text image replacement. I'm not really sure about any standard way of doing things with icons - text image replacement has been around for many years and I personally have been using it for a long time.

mgifford’s picture

@andypost - ya I was monitoring that one too. I just wanted to highlight the different approaches taken by the two different threads.

@Jeff Burnz - ok, so we'll nix the alt tags approach & run with the text image replacement for this. Sounds like a step ahead for everyone.

The patch in #36 applies nicely. I've applied it here - http://drupal7.dev.openconcept.ca/forum

The forum icons are here:
http://drupal7.dev.openconcept.ca/misc/forum-icons_0.png

I'm not sure why there are still any references at all to http://drupal7.dev.openconcept.ca/misc/forum-default.png and why the patch doesn't remove all references to forum-*.png and all instances of them too.

I'd like to have instances where there are content types available for all, but haven't been able to get the forum icons to appear where I'd expect to see them so far.

andypost’s picture

Sprite should be linear to make icons load same time.
Also patch is changed for linear sprite and removed useless rule

+#forum .icon .topic-status-default {
+  background-position: 0 0;
+}

EDIT mgifford, please rename forum-icons_1.png to forum-icons.png
And yes, old icon files should be removed with a patch

mgifford’s picture

done and new patch applied too.

andypost’s picture

Status: Needs review » Needs work
FileSize
7.33 KB

Forums should be fixed too. Screen attached

Jeff Burnz’s picture

"Sprite should be linear to make icons load same time." Can you explain that more andypost.

andypost’s picture

Jeff Burnz, I tried to figure that moving images inline within sprite horizontally is better (filesize is less! a bit :) and when sprite-image is loading on a slow connection then all icons would displayed while loading because displaying and loading happens line by line)

About issue - I found that forum-default.png and forum-new.png is used for forums listing but last one never displayed because .new-topics class is never set in forum-list.tpl.php

Also listing of forums and topics looks very similar so we need to unify them!!!

Forums list have same troubles with accessibility as topic list

yoroy’s picture

Source files for the icons are here: http://drupal.org/node/102743#comment-668432
Then, in #377204: new forum icons display in IE6 there's a trick to make the 8bit alpha channels behave in IE6.

andypost’s picture

yoroy, Are you sure that same icons should be used for forums and topics?

Currently, both listings looks really ugly

yoroy’s picture

andypost: I didn't review the icons in context of actual usage, I thougth this was about making them a sprite alone. Is there a screenshot that illustrates the design issue?

andypost’s picture

FileSize
11.95 KB
15.9 KB

@yoroy, attaching forum list and topic list from bartik (default theme)
Also you could find how it looks in garland http://drupal7.dev.openconcept.ca/forum

I think forums should have a different icons to indicate "folders" for example
For forums we need 2 additional icons:
- normal forum
- forum with unread posts

Jeff Burnz’s picture

We had some fugly little folder icons in D6, which actually looked more like envelopes, folders sound good for forums, the bubble icons for topics - sounds good.

Michelle’s picture

Here's the AF ones. They're custom made by a user but inspired by the Crystal ones. (Those aren't GPL, so I couldn't use them directly)






Michelle

andypost’s picture

Michelle, core forum does not have "locked" status for forums, locked is applied only for topics :(

EDIT: Issue about locked forums #303368: Allow for locked forums

Michelle’s picture

@andypost: Yeah, AF doesn't use it natively, either. I just included it because it's nice to have a matching one for people who are using forum access. Whether you'd want to have it in core, I don't know, but whoever makes the core set may want to consider making one and putting it somewhere for future expansion.

Michelle

andypost’s picture

snetcher’s picture

FileSize
5.78 KB
andypost’s picture

Status: Needs work » Needs review

Looks good, Let's gather more opinions/reviews on this icons and finish this patch

Jeff Burnz’s picture

Definitely in the right style I think, not sure I like the way the are slightly rotated counter clockwise.

snetcher’s picture

FileSize
5.29 KB

agree, it should be better

Bojhan’s picture

Looking good its definitely in style there are a few things I would change, sorry I can't make a mockup.

Hope yoroy can chime in he created those initial ones.

@Michelle Thanks for your input although your icons are good looking, I am not sure they fit the visual style of Bartik or Garland.

snetcher’s picture

FileSize
4.69 KB
andypost’s picture

So anyone could review icons? I think we need this before beta

mgifford’s picture

I don't think just anyone can do it. I think you need a theme person, right?

Jacine’s picture

The code looks good to me, but one issue I'm seeing is that the <th> for this "status" column is empty when viewing the forum topic table. Seems like adding "status" there with the element-invisible class would be a nice improvement, but maybe that's for a separate issue?

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

@Jacine - ya that already a separate issue that's marked RTBC #821702: Replace Forum Icon Column with Topic Span

I'm going to mark this RTBC after that review, thanks!

Bojhan’s picture

Status: Reviewed & tested by the community » Needs review

Oh, wait - I dont know what images we are comitting now, but my suggestion obviously didnt turn out good - see #60. Maybe without gray and less skewed? I will look at it tonight.

yoroy’s picture

I was wondering if we even need to use the folder at all. A folder is such an overused metaphor. It's kind of correct in this context but you know, meh. Maybe we can get away with using only the status indicators themselves: star and lock and work on 'stand-alone' versions for 'hot' and 'sticky'.

andypost’s picture

@yoroy Do you think that forums should not have had own icons?
BTW no visual separation for forums and topics

Forums (taxonomy terms) have 2 icons - 1 for read and another for unread so we just need to add 2 icons

andypost’s picture

FileSize
16.27 KB

Here the screenshot of forum with 1 subforum and topics

mgifford’s picture

@Bojhan & @yoroy - what do we need to do to move this forward. I'm fine with nixing the images, but if they are going to be there they need to be done properly.

What's outstanding to move this back to RTBC?

Jeff Burnz’s picture

The patch in #41 is a good approach so its just a matter of deciding on icons - the ones in #60 look pretty good to me. I'd say we roll up a patch with those and give it a test - see how they go with the core themes and the other icons etc.

mgifford’s picture

@snetcher can you splice out the images in #60 into a format for a linear sprite as defined in the patch for #41 and we can put this on my sandbox, test with core themes and work on getting this to RTBC.

webchick’s picture

Wait. Wait. WAIT.

Why on earth are we talking about switching forum icons 9 months after UX freeze?
Why isn't this a 3-line or so patch to add alt tags to the images here?

Jeff Burnz’s picture

Good point webchick, the patch in #41 is good as it is (doesn't change any icons).

The alt attribute fix in #29 is one approach to the issue, the CSS background/image replacement patch in #41 is another solution to the issue and has the added bonus of making to easy to change forum icons (easier to theme) and uses a sprite so reduces HTTP requests. The patch in 41 uses the existing forum icons and just places them all in one sprite.

mgifford’s picture

mgifford’s picture

Since sprites are all the rage these days, it's probably worth running with #41.

@webchick thanks for stepping in and bringing this back. Good to review the icons for D8, but.

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

I've applied the patch in #41 here and it works well: http://drupal7.dev.openconcept.ca/forum

Hopefully this helps move this along.

sun’s picture

This actually looks like a nice thing to do. Since no one really cares for Forum module in core, we should be grateful for any kind of improvement, especially when it comes to visual improvements.

That said, I can't think of any way how this patch could break existing sites or contributed modules (other than Advanced Forum itself, but Michelle participated here). So I don't see why we shouldn't commit this patch.

Michelle’s picture

@sun: I'll adapt AF as needed to changes in core so no worries there. :) AF does its own thing with icons, anyway, so probably wouldn't be affected.

Michelle

sun’s picture

Version: 7.x-dev » 8.x-dev

Although badly needed, this is D8 material according to the rules (I had to learn today). It may be backported at a later point in time (though that's unlikely).

Bojhan’s picture

Version: 8.x-dev » 7.x-dev

Bugs aren't D8?

Everett Zufelt’s picture

@Sun

Do not bump accessibility issues to D8 without clear documentation of 'the rules'.

mgifford’s picture

This is a patch which is ready to go essentially since the beginning of August. Sure, it's October now and everyone wants Drupal 7 out the door, but the code is very simple & everyone is in agreement that it would be an improvement. Why not just put it in? It doesn't change the UI, just enhances the accessibility.

Jeff Burnz’s picture

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed the patch in #41. Fixes a usability bug and should improve performance. Thanks all.

Jeff Burnz’s picture

Holy crap, this is almost too good to be true, thank-you so much Dries, you just made forums sane to theme for the first time since I don't know when.

Status: Fixed » Closed (fixed)
Issue tags: -Accessibility

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