Flash dialog sets only false param

[datalink]

If I unset checkbox menu in flash dialog, source shows

<param name="menu" value="false" />

But if I set it to true, source shows nothing. Should be

<param name="menu" value="true" />

[Jakub Ś]

But "menu" is one of optional parameters which if omitted is set to true. Further more if this parameter is omitted, menu is displayed.

What exactly is the problem here?

[datalink]

OK, you are right. But allowfullscreen must be set to true, that it works.

<param name="allowfullscreen" value="true" />

and

<embed allowfullscreen="true" ...

[Jakub Ś]

According to: ​http://www.adobe.com/devnet/flashplayer/articles/full_screen_mode.html, section "security" allowfullscreen option defaults to false so checking this option in flash properties dialog should result in:

<param name="allowfullscreen" value="true" />

and

<embed allowfullscreen="true" ...

[datalink]

I removed allowFullScreen from array on line 90:

names = [ 'allowFullScreen', 'play', 'loop', 'menu' ];

Now everyting is working fine.

[Artur Delura]

The array contains attributes which default to true. I cannot think right now about a case which removing the allowFullScreen for it will break, but change will be semantically incorrect and may lead to problems in the future (but immediate regression is probable too). Therefore we need a more straighforward solution which I proposed in ​branch:t/7634 and tests.

[Olek Nowodziński]

Rebased branches on latest master.

[Olek Nowodziński]

Tests

1. Why did you use 2 editors? What does the additional editor change since it has the same configuration as the first one? You should run both tests using the same editor instance, otherwise it's just a waste of precious time.

2. Regarding

   FLASH_URL = 'http://www.flashactionscripttutorials...
   

We must avoid external resources in general to preserve performance and stability. And above all we should avoid those resources which come from 3rd party providers of unknown status (unlike Google, jQuery, etc.). What if that resource is gone? What if someone runs tests offline? What if, at some point in the future, that resource will determine whether tests pass or not? What if that resource becomes something generally regarded as abusive or dangerous? We have no control over that thing. Use dt/_assets/sample.swf or some dummy string, i.e. foo.swf.

3. It would be nice if you mentioned in test name of "test param allowFullScreen present in code with true value" that this is default behaviour for the plugin.

4. I think that

   bot.editor.document.findOne( 'img' );
   

   would be shorter than

   bot.editor.document.getElementsByTag( 'img' ).getItem( 0 );
   

   in selectFirstFlashElementInEditor().

5. I think that

   CKEDITOR.dom.element.createFromHtml( bot.getData() );
   

   would be much simpler in parseEditorDataToDOM()

6. AFAIR dialog.selectPage( 'properties' ); is obsolete in your case.

[Olek Nowodziński]

As for dev code, I think that including additional code, which is not itself straightforward, to handle just one case is not a good idea. As long as the solution in comment:4 works, we should use it and leave an adequate comment to explain why it is so.

[Piotrek Koszuliński]

I was against the comment:4 since it's semantically incorrect, but the correct solution is indeed too long. I did not think that it will be so problematic. Better to choose the hacky, but short way + leave the comment and tests.

[Artur Delura]

Changes and tests in ​branch:t/7634b

[Olek Nowodziński]

Pushed a single commit to ​branch:t/7634b.

1. Path to asset is invalid ​https://github.com/cksource/ckeditor-dev/blob/5e73d807203fabc37d8990ee1293b03430f251af/tests/plugins/flash/fullScreen.js#L8

   http://tests.ckeditor.dev:1030/tests/plugins/flash/_assets/sample 404 (Not Found) 
   

2. Please refer to ​https://github.com/cksource/ckeditor-dev/blob/5e73d807203fabc37d8990ee1293b03430f251af/tests/_assets/sample.swf instead of ​https://github.com/cksource/ckeditor-dev/blob/5e73d807203fabc37d8990ee1293b03430f251af/tests/plugins/flash/_assets/sample.swf. The second one is not our property nor CKSource possess any rights to re-use that resource.
3. ​http://tests.ckeditor.dev:1030/tests/plugins/flash/fullScreen tests fail in IE8.

[Olek Nowodziński]

​git:e8e46f3 landed in master.