WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 2 months ago

#21221 new enhancement

Image title and alt attribute content should be texturized.

Reported by: stephdau Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.4.1
Component: Media Keywords: dev-feedback has-patch
Focuses: accessibility Cc:

Description

gallery_shortcode() texturizes the caption shown underneath images in galleries.

For consistency, alt and title tags content should also be texturized.
This is also valuable for developers extending the gallery shortcode or output, such as with the WordPress.com (and Jetpack) Gallery Carousel feature, as it provides i18n'd texturization, for EG.

See attached patch, which:

  • uses wptexturize() in wp_get_attachment_image() directly (/wp-includes/media.php), which makes it work with gallries, attachment pages, etc.
  • also uses wptexturize() in get_image_tag() (/wp-includes/media.php), for consistency.
  • uses wptexturize() in wp_get_attachment_link() (/wp-includespost-template.php), for consistency

Attachments (5)

21221.diff (2.3 KB) - added by stephdau 3 years ago.
21221.2.diff (1.3 KB) - added by brianhogg 6 months ago.
patch refresh
unit_tests_21221.diff (1.1 KB) - added by welcher 3 months ago.
First pass unit test
21221.3.diff (2.5 KB) - added by adamsilverstein 3 months ago.
21221.4.diff (2.5 KB) - added by adamsilverstein 3 months ago.

Download all attachments as: .zip

Change History (20)

@stephdau3 years ago

comment:1 follow-up: @toscho3 years ago

  • Cc info@… added

Do not hard code the function, use a apply_filters() instead. See #19550.

comment:2 follow-up: @ericlewis12 months ago

  • Focuses accessibility added
  • Keywords reporter-feedback dev-feedback added; has-patch removed

Why is esc_attr() not enough already? What's the benefit of wp_texturize()?

comment:3 in reply to: ↑ 1 @adamsilverstein6 months ago

Replying to toscho:

Do not hard code the function, use a apply_filters() instead. See #19550.

Based on #19550 and r28715, wp_texturize already has a filter to allow you to turn it off entirely, that seems sufficient.

comment:4 in reply to: ↑ 2 @adamsilverstein6 months ago

Replying to ericlewis:

Why is esc_attr() not enough already? What's the benefit of wp_texturize()?

these do different things entirely - esc_attr is security related, stripping scripts, etc. wptexturize transforms text - "quotes to smart quotes, apostrophes, dashes, ellipses, the trademark symbol, and the multiplication symbol. "

I think would be nice to see this improved typography in alt and title tags, although I'm a bit wary of potential pitfalls or unexpected results.

@brianhogg is working on an updated patch here at #wcto. The current patch no longer applies cleanly.

@brianhogg6 months ago

patch refresh

comment:5 @wonderboymusic3 months ago

  • Keywords needs-unit-tests added; reporter-feedback dev-feedback removed
  • Milestone changed from Awaiting Review to Future Release

This should have tests to show what is not happening now and what should happen when the patch is applied.

@welcher3 months ago

First pass unit test

comment:6 follow-up: @welcher3 months ago

I've added some initial unit tests. They are my first core tests so any feedback is appreciated. I am re-using some of the data providers already in the class and if that approach makes sense, I'll go back and add the rest.

@adamsilverstein3 months ago

@adamsilverstein3 months ago

comment:7 in reply to: ↑ 6 @adamsilverstein3 months ago

  • Keywords dev-feedback added; needs-unit-tests removed
  • Milestone changed from Future Release to 4.2

Looks good @welcher, I haven't seen that @dataProvider technique before, excellent.

I verified tests failed before patch and pass after...

In 21221.4.diff I refreshed @brianhogg's patch and combined with your tests, plus made some tiny code standards fixes (missing space near closing bracket of unit test functions).


Replying to welcher:

I've added some initial unit tests. They are my first core tests so any feedback is appreciated. I am re-using some of the data providers already in the class and if that approach makes sense, I'll go back and add the rest.

comment:8 @DrewAPicture3 months ago

  • Keywords has-patch added

comment:9 @miqrogroove3 months ago

21221.4.diff seems safe, and I have some big picture questions about it:

  • Why does an alt attribute need to be texturized?
  • Why use an HTML parser in an attribute context?
  • Is it okay to not texturize title attributes when they contain angle braces?

comment:10 @joedolson3 months ago

I don't have answers for the second two questions; but for the first, the problem was for internationalization. Without texturizing, multibyte characters in attributes weren't expressed correctly.

comment:11 @miqrogroove3 months ago

wptexturize is an ASCII only search and replace algorithm. What does this have to do with multibyte characters?

comment:12 @joedolson3 months ago

Never mind then! I thought that was @stephdau's original intent from my prior review of this ticket, but if it's not a valid reason, then I'm not sure. I've never looked at this ticket in significant depth.

comment:13 @adamsilverstein3 months ago

@stephdau: can you clarify this part: it provides i18n'd texturization, for EG

comment:14 @slackbot2 months ago

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

comment:15 @DrewAPicture2 months ago

  • Milestone changed from 4.2 to Future Release

Looks like the feedback from @miqrogroove hasn't yet been fully addressed. Pushing to a future release.

Note: See TracTickets for help on using tickets.