Defer Youtube video loading

Indeed it's not working. The video is correctly loaded when the wysiwyg is rendered in front, but when editing the content in the backoffice it's broken.

I will correct this.

- We can disable the "defer loading" feature in the backoffice.

- We can listen to every change in the DOM and load the video when we see the scald-youtube class but it's heavy.
We can also listen only the wysiwyg div to detect changes.

I prefer the first solution, it's simplier. And the wywisyg already use a lots of JS, so I think it's better to avoid adding more. What do you think about this?

So, it seems that we cannot test if we are in an admin page with is_admin_path function because the path "scald/library_dnd" used by the dnd library is not defined as an admin path.
So I've just tested directly the path in order to know if we are in a dnd context. Now dragging a video into the wysiwyg works :D

I also finally added a listener in order to support when the video is added dynamically in javascript. Videos are often in popins and the content of a popin is generally added by javascript, so I think it's important afterall.

I agree that we can't accept a patch if it does not render the video correctly.

The problem could be corrected by adding the scald_youtube_player.js file into the wysiwyg iframe. But I've searched a little and it seems really complicated. If you know a way to do that, I'm interested. If we can do that, we can forget about detecting the dnd thing.

Concerning the path, I've printed the current path into the video template and it was returning "node/*/edit" when the video was rendered at the page loading and "scald/library_dnd" when we add a video using dnd.

I've tried to reproduce the problem of the iframe frameborders, but I can't for now.
I've updated scald to the last version because I was using 2.6 but it doesn't change anything. I also tried to disable the "Scald SAS conversion" into the wysiwyg profile, but it was not successfull either.

I will install a new drupal website to test the patch and try to reproduce the problem because it is working on my project but there are some patches on scald and some custom code to have ESI blocks on videos (normally not used when we're connected).

Yes, I've installed a brand new drupal website and the ajax calls is also done on "atom/ajax-widget-expand/1".

Checking "atom/ajax-widget-expand/*" and "scald/library_dnd" works in both cases, but the code seems pretty ugly...

Searching the problem in the scald commits to find the difference, I found something interesting. Checking that the path is an admin path works.

I was with the scald 1.6 version and the 1.7 version includes this issue #2410373 that add "scald/library_dnd" as an admin path.

It works on my both websites after updating scald version. There is just one little problem, when we look at the patch of the issue that add "scald/library_dnd" as an admin path (#2410373 patch), we can see that we need to set the "dnd_modal_admin" variable to "1" in order to have an admin path. It can be set in "admin/config/content/dnd". I don't know why they let the choice to the administrator...

But if this config is not set to TRUE, we will have the problem of the iframe frameborders (only when we use "scald/library_dnd" path). Should we create a config form in order to enable/disable the "defer" feature manually and add a description to notice the administrator about the dnd_modal_admin variable?

I will search if scald core provide something which could help us.

I searched into scald core but I couldn't make it work using some hook from the core :(

It seems that the dnd library use admin paths to render atoms in ajax. So I just test that it is an admin path and I add an exception for the "scald/library_dnd" which depends on administrator choice. So it should work on all cases now.

I didn't manage to make this work, so I've worked on my original approach that is defer the videos into the wysiwyg too. It works pretty well on my configuration, I think it should be ok. I thought it was more complicated than disable into the backoffice but I was wrong.

So in order to test this:
- By default the defer features is disabled, you need to enable it in /admin/config/content/scald
- I've created a wysiwyg plugin that will add the JS we need into the iframe in order to load Youtube videos. So you also need to enable the plugin for your wysiwyg profile when the Defer features is enabled. I've written this in the description of the field in the configuration page.
- After that it should be working in all cases.

I've also corrected the javascript for Internet Explorer, I was using a function that was not defined in IE.

I was thinking about when your render an atom in autoplay in the page but you want to stay hidden and quiet waiting for a click to open a popin. The developer can keep the rendering of the scald into a data-attribute and insert it when opening the popin.

But I think it is probably not the right way to do that, an ajax call to get the rendering of the atom is probably better. 'DOMSubtreeModified' could be quite heavy so removing it, is a good idea in order to avoid multiple calls to that javascript. You're right.

I've just removed it in the last patch.

I launch the test but I totally forgot that the patch needed the other one from #2879018

I've corrected all your suggestions. I hope it's ok.

Conerning the CSS, I think I forgot to add the file for the previous patch. I'm hiding the ckeditor button which is required to add the JS but useless for the user.

I watched your modifications.
The configuration of the defer option is indeed in a better place now. I hadn't understood your comment, I thought it was just a field group.

I use the wysiwyg module and not the ckeditor one. It is not working anymore with your patch.
CKEditor uses an iframe so I suppose that the CkEditor module adds JS files put in the #attached inside the iframe otherwise I don't see how it could work.

I searched into the Wysiwyg module an issue concerning adding custom JS files. There is one (#1126656) but they say that the Wysiwyg module supports several editors that not use iframes, so if we want to add custom JS, we have to create a plugin. So I think we should keep the plugin in order to keep wysiwyg compatibility.
And if we don't keep compatibility with wysiwyg, we should merge scald_youtube_player_library.js and scald_youtube_player.js which were separated in order to be used again into the plugin.

I just did the patch again keeping the wysiwyg plugin, in case we keep it.

Ok I've applied your modifications.

I've also noticed that it was buggy into the CkEditor iframe using wysiwyg. I didn't noticed it because of my browser cache. JQuery is not loaded into the iframe with wysiwyg, so we got an error when we call the 'initializeYoutubeVideos' function.

So I used my previous function written in native JS. And I've corrected the error about the context in comment #24. Native JS should work everywhere now.

Thanks a lot for your help, it is really formative and it shows me that it is very hard to think about all the cases.

In order to debug this, I'm interested to know your configuration. I was testing with Firefox 54 Linux.

var contextUsed = (typeof context !== 'undefined' && context !== document ? context[0] : document);

For me context can be the document itself or a jQuery object but I think there is a third case, it can be a DOM reference. I can't test this this week end but I will on Monday.

VM3524:10 div.cke_widget_element.dnd-widget-wrapper.context-sdl_editor_representation.type-video

This line is directly a reference to the DOM or a jQuery object? I think it is the first case by looking at your code which was working.

The code below could work, I will test it on Monday and see if I can reproduce your problem.

var contextUsed = (typeof context !== 'undefined' && context !== document ? context : document);
// If we have a jQuery object, extract the DOM element.
if (typeof context[0] !== 'undefined') {
  contextUsed = context[0];
}

Just failed to do a correct patch on Windows so I will do it again correctly on Linux on Monday and test it ;)

I've corrected coding standards for JS and PHP files.

And I've just added one test in the JS because for some reason the JS is called with an empty context when I drag a scald and context[0] cause an error.

// If we have a jQuery object, extract the DOM element.
  if (typeof context !== 'undefined' && typeof context[0] !== 'undefined') {
    contextUsed = context[0];
  }

I think we finally reach the end of this issue :)

Sorry, I've uploaded the same patch twice, don't really know how I did that.

Sorry about the delay for my answer, I was very busy.

"use strict" is just a javascript coding standard, it is not an issue at all. But it is strange because it doesn't crash in my browser.
So I've done one version of the patch without the use strict if you want to test.

And I've also done a patch which add only the necessary JS test for me in addition to your patch.
Maybe I've done a modification in the coding standards that was buggy, so I've remove all the other modifications.

I know but I can't reproduce the error on my environment ( Firefox 55 and Chrome 58 on Ubuntu 14).

So should we test the patch #46 wich just remove "use strict" ?
Or there is the patch #47 which is normally the same as yours in #38 but with an additionnal test in order to avoid an error when the context is not defined.