WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 6 months ago

Last modified 5 months ago

#15926 closed defect (bug) (fixed)

Add alt attribute support for Custom Header functionality

Reported by: jane Owned by: SergeyBiryukov
Milestone: 4.1 Priority: normal
Severity: minor Version: 3.0
Component: Customize Keywords:
Focuses: accessibility Cc:

Description (last modified by joedolson)

Section 1194.22 Web-based Internet information and applications: (a) A text equivalent for every non-text element shall be provided (e.g., via "alt", "longdesc", or in element content).

To meet accessibility guidelines, users need to be able to give custom header images (when supported by a theme) alt attributes.

Attachments (10)

custom-header.php (22.1 KB) - added by firetag 4 years ago.
theme.php (49.3 KB) - added by firetag 4 years ago.
15926.patch (1.9 KB) - added by SergeyBiryukov 4 years ago.
15926.2.patch (33.2 KB) - added by firetag 4 years ago.
the patch
therightone.patch (34.3 KB) - added by firetag 4 years ago.
final.patch (3.6 KB) - added by firetag 4 years ago.
15926.diff (4.5 KB) - added by kovshenin 2 years ago.
15926.2.diff (4.5 KB) - added by kovshenin 2 years ago.
15926.3.diff (4.5 KB) - added by kovshenin 2 years ago.
Use esc_attr instead of esc_html when saving theme mod.
15926.4.diff (2.4 KB) - added by rianrietveld 6 months ago.

Download all attachments as: .zip

Change History (61)

comment:1 @GamajoTech4 years ago

  • Cc gary@… added

Good luck trying to add an alt attribute (not tag) to a background-image ;-)

+1 for the alt attribute on header image mod though.

comment:2 @lancewillett4 years ago

  • Cc lance@… added

Background images by their very nature are decorative and if they need text equivalents should not be background images.

Why does the header image alt attribute need to be user editable? The 508 compliance only requires that the text exists. Probably for the typical theme like Twenty Ten the image name would do just fine.

comment:3 @GamajoTech4 years ago

An image name does not necessarily convey what the contents of the image are.

The follow up to this ticket would also be to ensure that if the alt attribute is user-editable, that the title attribute is also user-editable, as users may not want to be forced to have the description appear as a tooltip.

comment:4 @zeo4 years ago

s/tags/attribute

comment:5 @nacin4 years ago

  • Milestone changed from Awaiting Review to Future Release

@firetag4 years ago

@firetag4 years ago

comment:6 @firetag4 years ago

  • Cc firetag added

Didn't exactly know how to submit a patch so I just uploaded the affected files. In wp-admin/custom-header.php it affected lines 209-213 and 501-512 and in wp-includes/theme.php it affected lines 1472-1492.

@SergeyBiryukov4 years ago

comment:7 @SergeyBiryukov4 years ago

Turned into a patch with a few changes. Note that preg_replace('/[^0-9a-zA-Z ]/', '', $_POST['alt-text']) doesn't allow non-English characters. I guess esc_html() can be used instead.

http://codex.wordpress.org/Reporting_Bugs#Patching_Bugs might be helpful.

comment:8 @garyc404 years ago

  • Keywords gci needs-patch added; 508-compliance removed

To firetag:

You're heading in the right direction :)

However, there are a few issues with your approach, so I need you to do some more work on this CGI task:

  • The function name (as well as theme_mod name) should be header_alt instead of background_alt. This is because background image is applied using css, which doesn't let themes utilize the alt attribute. Only header_image can have header_alt, in my opinion.
  • The form table row for Alternative Text should only be displayed when there's a header image defined (i.e. get_header_image() evaluates to true)
  • Also, one small enhancement, move check_admin_referer( 'custom-header-options', ... ) outside of the if blocks. Otherwise, the function could be executed repeatedly.
  • You should also modify Twenty Ten default theme to use header_alt.
  • As Sergey mentioned above, use esc_html() instead of preg_replace(). See Data Validation.

This time, try to make a patch. I don't think I reward you points if you can't submit a patch. It's part of the contribution process. Use either of these guides:

Once these issues are addressed, I guess the patch would be good enough to mark the GCI task as completed.

Version 0, edited 4 years ago by garyc40 (next)

comment:9 @SergeyBiryukov4 years ago

Ah, I should have checked the list on GCI. It's the second time I accidentally interfere in somebody else's task :)

Last edited 4 years ago by SergeyBiryukov (previous) (diff)

@firetag4 years ago

the patch

comment:10 @firetag4 years ago

Ok I uploaded the patch with all the changes you mentioned. Thanks for extending the deadline as well. I will not be home till late tonight if you could extend it further if it doesn't work that would be fantastic! Thanks.

@firetag4 years ago

comment:11 @firetag4 years ago

Ok nvm use the patch that says the right one I forgot to change the check admin referrer in the other patch. So it's therightone.patch Thanks.

comment:12 @garyc404 years ago

Replying to firetag:

It seems like you're making changes on version 3.0.4 of the files instead of current trunk. What this means is, your patch can't be applied to the bleeding edge version of WordPress (we're working on 3.1 now).

Please follow one of the guides I sent you above to set up your local SVN repository, and patch from there. It's a steep learning curve, I know. But you're only a few steps away from a finished patch. Hang in there!

I'm waiting for your patch.

Replying to SergeyBiryukov:

Ah, I should have checked the list on GCI. It's the second time I accidentally interfere in somebody else's task :)

No problem, I should have added a GCI keyword but I forgot :)

@firetag4 years ago

comment:13 follow-up: @firetag4 years ago

Ok I added what I beleive to be the final patch... it's called final.patch.. sorry it took long I was away all day.

comment:14 in reply to: ↑ 13 @garyc404 years ago

Replying to firetag:

Ok I added what I beleive to be the final patch... it's called final.patch.. sorry it took long I was away all day.

No problem. Patch works good :) Thanks!

comment:15 @garyc404 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

comment:16 @Anton Torvald3 years ago

Nothing.

Last edited 3 years ago by ocean90 (previous) (diff)

comment:17 @ElanTechnosys3 years ago

  • Owner set to ElanTechnosys
  • Status changed from new to accepted

comment:18 @ElanTechnosys3 years ago

  • Owner ElanTechnosys deleted
  • Status changed from accepted to assigned

comment:19 @wonderboymusic2 years ago

  • Keywords ux-feedback needs-refresh added; needs-testing removed
  • Milestone changed from Future Release to Awaiting Review

@kovshenin2 years ago

@kovshenin2 years ago

comment:20 follow-up: @kovshenin2 years ago

  • Cc kovshenin added
  • Keywords 3.2-early needs-refresh removed

Refreshed in 15926.2.diff, moved field to the Header Text area, updated all default themes, added sanitization, added the value to get_custom_header(). Both get_custom_header()->alt and get_theme_mod( 'header_alt_text' ); would be correct to use.

As opposed to 1.diff, 2.diff stores the escaped value. I don't really like the idea of storing the escaped value (as opposed to sanitized) but otherwise it would be vulnerable to XSS if theme developers forget to esc_attr when printing the value from get_theme_mod. Besides, blogname and other core options also store escaped values.

Thinking about a better approach to tackle themes. Perhaps use get_theme_mod instead, which will not trigger an undefined notice in earlier versions of WordPress but get_custom_header looks so much nicer.

Also, what if a theme does not implement the alt tags yet? Having that extra field do nothing in Appearance - Header seems like a waste, but doing current_theme_supports( 'header_alt_text' ); seems like too much. Thoughts?

comment:21 in reply to: ↑ 20 ; follow-up: @SergeyBiryukov2 years ago

Replying to kovshenin:

As opposed to 1.diff, 2.diff stores the escaped value.

Shouldn't it be escaped with esc_attr() rather than esc_html()?

comment:22 in reply to: ↑ 21 @kovshenin2 years ago

Replying to SergeyBiryukov: Good call! Not that they're different :)

@kovshenin2 years ago

Use esc_attr instead of esc_html when saving theme mod.

comment:23 @jlambe19 months ago

I'm testing the patch with latest svn 3.7 beta and using a child theme of Twenty Twelve. I saved an alternative text but there is no output of it in the alt="" attribute on the front-end.

comment:24 @jlambe19 months ago

Sorry my mistake. The .diff file was not correctly applied for some reasons. Works as expected !

comment:25 @grahamarmfield17 months ago

  • Cc graham.armfield@… added

comment:26 @ryno26715 months ago

alt TEXT. not tag. sorry; that bugs me :)
Carry on.

comment:27 @nacin15 months ago

  • Component changed from Accessibility to Appearance
  • Focuses accessibility added

comment:28 @ircbot7 months ago

This ticket was mentioned in IRC in #wordpress-ui by _Redd. View the logs.

comment:29 @ircbot7 months ago

This ticket was mentioned in IRC in #wordpress-ui by _Redd. View the logs.

comment:30 @davidakennedy7 months ago

  • Summary changed from Give header and background images alt tags to Add alt attribute support for Custom Header functionality

comment:31 @joedolson7 months ago

  • Description modified (diff)

comment:32 @joedolson7 months ago

  • Keywords needs-refresh added

comment:33 @ircbot7 months ago

This ticket was mentioned in IRC in #wordpress-ui by accessiblejoe. View the logs.

comment:34 @rianrietveld6 months ago

  • Keywords needs-testing added; needs-refresh removed

Why don't we solve the alt text like this:

The heading image is inside an <a> element linking to home_url( '/') (except for twenty ten).

Because the alt text of the header image is actually a link text this way, we could replace the empty alt="" into an alt="<?php bloginfo( 'name ' ); ?>.
Using an alt text with a description of what's on the images is not a good alt in this case.

For the themes:
Twenty fourteen: in header.php alt="<?php bloginfo( 'name ' ); ?>
Twenty thirteen: header image is a background image, so no alt text in needed
Twenty twelve: in header.php alt="<?php bloginfo( 'name ' ); ?>
Twenty eleven: in header.php alt="<?php bloginfo( 'name ' ); ?>
Twenty ten: here the image is not inside a link to the homepage, so a alt="" can be used here.

This way it works by default correctly.

Attached 15926.4.diff with the changes for Twenty fourteen, Twenty twelve and Twenty eleven.

Last edited 6 months ago by rianrietveld (previous) (diff)

@rianrietveld6 months ago

comment:35 @SergeyBiryukov6 months ago

  • Milestone changed from Awaiting Review to 4.1

comment:36 @kitchin6 months ago

I think you want

alt="<?php echo esc_attr( get_bloginfo( 'name', 'display' ) ); ?>"

based on what's already in twentyten/header.php and twentytwelve/header.php for title attributes.

comment:37 @SergeyBiryukov6 months ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from assigned to closed

In 29842:

Bundled themes: Add an alt attribute with the site title for header images linked to the home page.

props rianrietveld.
fixes #15926.

comment:38 @ircbot6 months ago

This ticket was mentioned in IRC in #wordpress-ui by RianRietveld. View the logs.

comment:39 @joedolson6 months ago

  • Keywords needs-patch needs-refresh added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

The patches provided by Rian are fine, as they are, but don't answer the question raised by this ticket. This ticket is about allowing an alt attribute to be set as part of the standard custom header functionality of the customizer, which that patch doesn't address.

comment:40 @SergeyBiryukov6 months ago

I've reviewed the previous patches that add the UI for setting the alt attribute, and patching the bundled themes appeared to be the only feasible solution here.

  • As 15926.3.diff shows, even if we add the UI to the Custom Header screen or the Customizer, it won't work out of the box if the theme does not support it, which would be confusing. We could probably add alt as a new argument for add_theme_support( 'custom-header' ) though, and only display the alt text input if the theme declares support for it.
  • comment:34 suggests that only header images linked to the home page should have a non-empty alt text, and the site title seems to be the best option in that case. Is there a use case for a different alt text?
Last edited 6 months ago by SergeyBiryukov (previous) (diff)

comment:41 @joedolson6 months ago

Comment 34 is referring to specific cases, so it's not reflecting the general case.

The general case is that any image that contains text or contributes content to the site must have an alt attribute. Any image that links anywhere must also have an alt attribute, regardless of whether the image itself contributes content.

In order to conform with ATAG guidelines (Authoring Tool Accessibility Guidelines), we need to provide the ability for authors to define alternate text for an image. There's obviously no way to do this and force it to work for all existing themes, but we can create the ability to add it in theme updates and future themes.

I think that adding it as a new argument to theme support for custom headers is a great solution.

Having a default alt attribute for linked images is fine, but it doesn't solve the general case.

comment:42 follow-up: @rianrietveld6 months ago

Agree that every image need an alt tag, even if it's alt="".
In the case of the Custom Header functionality with the themes 2014, 2012 and 2010 the alt text serves als link text. My fear is that, if we use the alt text entered with an image, with most websites the alt text will be something like IMG_123 or photo1 or such (as in my experience users hardly ever fill out the title or alt text, even if they are told why that it's important) .
Using the bloginfo( 'name ' ) makes it fool proof tmho.

Adding alt as an argument to add_theme_support( 'custom-header' ) would be a good thing, because that makes it easier for theme developers to add a different alt text to the header image / home link.
Is that worth a new ticket, or is that something to solve in this ticket?

Last edited 6 months ago by rianrietveld (previous) (diff)

comment:43 @ircbot6 months ago

This ticket was mentioned in IRC in #wordpress-ui by RianRietveld. View the logs.

comment:44 @sharonaustin6 months ago

As I understand it then, there are "two" functions that require attention:

  1. The link
  2. The description

Is it possible to handle the "link" component by adding aria-label in conjunction with handling the "description" by adding aria-describedby?

e.g. array('aria-label' => "link to home" , 'aria-describedby' => "$image-description" );

Source I consulted on use of aria-label versus aria-labeledby: http://www.w3.org/TR/wai-aria/states_and_properties#aria-label

comment:45 @rianrietveld6 months ago

#28861 was marked as a duplicate.

comment:46 in reply to: ↑ 42 @sharonaustin6 months ago

Replying to rianrietveld: Rian, I absolutely love what you're doing here.

Agree that every image need an alt tag, even if it's alt="".

As I alluded to in ticket #28861, I am as worried about an empty alt atribute as I am a bad one. As I understand it, empty alt attributes will cause some screen readers to remove focus from the image, which, in the case of the headers for certain themes, is also serving as a link home.
I don't know if the empty alt attribute over-rides the <a> element, or vice versa, when it comes to screen reader technology. And, many of us have learned that we should leave an alt attribute empty rather than put in descriptions for decorative images--which is why I was considering adding the ARIA features to it, so that if the alt attribute is handled incorrectly, users would understand that it is a link home.

Thanks to all for everything that's being done here.


comment:47 @ircbot6 months ago

This ticket was mentioned in IRC in #wordpress-dev by celloexpressions. View the logs.

comment:48 @celloexpressions6 months ago

  • Keywords gci needs-testing needs-refresh removed
  • Milestone changed from 4.1 to Future Release

Unfortunately, we need a patch for adding the ability to specify the alt text here, and it's likely to be fairly complex. Punting for now, work can continue here and if we get a patch before beta we can move this back to 4.1.

comment:49 @joedolson6 months ago

  • Keywords ux-feedback needs-patch removed
  • Resolution set to fixed
  • Status changed from reopened to closed

Rian and I discussed this at WCSF contributor day, and we agreed that this is an issue that can only practically be handled on a per-theme basis, since the solution significantly depends on whether or not the image is linked, and since the only solution to that would be to add another theme support declaration, this is functionally no different as a burden to the theme developer than just having appropriate header.php setup. Closing as fixed in bundled themes.

comment:50 @slackbot6 months ago

This ticket was mentioned in Slack in #core by johnbillion. View the logs.

comment:51 @SergeyBiryukov5 months ago

  • Milestone changed from Future Release to 4.1
Note: See TracTickets for help on using tickets.