WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 3 months ago

#15926 assigned defect (bug)

Give header and background images alt tags

Reported by: jane Owned by:
Milestone: Awaiting Review Priority: normal
Severity: minor Version: 3.0
Component: Appearance Keywords: gci has-patch ux-feedback
Focuses: accessibility Cc:

Description

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, the header images and backgrounds (when supported by a theme) need to be given user-editable alt tags.

Attachments (9)

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

Download all attachments as: .zip

Change History (36)

comment:1 GamajoTech3 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 lancewillett3 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 GamajoTech3 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 zeo3 years ago

s/tags/attribute

comment:5 nacin3 years ago

  • Milestone changed from Awaiting Review to Future Release

firetag3 years ago

firetag3 years ago

comment:6 firetag3 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.

SergeyBiryukov3 years ago

comment:7 SergeyBiryukov3 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 garyc403 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 GCI 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 can 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.

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

comment:9 SergeyBiryukov3 years ago

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

Version 0, edited 3 years ago by SergeyBiryukov (next)

firetag3 years ago

the patch

comment:10 firetag3 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.

firetag3 years ago

comment:11 firetag3 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 garyc403 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 :)

firetag3 years ago

comment:13 follow-up: firetag3 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 garyc403 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 garyc403 years ago

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

comment:16 Anton Torvald2 years ago

Nothing.

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

comment:17 ElanTechnosys2 years ago

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

comment:18 ElanTechnosys2 years ago

  • Owner ElanTechnosys deleted
  • Status changed from accepted to assigned

comment:19 wonderboymusic15 months ago

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

kovshenin15 months ago

kovshenin15 months ago

comment:20 follow-up: kovshenin15 months 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: SergeyBiryukov15 months 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 kovshenin15 months ago

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

kovshenin15 months ago

Use esc_attr instead of esc_html when saving theme mod.

comment:23 jlambe6 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 jlambe6 months ago

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

comment:25 grahamarmfield4 months ago

  • Cc graham.armfield@… added

comment:26 ryno2673 months ago

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

comment:27 nacin3 months ago

  • Component changed from Accessibility to Appearance
  • Focuses accessibility added
Note: See TracTickets for help on using tickets.