Who's new & Who's online blocks have empty headings

The failure is in theme_item_list():

    if ($title !== '') {
      $title = '<h3>' . $title . '</h3>';
    }

This lets through a $title of NULL. Anyone with a better grasp of PHP's empty/null/isset malarkey care to tackle this? :)

Assuming that we never want an unset, NULL, or '' / 0 / '0' (empty) $title, we should use:

if (!empty($title)) { ... }

Assuming that we want to allow '0' or 0, we want:

// $title is set, is not NULL, and is not the empty string ''.
if (isset($title) && $title !== '') { ... }

Assuming that we want to allow '0', but not 0 (which evaluates to the empty string '')

// $title is set, is not NULL, and does not evaluate to the empty string ''.
if (isset($title) && $title != '') { ... }

My guess is that we want the third example.

Hmmm,

I'm wrong, '0' evaluates to '' too.

Heh, read it and weep. :) http://php.net/manual/en/types.comparisons.php <3 PHP

Bottom line, do we want $title = '0' or 0 to be printed as 0, or ignored?

The attached patch will print $title if it does not evaluate to the empty string. So, $title will not be printed if $title=='0'. I think we can live with that here, I don't believe that we have a Coding Standard for rendering empty strings.

Can anyone explain how to replicate this condition? I would test the patch, but I can't get it to NOT work in the first place.

mgifford: I've got a site running the 8.x HEAD and added both blocks to the sidebar. Both as an admin and anonymous user, I'm seeing the title on both blocks. I tried overriding the block titles to 0 or empty string, and it worked as expected (no h2 tags) in both cases.

> Bottom line, do we want $title = '0' or 0 to be printed as 0, or ignored?

I suspect that if we don't allow a block title consisting of a single '0', someone down the line will file a bug because that's what they need...

Reproduced the problem and got two empty heading errors (accessibility errors). Applied the patch and Voila! got no errors. Please see screenshots after patch was applied. I think it can be marked as RTBC.

I have been doing some accessibility work so picked this patch to test.

Sure @mgifford. Please let me know if in anyway I can help out.

Sorry I'm confused, 0 should not be equivalent to '' with a strict comparison, which is what the existing code has. So why can't we just check for isset and the strict check as suggested in #5?

// $title is set, is not NULL, and is not the empty string ''.
if (isset($title) && $title !== '') { ... }

Regardless, there should be a code comment explaining the accessibility issue.

Based on Catch's comment #15. In this patch checking to see if title is set and its not null and empty string. Also added accessibility comment.

Trying again with the right patch.

Thanks @kgoel! Setting NR for the bot to test it.

updated description :)

Ah, I think that is better comment text. Thanks @mgifford and @kgoel!

Attached cleans up the comment a bit more. RTBCing nonetheless because the changes from #20 are trivial.

Thanks! Committed/pushed to 8.x.

This should probably be backported.

D7 patch

Changed version in the issue from 8.x to 7.x for backport.

#23: 1349722-23.patch queued for re-testing.

Fixed the commenting indenting. Updated patch for D7

Thanks @kgoel. The backport looks pretty good but needs one small change:

+++ b/includes/theme.incundefined
@@ -1967,8 +1967,11 @@ function theme_item_list($variables) {
+// Only output the list container and title, if there are any list items.
+// Check to see whether the block title exists before adding a header.
+// Empty headers are not semantic and present accessibility challenges.

These comments should be indented by two spaces.

Thanks xjm! Learning something with each patch. Updated one for D7

That was fast! Setting "Needs review" so it will be tested. :)

Oh whoops, looks like I was posting at the same time as you! The indentation in #29 is correct--I was referring to the earlier version. :)

So RTBC for #29 (not #31).

Thanks xjm!

Committed and pushed #29 to 7.x. Thanks!