Make WordPress Core

Opened 10 years ago

Closed 10 years ago

#29383 closed defect (bug) (fixed)

WPPlaylistView doesn't find the right script.

Reported by: programmin's profile programmin Owned by: azaozz's profile azaozz
Milestone: 4.0 Priority: normal
Severity: normal Version: 4.0
Component: Media Keywords: has-patch commit dev-reviewed
Focuses: Cc:

Description

WPPlaylistView is looking for any script, then getting the contents, when it's initializing the playlist. On my test setup this is grabbing a different script, a google-analytics ga.js.

Please change wp-playlist.js to look for

this.$('script[type="application/json"]').html()

instead of

this.$('script').html()

in initialize function.

Attachments (2)

29383.patch (1.1 KB) - added by SergeyBiryukov 10 years ago.
29383.2.patch (1.1 KB) - added by SergeyBiryukov 10 years ago.

Download all attachments as: .zip

Change History (19)

#1 @wonderboymusic
10 years ago

it is looking for a script inside of .wp-playlist - are you shoving more <script>s in there? If so, why?

#2 follow-up: @programmin
10 years ago

Honestly I don't know where the theme was shoving more script tags into it right now. Regardless I think it should be possible to add script tags, and not have the playlist blindly grab the first script tag's contents.

It would also be much more readable to grab it like this, as getting the html() contents of a script tag is rarely done, and this would clarify that it is getting script[type="application/json"].

#3 in reply to: ↑ 2 @wonderboymusic
10 years ago

Replying to programmin:

and not have the playlist blindly grab the first script tag's contents.

script[type="application/json"] would still blindly grab the first <script> that matched. So any theme that, for whatever reason, wanted to prepend a script with application/json would win. This is the fault of the theme/plugin. It is breaking your playlists.

#4 @programmin
10 years ago

Very very rarely will you see a script with application/json type, I see it's used here to store {object} that will be needed by the script, why not be specific and avoid accidental interference of any other script that happened to printout?

#5 @programmin
10 years ago

Here's a minimal test case that actually breaks all the wp-playlist functionality in a few lines of code:

<?php
/*
Plugin Name: Test
Description: Example causing WP bug 29383
Version: 0.1
*/

add_action( 'wp_print_scripts', 'printstuff' );
function printstuff() {

?><script>
(function() {
var ga = document.createElement('script'); ga.type = 'text/javascript'; ga.async = true;
	ga.src = 'http://anywhere';
	var s = document.getElementsByTagName('script')[0]; s.parentNode.insertBefore(ga, s);
})();
</script>
<?php

}

#6 @SergeyBiryukov
10 years ago

  • Component changed from General to Media

#7 @programmin
10 years ago

Still happening on RC, surprisingly this doesn't seem to happen in 3.9 because it wasn't separating wp-view into an iframe.

#8 @SergeyBiryukov
10 years ago

  • Milestone changed from Awaiting Review to 4.0

Confirmed the issue.

#9 follow-up: @SergeyBiryukov
10 years ago

  • Keywords has-patch added

29383.patch fixes it for me.

#10 @programmin
10 years ago

Thank you, that also seems to fix it. Is this going to be ready in 4.0 I hope?

#11 in reply to: ↑ 9 @azaozz
10 years ago

Replying to SergeyBiryukov:

Yep, the <script> tag supports all of the global attributes, so adding a class or id or data-* would work. Maybe even easier to use a class as jQuery caches the data-* attributes :)

Last edited 10 years ago by azaozz (previous) (diff)

#12 @SergeyBiryukov
10 years ago

class wasn't a valid attribute for <script> in HTML4, and I wasn't sure if it's valid in HTML5. Apparently it is.

#13 @programmin
10 years ago

Just using script[type="application/json"] should work, according to this article
http://css-tricks.com/attribute-selectors/

"Every single example above works in all modern browsers: Safari, Chrome, Firefox, Opera, and IE. Internet Explorer has perfect support for all of these down to version 7":

It would also make it clear that this "script" is actually a json it's grabbing.

Class would be ok too though.

#14 @programmin
10 years ago

I just updated svn and it looks like this is not patched yet in wp-playlist.js Can someone please make sure this gets merged, this would be a pretty big regression.

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.


10 years ago

#16 @nacin
10 years ago

  • Keywords commit dev-reviewed added

The attribute selector for application/json doesn't bother me, but a class seems more appropriate as application/json is still just a guess / process of elimination, rather than explicitly picking the right one.

#17 @azaozz
10 years ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from new to closed

In 29660:

Media: add a class to the <script> tag for the JSON encoded playlist data so it can be easily selected in WPPlaylistView. Props SergeyBiryukov, fixes #29383

Note: See TracTickets for help on using tickets.