Opened 8 years ago
Closed 6 years ago
#40245 closed enhancement (fixed)
Youtube embeds lack title attribute
Reported by: | morriscountynj | Owned by: | swissspidy |
---|---|---|---|
Milestone: | 5.2 | Priority: | normal |
Severity: | normal | Version: | 2.9 |
Component: | Embeds | Keywords: | has-patch has-unit-tests commit |
Focuses: | accessibility | Cc: |
Description
I use a service called Siteimprove to monitor for accessibility issues. They pointed out an issue -- when you embed a Youtube video via a link, the code comes out something like this
<iframe width="960" height="540" src="https://www.youtube.com/embed/iGddWyoWkyg?feature=oembed" frameborder="0" allowfullscreen=""></iframe>
However, Siteimprove gives that an A-level error:
The iFrame is missing a description
The iFrame has no 'title' attribute or the 'title' attribute is empty.
Provide the frame with the attribute title=”” and add a description of the content in the title.
Is there any way to add a field to enter a title for the iframe?
See this page for an example: Example
Attachments (9)
Change History (69)
#1
@
8 years ago
- Summary changed from Youtube embed -- need space to put title="" in iframe to Youtube embeds lack title attribute
- Type changed from feature request to enhancement
- Version changed from 4.7.3 to 2.9
#2
@
8 years ago
- Focuses accessibility added
Adding the accessibility focus, since iframe elements should really have a title attribute (and it's the only valid case of title attribute usage WordPress currently recommends, see all the work done so far about title attributes).
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 afercia. View the logs.
7 years ago
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
7 years ago
#8
follow-up:
↓ 9
@
7 years ago
Even though noone responded to the Google ticket, this seems to have been fixed already, see the example embed link @joedolson posted: https://www.youtube.com/oembed?url=http%3A//youtube.com/watch%3Fv%3DM3r2XDceM6A&format=json
However, when I paste the URL (i.e., https://www.youtube.com/watch?v=M3r2XDceM6A) into the editor, the generated output still lacks the title attribute:
<iframe src="https://www.youtube.com/embed/M3r2XDceM6A?feature=oembed" allowfullscreen="" height="473" frameborder="0" width="840"></iframe>
Am I doing anything wrong, or did I misunderstand what the example link was about? Can you help, @swissspidy?
#9
in reply to:
↑ 8
@
7 years ago
Replying to tfrommen:
this seems to have been fixed already, see the example embed link @joedolson posted: https://www.youtube.com/oembed?url=http%3A//youtube.com/watch%3Fv%3DM3r2XDceM6A&format=json
There's still no title
on the <iframe>
element in that response, only as a separate value in the array.
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
7 years ago
#11
@
7 years ago
Seems a fix from upstream isn't going to happen. @swissspidy any chance to make a patch for this?
#12
@
7 years ago
It would be great if we could first check other oEmbed providers to see whether this is a broader problem. If so, we could apply a more general solution, not just one tied to YouTube.
#13
@
7 years ago
It would be great if we could first check other oEmbed providers to see whether this is a broader problem
I've made my part and checked Vimeo :) it adds a title attribute on the iframe with the video title. Just the title of the video. It doesn't say the iframe content is about a video though. So, for example, when embedding a video that ironically has a title like The Bigger Picture, the iframe title doesn't let me actually understand what the iframe content is about.
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
7 years ago
#15
@
7 years ago
- Milestone changed from 4.9 to Future Release
Beta 1 is in 2 days. Punting as per today's a11y bug scrub.
#16
@
7 years ago
I wrote some code to solve this issue for my websites. I don't mind submitting a patch to get things started, if it's still needed.
Here's how I solved with a filter. Basically it looks for the title in the embed data array and as a possible existing title attribute. If anything, even if the title does exist in the iframe markup, it allows for it to be filtered. If it doesn't exist, it allows for it to be created from the data array or via filter.
https://github.com/wpcampus/wpcampus-network-plugin/commit/a48e4c81b1233a7051139046a91b3a19777ea902
This ticket was mentioned in Slack in #accessibility by bamadesigner. View the logs.
7 years ago
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
7 years ago
#22
@
7 years ago
- Keywords has-patch added
- Milestone changed from Future Release to 5.0
@bamadesigner thanks for the patch! Seems a very good start to me. Couple minor things: the keyword public
should be removed and the title attribute value should be escaped with esc_attr()
.
Moving to 5.0 for consideration. Review and feedback from the Embeds component team would be greatly appreciated.
#24
@
7 years ago
Why is an existing title attribute being replaced in that patch? Shouldn't this only add missing attributes instead of overriding the provided ones?
#26
@
7 years ago
If we‘re going to add that, I think that filter should be renamed to oembed_iframe_title_attribute or something. Also, it needs to be documented.
#30
@
6 years ago
@swissspidy mind owning this ticket? As I know you have some expertise with embeds :) Would be great to make it land in 5.1.
#31
@
6 years ago
- Keywords needs-unit-tests added; needs-refresh removed
- Milestone changed from 5.1 to 5.2
This could do with a bunch of unit tests.
#32
@
6 years ago
- Keywords has-unit-tests added; needs-unit-tests removed
Added a new patch with unit tests.
I think $pattern
needs to be improved so that only iframes will be touched.
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-editor by brianpeat. View the logs.
6 years ago
#36
@
6 years ago
@swissspidy when you have a chance: what's left here? Ten days left to 5.2 beta 1. I'd really appreciate some feedback to not punt this ticket again to next release. Thank you :)
#37
@
6 years ago
I think wp_filter_oembed_title_attribute
needs some adjustments to make sure it only targets iframes. Otherwise anyone filtering oembed_title_attribute
would add title attributes to any element.
Maybe using a regex like this: <iframe[^>]*?title=["']([^"']*)["'][^>]*?>
I'm not a regex expert though :-)
#38
@
6 years ago
@swissspidy thanks. Who's the best person who can help you moving this ticket on? :)
This ticket was mentioned in Slack in #core by afercia. View the logs.
6 years ago
#40
@
6 years ago
Did I hear regex expert ? Want me to take a look ?
What are the specs and have we got test data for things which should and shouldn't match ?
#41
@
6 years ago
@jrf thanks! I think @swissspidy has more context but I'll try to describe what I see in the patch the best I can.
We need to add a title attribute with the video title to the HTML returned from an oembed provider response. The patch already checks if the oembed is a video.
After that, it uses the following regex to search for an existing title attribute in the returned HTML:
$pattern = '/title\=[\"|\\\']{1}([^\"\\\']*)[\"|\\\']{1}/i';
that could match any title attribute in any element in the returned HTML. Instead, we need to search for a match only within the <iframe ...>
HTML tag, if any.
#42
@
6 years ago
@afercia Ok, I'll have a look. I presume you'd be happy to see other improvements to the regex ? It currently looks very unstable with regards to the quote matching to me.
#43
@
6 years ago
@jrf please do feel free to make this regex yours :) I totally trust your judgment.
#44
@
6 years ago
Improved regex line:
<?php $pattern = '`<iframe[^>]*?title=(\\\\'|\\\\"|[\'"])([^>]*?)\1`i';
Important: $matches[1]
on line 802 and 803 will need to be changed to $matches[2]
when using this regex.
Test data used:
<img src="..." title="title"/> '<iframe src="..." title="title" width="324" height="auto"/>' "<iframe src='...' width='324' height='auto' title='title' ></iframe>" "<iframe src=\"...\" title=\"title\" width=\"324\" height=\"auto\"/>" '<iframe src=\'...\' width=\'324\' height=\'auto\' title=\'title\' ></iframe>' "<iframe src=\"...\" title=\"title's with apostrophe\" width=\"324\" height=\"auto\"/>" "<iframe src='...' width='324' height='auto' title='title \"quoting\" something' ></iframe>"
Aside from the first one which shouldn't match, all the others now match the title correctly.
Just for fun, try using the old regex against this data set.
Let me know if there are any peculiarities in how the oembed data is received which should be taken into account additionally.
#45
@
6 years ago
FYI: Regex tests https://regex101.com/r/M8epXh/2
#46
@
6 years ago
Thanks @jrf! Then, I guess there's the need for a second (simpler) regex to use for the replacement at line 823. Would something like the following be OK or can we use something simpler?
<?php '`title=(\\\\\'|\\\\"|[\'"]).*?\1`i'
#47
@
6 years ago
@swissspidy I wonder if we should prepend something to the title attribute string. With the current patch the title attribute would be, for example:
<iframe title="House of Commons debate on extension of Article 50" ...
Users will be informed about the topic but still they don't know it's a video :) How about prepending Video:
<iframe title="Video: House of Commons debate on extension of Article 50" ...
#48
@
6 years ago
@afercia Good question. To be fair, I would just simplify that code block like so:
Original code:
<?php if ( $has_title_attr ) { $result = preg_replace( $pattern, 'title="' . esc_attr( $title ) . '"', $result ); } else { return str_ireplace( '<iframe ', sprintf( '<iframe title="%s" ', esc_attr( $title ) ), $result ); } return $result;
Suggested alternative:
<?php if ( $has_title_attr ) { // Remove the old title. $result = preg_replace( $pattern, '', $result ); } return str_ireplace( '<iframe ', sprintf( '<iframe title="%s" ', esc_attr( $title ) ), $result );
#49
@
6 years ago
@jrf to my understanding:
<?php // Remove the old title. $result = preg_replace( $pattern, '', $result );
would remove also the <iframe
start tag :) the full match of $pattern
includes anything starting with <iframe
(which we need for the first check) and a title attribute.
#50
@
6 years ago
@afercia Shoot, yes, you're right. I shouldn't be doing twenty things at the same time.
Ok, what about this: (though you run the risk of replacing the title in other HTML elements too if it would be exactly the same)
<?php if ( $has_title_attr ) { // Remove the old title. $result = str_replace( ' title=' . $matches[1] . $matches[2] . $matches[1], '', $result ); } return str_ireplace( '<iframe ', sprintf( '<iframe title="%s" ', esc_attr( $title ) ), $result );
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
6 years ago
#52
@
6 years ago
Re: preprending "Video" - what about prepending "YouTube"? That would give users more definite context, and possibly tell users of YouTube how the controls will work, where 'Video' would still require them to discover that.
#53
@
6 years ago
I shouldn't be doing twenty things at the same time.
😄 Thanks for your help @jrf ! Yep, I'd tend to think improving the conditional instead of using one more regex would be best.
#54
@
6 years ago
Re: preprending "Video" - what about prepending "YouTube"?
Well, the patch currently applies to all iframe embeds, regardless of their source. It could be a YouTube video, Vimeo video, tweet, poll, etc. We could use the provider_name
field in the oEmbed data if it's available and prepend that to the title. Would that work?
#55
@
6 years ago
the patch currently applies to all iframe embeds, regardless of their source
The patch only tries to set a title if the embed type is of type rich
or video
: it returns early if ! in_array( $data->type, array( 'rich', 'video' )
Looks like Twitter is rich
but doesn't use an iframe (any longer?) and uses the shadow DOM instead.
Regardless, this doesn't help us. We could certainly prepend Video:
when the type is video
, then the regex searches for an iframe but it's not possible to predict what a rich
embed with iframe actually is: it could be a Tweet, a WordPress post, whatever.
Anyways, having an iframe title is better than not having it :)
#56
@
6 years ago
40245.2.diff refreshes the patch:
- uses the regex and final replacement suggested by @jrf
- renames all the things to clarify it's about the iframe title attribute
- running
grunt precommit
updated also thewp-api-generated.js
file, adding the title attribute to "Una cancion para programadores". Didn't know about that song :)
Some testing and review would be appreciated. Reminder: the oembed responses get saved as post meta. When testing, either use a new video to embed each time or delete the _oembed ...
entries from the database.
#57
@
6 years ago
@afercia Looks like your new patch is missing the unit tests which were included in @swissspidy's patch....
#58
@
6 years ago
Uh thanks @jrf. I even spent some time to rename stuff in the tests... and forgot to svn add
. #blamesvn.
Patch updated.
#59
@
6 years ago
- Keywords commit added
Added a new test_filter_oembed_iframe_title_attribute_does_not_modify_other_tags
test in 40245.3.diff.
Hey there,
Welcome to WordPress Trac and thanks for your report!
WordPress embeds have a title attribute for exact these reasons. Unfortunately, YouTube doesn't add a title attribute themselves. oEmbed fetches the
<iframe>
code directly from YouTube. See https://www.youtube.com/oembed?url=http%3A//youtube.com/watch%3Fv%3DM3r2XDceM6A&format=json for an example.It seems like Drupal fixed this on their own by adding a custom title attribute, see https://www.drupal.org/node/2085749. Apparently they also added a name attribute: https://www.drupal.org/node/2129317. We could certainly do the same, although I'd love to see YouTube fixing this on their end.