Opened 9 years ago
Closed 8 years ago
#36735 closed defect (bug) (fixed)
Add media, insert from URL and alt attribute
Reported by: | afercia | Owned by: | 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)
Change History (33)
This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.
9 years ago
#4
in reply to:
↑ 3
@
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
@
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.
#7
@
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.
#8
@
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
#11
@
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
@
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.
This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.
8 years ago
#18
@
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;
#19
@
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:
↓ 24
@
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
@
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
@
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
@
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. 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.
This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.
8 years ago
#26
@
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.
#27
@
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!
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,
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.
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.