WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 4 weeks 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)

for40245.diff (2.2 KB) - added by bamadesigner 14 months ago.
for40245updated.diff (2.2 KB) - added by bamadesigner 14 months ago.
Updated patch for 40245
40245_update.diff (3.1 KB) - added by TomHarrigan 4 months ago.
patch refresh
40245_update.2.diff (3.1 KB) - added by TomHarrigan 4 months ago.
patch refresh - comment fix
40245_update.3.diff (2.1 KB) - added by TomHarrigan 4 months ago.
patch refresh - proper diff
40245_update.4.diff (2.1 KB) - added by TomHarrigan 4 months ago.
patch refresh - proper diff for real this time…
40245.diff (5.1 KB) - added by swissspidy 3 months ago.
40245.2.diff (5.3 KB) - added by afercia 4 weeks ago.
40245.3.diff (5.9 KB) - added by swissspidy 4 weeks ago.

Download all attachments as: .zip

Change History (69)

#1 @swissspidy
2 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

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.

#2 @afercia
2 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.


2 years ago

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


22 months ago

#5 @afercia
22 months ago

  • Milestone changed from Awaiting Review to 4.9

We should try a workaround for 4.9.

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


21 months ago

#8 follow-up: @tfrommen
20 months 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 @SergeyBiryukov
20 months 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.


19 months ago

#11 @afercia
19 months ago

Seems a fix from upstream isn't going to happen. @swissspidy any chance to make a patch for this?

#12 @swissspidy
19 months 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 @afercia
19 months 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.


19 months ago

#15 @afercia
19 months ago

  • Milestone changed from 4.9 to Future Release

Beta 1 is in 2 days. Punting as per today's a11y bug scrub.

#16 @bamadesigner
16 months 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.


16 months ago

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


15 months ago

#19 follow-up: @afercia
15 months ago

@bamadesigner please do feel free to go ahead :)

#20 in reply to: ↑ 19 @bamadesigner
14 months ago

On it!

Replying to afercia:

@bamadesigner please do feel free to go ahead :)

#21 @bamadesigner
14 months ago

Here you go @afercia! I apologize for the delay. #life

for40245.diff

#22 @afercia
14 months 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.

@bamadesigner
14 months ago

Updated patch for 40245

#23 @bamadesigner
14 months ago

Ah! Great catch. I've uploaded the new patch. Thanks!

for40245updated.diff

#24 @swissspidy
14 months 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?

#25 @bamadesigner
14 months ago

It's being replaced because this patch also introduces a "oembed_title" filter.

#26 @swissspidy
13 months 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.

#27 @ocean90
13 months ago

#43647 was marked as a duplicate.

#28 @swissspidy
10 months ago

  • Keywords needs-refresh added

#29 @pento
6 months ago

  • Milestone changed from 5.0 to 5.1

@TomHarrigan
4 months ago

patch refresh

@TomHarrigan
4 months ago

patch refresh - comment fix

@TomHarrigan
4 months ago

patch refresh - proper diff

@TomHarrigan
4 months ago

patch refresh - proper diff for real this time...

#30 @afercia
3 months 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 @pento
3 months 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.

@swissspidy
3 months ago

#32 @swissspidy
3 months 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.


3 months ago

#34 @swissspidy
3 months ago

  • Owner set to swissspidy
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core-editor by brianpeat. View the logs.


2 months ago

#36 @afercia
5 weeks 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 @swissspidy
5 weeks 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 @afercia
5 weeks 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.


5 weeks ago

#40 @jrf
5 weeks 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 @afercia
5 weeks 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 @jrf
5 weeks 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 @afercia
5 weeks ago

@jrf please do feel free to make this regex yours :) I totally trust your judgment.

#44 @jrf
5 weeks 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.

Last edited 5 weeks ago by jrf (previous) (diff)

#46 @afercia
5 weeks 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 @afercia
5 weeks 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 @jrf
5 weeks 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 @afercia
5 weeks 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.

Last edited 5 weeks ago by afercia (previous) (diff)

#50 @jrf
5 weeks 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.


5 weeks ago

#52 @joedolson
5 weeks 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 @afercia
5 weeks 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 @swissspidy
5 weeks 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 @afercia
5 weeks 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 @afercia
4 weeks 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 the wp-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 @jrf
4 weeks ago

@afercia Looks like your new patch is missing the unit tests which were included in @swissspidy's patch....

@afercia
4 weeks ago

#58 @afercia
4 weeks ago

Uh thanks @jrf. I even spent some time to rename stuff in the tests... and forgot to svn add. #blamesvn.
Patch updated.

@swissspidy
4 weeks ago

#59 @swissspidy
4 weeks ago

  • Keywords commit added

Added a new test_filter_oembed_iframe_title_attribute_does_not_modify_other_tags test in 40245.3.diff.

#60 @afercia
4 weeks ago

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

In 44942:

Accessibility: Ensure embed iframes have a title attribute.

Screen reader users rely on the iframe title attribute to describe the contents of iframes. A meaningful title attribute allows to quickly identify the iframe content, so users can determine which iframe to enter and explore in detail or skip if desired.
Note: this is the only case where a title attribute is required for compliance with the W3C Web Content Accessibility Guidelines (WCAG).

  • checks for oEmbed response of type video or rich
  • checks if they use an iframe
  • fetches the title (if any) from the oEmbed response
  • adds the title to the embed iframe

Props bamadesigner, TomHarrigan, swissspidy, jrf, afercia.
Fixes #40245.

Note: See TracTickets for help on using tickets.