WordPress.org

Make WordPress Core

Opened 14 months ago

Last modified 4 months ago

#40590 new defect (bug)

wp_video_shortcode always adds controls="controls"

Reported by: paulschreiber Owned by:
Milestone: 5.0 Priority: normal
Severity: normal Version: 4.7.4
Component: Media Keywords: has-patch 2nd-opinion has-unit-tests
Focuses: Cc:

Description

wp_video_shortcode always adds controls="controls" to the <video> tags it generates.

The user should be able to pass in a value to $attr to disable output of controls="controls".

Attachments (4)

40590.diff (667 bytes) - added by subrataemfluence 14 months ago.
40590-2.diff (708 bytes) - added by paulschreiber 14 months ago.
40590-3.diff (963 bytes) - added by subrataemfluence 14 months ago.
40590-4.diff (3.1 KB) - added by birgire 4 months ago.

Download all attachments as: .zip

Change History (18)

#1 follow-up: @subrataemfluence
14 months ago

  • Keywords has-patch added
[video src="https://www.youtube.com/watch?v=NhheiPTdZCw" controls="false"]

The patch checks if an attribute called "controls" is set in $attr with value "false" in order to prevent output of controls="controls" attribute in the markup.

Otherwise it will add above attribute to the markup.

#2 in reply to: ↑ 1 @psiico
14 months ago

  • Keywords 2nd-opinion added

Applied your patch to wp-includes/media.php and the same problem keeps arising. Whether it's a self-hosted video or YouTube or Vimeo.

Actually, when using YT video, it seems that the YT iframe is contained inside the video (mejs-container) obj container which shows the controls from YT and mejs overlapped.

Replying to subrataemfluence:

[video src="https://www.youtube.com/watch?v=NhheiPTdZCw" controls="false"]

The patch checks if an attribute called "controls" is set in $attr with value "false" in order to prevent output of controls="controls" attribute in the markup.

Otherwise it will add above attribute to the markup.

Last edited 14 months ago by psiico (previous) (diff)

#3 @subrataemfluence
14 months ago

@psiico First of all thank you so much for testing the patch. When we use wp_video_shortcode like this

[video src="https://www.youtube.com/watch?v=NhheiPTdZCw"]

by default WordPress adds the controls attribute as per the function in media.php (if I have understood right) without the patch implemented. The patch worked for me for the above video URL in both occasions, i.e. by passing controls="false" and not adding this attribute at all. Anything else, e.g. controls="true" adds the attribute in markup.

This is how the markup gets generated (with / without the attribute). wp_video_shortcode does not actually render a frame rather it creates a HTML5 markup.

When I pass controls="true", i.e.

[video src="https://www.youtube.com/watch?v=NhheiPTdZCw" controls="true"]

the rendered output is like this:

<video class="wp-video-shortcode" id="video-802-1" width="640" height="360" preload="metadata" controls="controls"><source type="video/youtube" src="https://www.youtube.com/watch?v=NhheiPTdZCw&#038;_=1" /><a href="https://www.youtube.com/watch?v=NhheiPTdZCw">https://www.youtube.com/watch?v=NhheiPTdZCw</a></video>

and when I pass controls="false", i.e.

[video src="https://www.youtube.com/watch?v=NhheiPTdZCw" controls="false"]

or just

[video src="https://www.youtube.com/watch?v=NhheiPTdZCw"]

the output is:

<video class="wp-video-shortcode" id="video-802-1" width="640" height="360" preload="metadata" ><source type="video/youtube" src="https://www.youtube.com/watch?v=NhheiPTdZCw&#038;_=1" /><a href="https://www.youtube.com/watch?v=NhheiPTdZCw">https://www.youtube.com/watch?v=NhheiPTdZCw</a></video>

In latter two cases the WordPress is not adding controls="controls" in the markup.

Would you please tell me how I can reproduce your way of testing?

#4 @psiico
14 months ago

@subrataemfluence Yes, of course! Before I walk you though it, I think you should change the controls value from false to off to be in tune with the rest of the parameters.

So, I'm running the trunk version. Default theme and plugins. Edited the hello world post and pasted your code after the default content. [video src="https://www.youtube.com/watch?v=NhheiPTdZCw" controls="false"]

The hello world post renders as follows:

http://i.imgur.com/nLRpafj.png

The generated video html is as follows:

<div id="mep_0" class="mejs-container svg wp-video-shortcode mejs-video" tabindex="0" role="application" aria-label="Video Player" style="width: 524px; height: 294px;"><div class="mejs-inner"><div class="mejs-mediaelement"><iframe class="me-plugin" id="me_youtube_0_container" frameborder="0" allowfullscreen="1" title="YouTube video player" width="524" height="294" src="https://www.youtube.com/embed/NhheiPTdZCw?controls=0&amp;wmode=transparent&amp;enablejsapi=1&amp;origin=http%3A%2F%2Flocalhost&amp;widgetid=1" style="width: 524px; height: 294px;"></iframe><video class="wp-video-shortcode" id="video-1-1" width="525" height="295" preload="metadata" style="width: 100%; height: 100%; display: none;"><source type="video/youtube" src="https://www.youtube.com/watch?v=NhheiPTdZCw&amp;_=1"><a href="https://www.youtube.com/watch?v=NhheiPTdZCw">https://www.youtube.com/watch?v=NhheiPTdZCw</a></video></div><div class="mejs-layers"><div class="mejs-poster mejs-layer" style="display: none; width: 100%; height: 100%;"></div><div class="mejs-overlay mejs-layer" style="display: none; width: 100%; height: 100%;"><div class="mejs-overlay-loading"><span></span></div></div><div class="mejs-overlay mejs-layer" style="display: none; width: 100%; height: 100%;"><div class="mejs-overlay-error"></div></div><div class="mejs-overlay mejs-layer mejs-overlay-play" style="width: 100%; height: 100%;"><div class="mejs-overlay-button"></div></div></div><div class="mejs-controls" style="display: block;"><div class="mejs-button mejs-playpause-button mejs-play"><button type="button" aria-controls="mep_0" title="Play" aria-label="Play"></button></div><div class="mejs-time mejs-currenttime-container" role="timer" aria-live="off"><span class="mejs-currenttime">00:00</span></div><div class="mejs-time-rail" style="width: 378px;"><span class="mejs-time-total mejs-time-slider" style="width: 368px;" aria-label="Time Slider" aria-valuemin="0" aria-valuemax="322" aria-valuenow="0" aria-valuetext="00:00" role="slider" tabindex="0"><span class="mejs-time-buffering" style="display: none;"></span><span class="mejs-time-loaded" style="width: 0px;"></span><span class="mejs-time-current" style="width: 0px;"></span><span class="mejs-time-handle" style="left: -5px;"></span><span class="mejs-time-float"><span class="mejs-time-float-current">00:00</span><span class="mejs-time-float-corner"></span></span></span></div><div class="mejs-time mejs-duration-container"><span class="mejs-duration">05:22</span></div><div class="mejs-button mejs-volume-button mejs-mute"><button type="button" aria-controls="mep_0" title="Mute" aria-label="Mute"></button><a href="javascript:void(0);" class="mejs-volume-slider" style="display: none;"><span class="mejs-offscreen">Use Up/Down Arrow keys to increase or decrease volume.</span><div class="mejs-volume-total"></div><div class="mejs-volume-current" style="height: 100px; top: 8px;"></div><div class="mejs-volume-handle" style="top: 5px;"></div></a></div><div class="mejs-button mejs-fullscreen-button"><button type="button" aria-controls="mep_0" title="Fullscreen" aria-label="Fullscreen"></button></div></div><div class="mejs-clear"></div></div></div>
Last edited 14 months ago by psiico (previous) (diff)

#5 @paulschreiber
14 months ago

@psiico this code isn't in the trunk version. you'll need to manually patch your copy.

#6 @paulschreiber
14 months ago

@psiico this code isn't in the trunk version. you'll need to manually patch your copy.

#7 follow-up: @paulschreiber
14 months ago

I've updated the patch to: (a) allow more false values (false, off, 0) (b) use WordPress coding style

#8 @psiico
14 months ago

Oh yes! You are totally right and I miss that detail. I'm sorry for wasting your time! Nonetheless I applied the patch to a 4.7.4 version I had lying around, fully updated running Twenty Fifteen, no plugins, and I have the same output as I showed on my previous trunk related post. I even tried to render the shortcode both in post and page context and both outputted the same. Having controls="on" or "off" doesn't change in the slightest the code.

#9 in reply to: ↑ 7 @subrataemfluence
14 months ago

Thank you for modifying the patch. However, it is still adding controls="controls when I don't pass the parameter i.e. simply [video src="https://www.youtube.com/watch?v=NhheiPTdZCw"].

I have made some further changes in the code. It now first checks whether controls is at all set. If yes, it looks for values "on", "true" or "1". If any of these is found controls="controls" gets added to the markup. For everything else (with controls parameter set) the attribute will be ignored while rendering the output markup.

And if the parameter is not set at all, the attribute will never be added to the markup.

Replying to paulschreiber:

I've updated the patch to: (a) allow more false values (false, off, 0) (b) use WordPress coding style

#10 @paulschreiber
14 months ago

Requiring a controls attribute to be present in the shortcode is a bad idea. It will break all existing uses of the shortcode.

Having controls present is a sensible default.

#11 @SergeyBiryukov
4 months ago

  • Component changed from General to Media

#12 @birgire
4 months ago

Here's a somewhat related ticket #42718 for the muted attribute for the video shortcode.

@birgire
4 months ago

#13 @birgire
4 months ago

  • Keywords has-unit-tests added

Here's a suggestion in 40590-4.diff that:

  • uses wp_validate_boolean() on the controls attribute of the [video] shortcode, similar as with loop and autoplay attributes. It can validate inputs like these boolean strings: "true", "1", "false", "0" . If it's validated as true then controls="controls" is added to the <video> tag, otherwise it's not added.
  • sets the controls shortcode's attribute as true by default.
  • updates the existing test_wp_video_shortcode_attributes() test method, with two assertions. If more is needed, we could add new test methods for various input cases of the controls attribute.

This patch only covers adding/removing controls="controls" to the video tag, via the boolean string controls shortcode's attribute. It's another matter how MediaElement.js handles this video tag attribute. I did some quick testing and it looks like more is needed to remove the controls.

#14 @SergeyBiryukov
4 months ago

  • Milestone changed from Awaiting Review to 5.0
Note: See TracTickets for help on using tickets.