cssSelectorAncestor not applied to grouped child selectors properly

[ReactiveRaven]

With multiple videos on page and multiple play/pause controls, the cssSelectorAncestor does not apply properly, causing all play/pause controls after the first in the css selector group to actually control the last-instantiated video element.

Sample configuration:

screen.jPlayer({
     ready: function () {
         $(this).jPlayer("setMedia", {
             title: title,
             m4v: path
         });
     },
     play: function() {
         $(this).jPlayer("pauseOthers");
     },
     swfPath: "/js",
     supplied: "m4v, ogv",
     cssSelectorAncestor: "#foo",
     cssSelector: {
         title: "#title",
         play: ".video__play, .video__controls--play-pause",
         playBar: ".current",
         seekBar: ".progress-bar",
         mute: ".mute",
         unmute: ".unmute",
         fullScreen: ".fullscreen",
         currentTime: ".time-status__now",
         duration: ".time-status__total"
     },
     size: {
          width: "100%",
          height: "auto"
     }
 });

Pertinent part:

screen.jPlayer({
     cssSelectorAncestor: "#foo",
     cssSelector: {
         play: ".video__play, .video__controls--play-pause",
    }
});

So the CSS selector used to find play buttons appears to be #foo .video__play, .video__controls--play-pause and the second in the group (.video__controls--play-pause) is not contained within the #elementId parent object.

This can be worked around by changing the cssSelector.play key to .video__play, #foo .video__controls--play-pause.

[ReactiveRaven]

Appears to be caused on line 2385:

this.css.cs[fn] = this.options.cssSelectorAncestor + " " + cssSel;

if(cssSel) { // Checks for empty string
    this.css.jq[fn] = $(this.css.cs[fn]);

Seems like it should be simply assigning the cssSelector back, then using the ancestor during the query on line 2388.

ie:

this.css.cs[fn] = cssSel;

if(cssSel) { // Checks for empty string
    this.css.jq[fn] = $(this.options.cssSelectorAncestor).find(this.css.cs[fn]);

[thepag]

Not sure where you went wrong, but jPlayer already works with multiple instances. The cssSelectorAncestor specifies the unique ID and then the nested UI elements have classes that then go with the id to make a unique selector.

Did you not see this demo? http://jplayer.org/latest/demo-03-video/

[thepag]

This line here should be a class selector too: title: "#title",

[ReactiveRaven]

@thepag Hi, its not about having multiple on the page, its allowing grouped selectors for the controls

The current code generates a selector by naïvely gluing them together (eg #ancestor and .play, .pause) like this: #ancestor .play, .pause - which for clarity, does this:

#ancestor .play,
.pause

and so only the first of the grouped selectors is scoped inside the ancestor selector. I'm suggesting doing it as two separate queries, one to find the ancestor, the other to find the controls, so you effectively end up with:

#ancestor .play,
#ancestor .pause

[thepag]

Check again mate. It has worked the way you propose for about 2 years or more now.

Otherwise multiple instances on a single page would not work.

Maybe provide a link to your Deb site and I can review tomorrow for the error.

[ReactiveRaven]

Hi, I think this is just me explaining it badly, I've commented on the PR with a clearer example. I think you're focusing on the class names I used, not the code in jPlayer I highlighted? The code is just gluing two strings together, but grouped css selectors don't work that way, the comma starts a new selector and ignores that the ancestor was glued on the front. I'm not on the project that is using the plugin, I just noticed the issue while I was helping out another dev, so I don't have videos to hand to make an example I'm afraid. I can ask them to set one up somewhere public tomorrow if they have time though, but we've fixed it temporarily by changing

cssSelectorAncestor: "#foo",
cssSelector: {
    play: ".video__play, .video__controls--play-pause",

to

cssSelectorAncestor: "#foo",
cssSelector: {
    play: ".video__play, #foo .video__controls--play-pause",

(ie by manually gluing the ancestor on after each comma) for now though, so I don't know if they'll be working on the videos again soon or not.

[thepag]

I'll open this up again to make the pr more visible. I'm away atm, and hope to spend some time on jplayer next week, and in the new year.

As long as it does not break backwards compatibility I see no reason why I'd not accept it.