Make WordPress Core

Opened 7 years ago

Last modified 5 years ago

#40590 new defect (bug)

wp_video_shortcode always adds controls="controls"

Reported by: paulschreiber's profile paulschreiber Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.7.4
Component: Media Keywords: has-patch has-unit-tests needs-testing
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 7 years ago.
40590-2.diff (708 bytes) - added by paulschreiber 7 years ago.
40590-3.diff (963 bytes) - added by subrataemfluence 7 years ago.
40590-4.diff (3.1 KB) - added by birgire 6 years ago.

Download all attachments as: .zip

Change History (23)

#1 follow-up: @subrataemfluence
7 years 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
7 years 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 7 years ago by psiico (previous) (diff)

#3 @subrataemfluence
7 years 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
7 years 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 7 years ago by psiico (previous) (diff)

#5 @paulschreiber
7 years ago

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

#6 @paulschreiber
7 years ago

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

#7 follow-up: @paulschreiber
7 years ago

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

#8 @psiico
7 years 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
7 years 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
7 years 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
6 years ago

  • Component changed from General to Media

#12 @birgire
6 years ago

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

@birgire
6 years ago

#13 @birgire
6 years 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
6 years ago

  • Milestone changed from Awaiting Review to 5.0

#15 @pento
5 years ago

  • Milestone changed from 5.0 to 5.1

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


5 years ago

#17 @kirasong
5 years ago

A quick note that toggling video controls is a feature in the new block editor.

This ticket was mentioned in Slack in #core-media by mike. View the logs.


5 years ago

#19 @desrosj
5 years ago

  • Keywords needs-testing added; 2nd-opinion removed
  • Milestone changed from 5.1 to Future Release

This was discussed in today's Media chat. Here is a summary:

  • This is a reasonable change to the video shortcode, even though shortcodes have been unofficially replaced by blocks.
  • Adding the attribute is the preferred path forward (not adding a function argument).
  • The default for the attribute should definitely remain true to maintain the current default behavior.
  • This still needs more testing. @birgire's comment above makes it seem like this is an incomplete solution.

All this considered, let's move this to Future Release until it has a more solid patch everyone is confident in.

Note: See TracTickets for help on using tickets.