Merge lp:~adeuring/launchpad/bug-513382 into lp:launchpad

This is a fix for bug 513382: grey (gray) heat icons are too bold.

I changed the file lib/canonical/launchpad/images/flame-bw-icon.png so that it contains an icon as provided in an atachment for bug 513382.

No tests.

No lint.

To see the changes, compare http://people.canonical.com/~adeuring/bug-heat-icons.png with the icons show for example on https://bugs.edge.launchpad.net/malone/+bugs

Looks obviously grayed out now, thanks Abel.

Thanks for taking on this work, Abel, but this branch needs a bit more work to be ready.

The bug, which is actually bug 513380, has images linked. The color icon has also been updated there as well. These images were prepared by Martin A. to enable us to use a single image for each degree of heat. So the images attached to the bug should be used.

However, we need to add these to the sprites file so that they can be used in that way, rather than hard coding image tags. So some work will need to change the view function that generates the images or else changes to enable a formatter for heat icons.

Cheers,
deryck

15:37 < noodles775> adeuring: I think you've got the wrong bug linked on your MP ;) ui=me, thanks!
15:42 < adeuring> noodles775: thanks! and right, the bug is 513380...

Sorry, I should have checked the correct bug before ui-approving it.

OK, the five different images showing various combinations of red and grey heat icon are now in use.

I did not add the images to the huge sprite image because the images themselves carry information that is not represented anywhere else. This information would be lost if the heat icons are only used as background images.

I've added alt and title attributes, fixed one unrelated lint complaint (line too long) and removed a trailing whitespace.

A test of the bug heat view class needed some changes, as well as a page test helper function.

tests:

./bin/test -t bug-heat-view.txt
./bin/test -t xx-listing-basics.txt

no lint

I don't understand the second paragraph above where you explain why the images can't be in the sprite. Can you elaborate?

If indeed the images must be discrete then the code looks fine.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Brad,

On 23.02.2010 21:12, Brad Crittenden wrote:
> Review: Needs Information code
> I don't understand the second paragraph above where you explain why the images can't be in the sprite. Can you elaborate?
>
> If indeed the images must be discrete then the code looks fine.

The images carry unique information, the heat level: There is no other
content on the page that says "heat level 3". So, in order to adhere to
accessibility rules, we should provide an altternative text, like the
alt attribute of an <img> tag. Sprites on the other hand can be used
only for background images, as far as I understand it, and since
background images are purely "decorative", there is no way to associate
textual information with them.

Also, we want a title attribute where we show some more or less
interesting additional information, see bug 513219. IOW, a second reason
not to use a background image.

Abel
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFLhFRCekBPhm8NrtARAjVrAJ0ZOQDHD/RaRHyys+43kDe0053L9QCePMAf
KPT3EAvUBCkw0Zal4dePxDo=
=0xDU
-----END PGP SIGNATURE-----

> OK, the five different images showing various combinations of red and grey
> heat icon are now in use.
>
> I did not add the images to the huge sprite image because the images
> themselves carry information that is not represented anywhere else. This
> information would be lost if the heat icons are only used as background
> images.

I assume that the "information that is not represented anywhere else" is the TITLE and ALT attributes for <img>. The element with the sprite class could also have the TITLE set. Is the ALT really that important, or is there something else that I'm missing?

Hi Abel,

Sorry if I'm stepping into the code-review, but here are a few thoughts for implementation.

Just in case you haven't seen it, you can find out more details about sprites at: https://dev.launchpad.net/UI/CssSprites . As Edwin mentioned, you should be able to use something like:

<span class="sprite person" title="whatever"></span> (generating it from TAL of course),

but given that this will be used in multiple places, I think it would be worthwhile creating your own formatter, similar to the icon formatter that you've already got for bugs (in c/l/webapp/tales.py), so you could do something like:

<span tal:replace="structure bug/fmt:heat"></span>

That would allow you to generate the title too. We could even give the span textual content (ie. 'Heat level 4' or something more appropriate) for accessibility and hide it via the CSS, if you're keen.

What do you think?

@Edwin, are there instructions somewhere for using the sprite_image make target? (other than reading through bin/sprite-util ;) ). Can you please update the above sprite wiki page with a link to some instructions? Thanks!

Hi Edwin, hi Michael,

My reason to use the plain <img> tag is accessibility. See for example http://www.w3.org/TR/WCAG20/#text-equiv : "Non-text Content: All non-text content that is presented to the user has a text alternative that serves the equivalent purpose, except for the situations listed below. (Level A)"

<span class="sprite person" title="whatever"></span> is perhaps an alternative, but the main idea of the <img> tag's alt attribute is to provide a precise description of what is displayed ("3 out of 4 heat flames" in this case, which is quite precise).

The title attribute, on the other hand, should "offer advisory information about the element for which it is set" (http://www.w3.org/TR/1999/REC-html401-19991224/struct/global.html#adef-title), which is slightly different.

Regarding accessibility, textual content that is hidden via CSS does not work at all as a an alternative for <img alt="...">: Screen readers and similar technology are supposed to present the _visible_ content in some other form, but not the invisible content.

So I think we should keep the <img> tag in this case.

Hi Abel,

OK, I'll butt-back-out of the implementation discussion ;), but just to address the points you raised:

> Hi Edwin, hi Michael,
>
> My reason to use the plain <img> tag is accessibility. See for example
> http://www.w3.org/TR/WCAG20/#text-equiv : "Non-text Content: All non-text
> content that is presented to the user has a text alternative that serves the
> equivalent purpose, except for the situations listed below. (Level A)"

Yes, but I'm thinking of this around the other way: the text content (within the span produced by the formatter) would be "3 out of 4 heat flames", we'd just hide it in an accessible way (see below).

>
> <span class="sprite person" title="whatever"></span> is perhaps an
> alternative, but the main idea of the <img> tag's alt attribute is to provide
> a precise description of what is displayed ("3 out of 4 heat flames" in this
> case, which is quite precise).
>
> The title attribute, on the other hand, should "offer advisory information
> about the element for which it is set" (http://www.w3.org/TR/1999/REC-
> html401-19991224/struct/global.html#adef-title), which is slightly different.

Yes, the title can be read by screen-readers (like JAWS), but afaics, it's not a default option. So you're right - it's not an alternative for an alt attribute on an image.

>
> Regarding accessibility, textual content that is hidden via CSS does not work
> at all as a an alternative for <img alt="...">: Screen readers and similar
> technology are supposed to present the _visible_ content in some other form,
> but not the invisible content.

That's not really true. I mean, it is if you use visibility:hidden in your css, but I've always positioned the hidden text absolutely off the screen. Like this:

<span class="sprite heat heat4" title="3 out of 4 heat flames"><em>3 out of 4 heat flames</em></span>

span.heat em {
  position: absolute;
  left: -1000px;
  }

or similar. More info on accessible invisible content at: http://www.webaim.org/techniques/css/invisiblecontent/

>
> So I think we should keep the <img> tag in this case.

I'll leave it up to you guys to decide on the code/implementation. Just let me know when it's settled and i'll run the branch again locally for the UI review.

Hi Michael,

you are right, I meant that a "visibility:hidden" text is not a good alternative for the alt attribute of <img>. Positioning the text off screen avoids this, but I'd argue that we exploit a bug of screen readers in this case ;)

While implementing a variant of the heat icon display that uses

<span class="sprite heat heat0" title="Heat: 0"><em>0 out of 4 heat flames</em></span>

and hides the text via the CSS rule "position: absolute; left: -1000px;" I noticed that Firefox allows to search for this hidden text.

That makes searches for the words "heat" or "flame" somehwat confusing, if not useless on longer bug listings...

to reproduce: use revision 10375 of this branch, open https://bugs.launchpad.dev/ubuntu/+bugs in Firefox, start a full text search in firefox for the word "heat", and count how often you must hit F3 until Firefox marks the column header "Heat" as the "current search position"

Hi Abel,

Yep, but I'm not sure how important that is? I'll let you guys decide that. Also, regarding searching/keyboard navigation, I would recommend going with the full solution at:

http://www.webaim.org/techniques/css/invisiblecontent/

rather than my simplified example. You'll see there that they identify:

"A previous version of this article recommended using left:0px; top:-500px. While this worked by positioning the content above the top of the page, we've since discovered that if the hidden element contained a link or form element, that upon receiving keyboard focus, the browser would attempt to scroll to the element - thus scrolling the browser to the top of the page. This could result in confusion for sighted keyboard users. By positioning directly to the left, the browser will not scroll to the top of the page."

And finally, I think it's worth bringing this up at the next UI meeting to discuss (on Monday)... others might disagree with me :)

Hi Michael,

Perhaps I am a bit thick, but I don't see how the problem mentioned there and implementing all CSS rules affects the confusing search behaviour.

And agreed, not being able to efficiently search for two words is not a big problem -- but it is a problem. (And a funny side effect is that browser searches for a word in the title of the UI bug 513380 I'm fixing will become useless by the fix of the bug -- IOW, we are knowingly introducing another UI bug...)

I short: I understand that using CSS sprites is quite successful to reduce page load times, probably also to reduce the server load and whatever else -- but using sprites by all means for all images where is it technically possible is wrong. If we have an image that is the "main or only carrier of information", we should keep the <img> tag, together with alt and title attributes. Trying anything else is likely to have some kind of weird or annoying side effect.

But anyway, I'll keep the sprite implementation for the heat icons for now...

Though I haven't commented on this review lately I've been keeping up with the discussion. Based on Abel's latest discovery that the work-around will cause the psuedo-ALT text to be included in our site-wide search results I'd argue for reverting the CSS changes and going back to the original <IMG> approach so this change can get landed. It seems we need to explore a general solution to this problem, starting with the UI call on Monday, and once that is done we can retrofit this change.

I vote 'Approve' on the original version.

Abel please save the work you've done as it will be a good starting point for the broader discussion.

I agree with bac completely. I think we should just use img tags and let the interested parties debate a fix for this on the UI call.

I'm marking Approve for the img implementation using beuno's images as well.

Michael, or you ok to review the end-result of the UI -- i.e. that the images look right on the site -- and defer implementation details to code review and a UI call discussion?

> Michael, or you ok to review the end-result of the UI -- i.e. that the images
> look right on the site -- and defer implementation details to code review and
> a UI call discussion?

Yep, the UI looks great (as it did at the start :) ), and needs to land!

I *do* think you should check with Curtis, as it seems that we already have a standard way to do this (with the discussed pros/cons already evaluated), and it's pretty much what Abel has implemented with r10375 (it only needs to be updated to use the span.invisible that's already there for this exact reason). But again, that's up to you guys.