Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#36735 closed defect (bug) (fixed)

Add media, insert from URL and alt attribute

Reported by: afercia's profile afercia Owned by: joemcgill's profile joemcgill
Milestone: 4.6 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch commit
Focuses: accessibility Cc:

Description

When inserting an image using "Insert from URL" in the media modal, an alt attribute should be always inserted in the HTML. When users fill out the "Alt Text" field, the alt attribute is correctly inserted.
But when they leave the field empty, no alt attribute is inserted, not even an empty one. The resulting markup would be something like:

<img class="alignnone" src="https://cldup.com/YeRLlgnhxd.png" width="1195" height="249" />

and in this case an alt="" should be automatically added.

Attachments (5)

36735.patch (452 bytes) - added by ambrosey 8 years ago.
36735.1.patch (988 bytes) - added by dabnpits 8 years ago.
36735.2.patch (920 bytes) - added by joemcgill 8 years ago.
36735.3.patch (401 bytes) - added by joemcgill 8 years ago.
36735.4.patch (533 bytes) - added by adamsilverstein 8 years ago.

Download all attachments as: .zip

Change History (33)

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


8 years ago

#2 @joemcgill
8 years ago

  • Keywords good-first-bug added

#3 follow-up: @ambrosey
8 years ago

  • Keywords has-patch added; needs-patch removed

I looked into this and realized that the problem was that the filters set up on lines 57-61 of media-editor.js were not triggering,

				if ( 'image' === props.type && ! props.alt ) {
					props.alt = props.caption || props.title || '';
					props.alt = props.alt.replace( /<\/?[^>]+>/g, '' );
					props.alt = props.alt.replace( /[\r\n]+/g, ' ' );
				}

because props.type wasn't being assigned. This patch assigns props.type properly within the use case where the type is image, and enters this function properly.

However, now the alt tag that comes through is picking up props.title which is defaulting to the URL being pulled -- i.e.

<img class="alignnone" src="https://cldup.com/YeRLlgnhxd.png" width="1195" height="249" alt="https://cldup.com/YeRLlgnhxd.png" />

I don't think that is what the behavior should be, but I don't understand what the intent is for the alt to pick up in this case.

@ambrosey
8 years ago

#4 in reply to: ↑ 3 @cgrymala
8 years ago

Replying to ambrosey:

However, now the alt tag that comes through is picking up props.title which is defaulting to the URL being pulled

I don't think that is what the behavior should be, but I don't understand what the intent is for the alt to pick up in this case.

It's definitely not ideal, from an accessibility standpoint, but, it does appear to be consistent with the way other "Insert Media" events occur. Currently, WordPress is set up to look at the alt field first, then fallback to the title or caption to use as the alt attribute for the image if the alt field is blank.

#5 @joemcgill
8 years ago

  • Milestone changed from Awaiting Review to 4.6

Thanks for digging into this, @ambrosey, and good catch. @cgrymala is correct that the fallback to caption and then to title mimics what happens in wp_get_attachment_image() see docs. It may be worth discussing a better alternative.

#6 @afercia
8 years ago

For a similar discussion about the alt fallback see also #34635.

#7 @dabnpits
8 years ago

I've attempted to take this a little further in terms of accessibility by preventing the alt tag using the title as a fallback if the title is the URL.

@dabnpits
8 years ago

#8 @joemcgill
8 years ago

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

Hi @dabnpits that's a nice improvement, and maybe one that could be applied in wp_get_attachment_image() as well but that might be better handled as a part of adding a new wp_get_attachment_image_alt() helper function in #30180.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


8 years ago

#10 @afercia
8 years ago

Related: #34635

@joemcgill
8 years ago

#11 @joemcgill
8 years ago

36735.2.patch is similar to 36735.1.patch except that it sets the type property during the callback on the workflow.state for embeds, instead of waiting for wp.media.string.image() and also cleans up a few coding style issues.

Not sure which is the better place in the chain to explicitly set the type property since usually it is taken from the passed attachment, which doesn't exist in this case.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


8 years ago

This ticket was mentioned in Slack in #accessibility by joemcgill. View the logs.


8 years ago

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


8 years ago

#15 @joemcgill
8 years ago

After looking at this a bit more thoroughly, I slightly prefer setting props.type in wp.media.string.image() as suggested by @dabnpits in 36735.1.patch, though there isn't much functional difference from what I can tell.

#16 @joemcgill
8 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 38065:

Media: Always add alt attributes to images inserted from URLs

Previously, when inserting an image from a URL, leaving the alt
field blank in the media modal would result in an image being
inserted into the editor without an alt attribute, rather than
an empty alt. This happened because the props.type would not
get set in wp.media.string.props() — because attachment is
undefined in this case — causing the image fallbacks to get
skipped.

This fixes the issue by explicitly setting props.type to 'image'
in wp.media.string.image() before filling out the rest of the
properties.

Props ambrosey, dabnpits.
Fixes #36735.

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


8 years ago

#18 @afineman
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Currently

alt

is unspecified.

Users should be able to specify

alt=""

See existing source code from media-editor.js line 54-70 below:

	// Final fallbacks run after all processing has been completed.
			fallbacks = function( props ) {
				// Generate alt fallbacks and strip tags.
				if ( 'image' === props.type && ! props.alt ) {
					if ( props.caption ) {
						props.alt = props.caption;
					} else if ( props.title !== props.url ) {
						props.alt = props.title;
					} else {
						props.alt = '';
					}

					props.alt = props.alt.replace( /<\/?[^>]+>/g, '' );
					props.alt = props.alt.replace( /[\r\n]+/g, ' ' );
				}

				return props;

@joemcgill
8 years ago

#19 @joemcgill
8 years ago

Good catch @afineman. This was easy to miss because the visual editor automatically converted this to the proper alt="" notation, but the text editor does not. 36735.3.patch addresses the issue by making an exception in wp.html.string() so empty attribute notation isn't used when the attribute is alt.

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


8 years ago

#21 follow-up: @adamsilverstein
8 years ago

@joemcgill thanks for the quick fix here, I was looking at this ticket today with @afineman and wondering if in wp.html.string we should be inserting as the comment calls it 'empty attribute notation' for any attribute?

Googling the alt tag specifically, I clearly see that it should be a blank string and not just alt with no value... my question is: is it ever proper HTML syntax to insert an attribute without a value, ie. attribute vs. attribute=""?

#22 @adamsilverstein
8 years ago

  • Keywords dev-feedback 2nd-opinion added; good-first-bug removed

36735.4.patch is an alternate approach completely removing the early exit for blank attributes in wp.html.string - this result in empty attributes always getting the ="" appended indicating their blank value. not certain this is proper/better, offering it here pending more research/feedback from the accessibility team, etc.

#23 @afineman
8 years ago

Thanks @joemcgill and @adamsilverstein
I'm new to this and not sure how to apply the patch to see. But one other idea I had that seemed to work — albeit a bit sloppy — was changing line 63 media-editor.js

props.alt = '""';


It results in 4 double quotes first, but resolved to 2 double quotes when the post saved.

#24 in reply to: ↑ 21 @joemcgill
8 years ago

Replying to adamsilverstein:

@joemcgill thanks for the quick fix here, I was looking at this ticket today with @afineman and wondering if in wp.html.string we should be inserting as the comment calls it 'empty attribute notation' for any attribute?

This is a good question. Looking back through the history here, it seems like the current behavior was introduced in [22567], but it's not clear why @koop added this "empty attribute notation" check. Looking at the W3C draft spec, the only valid use case for that style that I could come up with is this section on [boolean attributes](http://w3c.github.io/html/infrastructure.html#sec-boolean-attributes). Ex:

<label><input type=checkbox checked name=cheese disabled> Cheese</label>

However, even for those cases I can't see how something like prop.checkbox = '' would be expected to be true, so 36735.4.patch seems like the right approach here. All of our QUnit tests still pass with 36735.4.patch, but I'm not sure we have adequate (or any) coverage for something that would break by changing that check.

Version 0, edited 8 years ago by joemcgill (next)

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


8 years ago

#26 @afineman
8 years ago

Was able to get the patch installed thanks to @rfair404 help
@joemcgill FYI 36735.3.patch didn't seem to work.
However, patch 36734.4 from @adamsilverstein seems to be working — at least with images inserted by URL. Also works with images that were in the media library — however, those need to have their Title removed, or the alt will print the title name.

For A11y, would be best to have better way to ensure

alt=""

even if users want to have titles for their images on the backend.

Last edited 8 years ago by afineman (previous) (diff)

#27 @joemcgill
8 years ago

  • Keywords commit added; dev-feedback 2nd-opinion removed

Thanks for testing @afineman. 36735.3.patch seems to be working for me, so maybe you were still getting a cached version of the shortcode.js file. At any rate, I think 36735.4.patch is probably the way to go, unless someone has a use case for the current behavior.

Changing the behavior of images inserted from the media library is related, but a bit outside the scope of this ticket. The changes you're proposing for the title would be better discussed as a part of #34635. Feel free to follow along and leave feedback there. Cheers!

#28 @joemcgill
8 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 38116:

Media: Ensure empty alt attributes are set to blank strings.

This is a follow up to [38065] to ensure that wp.html.string() always
converts empty alt attributes to alt="" rather than returning HTML using
empty attribute notation, like alt. This allows screen readers to ignore
images with empty alt attributes, rather than reading out the URL string.

Additionally this completely removes the logic in wp.html.string() for
converting blank attributes to empty attribute notation since empty attribute
notation is generally meant to denote a boolean value, e.g.,
checked == checked="checked", which doesn't apply in this context because
boolean attributes must be omitted in order to represent a false value.
See: https://www.w3.org/TR/html5/infrastructure.html#boolean-attributes

Props adamsilverstein, afineman, joemcgill.
Fixes #36735.

Note: See TracTickets for help on using tickets.