#7634 closed Bug (fixed)
Flash dialog sets only false param
Reported by: | datalink | Owned by: | Olek Nowodziński |
---|---|---|---|
Priority: | Normal | Milestone: | CKEditor 4.4.2 |
Component: | General | Version: | |
Keywords: | Cc: |
Description
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" />
Change History (16)
comment:1 Changed 14 years ago by
Status: | new → pending |
---|
comment:2 Changed 14 years ago by
OK, you are right. But allowfullscreen must be set to true, that it works.
<param name="allowfullscreen" value="true" />
and
<embed allowfullscreen="true" ...
comment:3 Changed 14 years ago by
Status: | pending → confirmed |
---|
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" ...
comment:4 Changed 11 years ago by
I removed allowFullScreen from array on line 90:
names = [ 'allowFullScreen', 'play', 'loop', 'menu' ];
Now everyting is working fine.
comment:6 Changed 11 years ago by
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.
comment:7 Changed 11 years ago by
Owner: | set to Artur Delura |
---|---|
Status: | confirmed → assigned |
comment:8 Changed 11 years ago by
Status: | assigned → review |
---|
comment:10 Changed 11 years ago by
Status: | review → review_failed |
---|
Tests
- 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.
- 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
.
- 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.
- I think that
bot.editor.document.findOne( 'img' );
would be shorter thanbot.editor.document.getElementsByTag( 'img' ).getItem( 0 );
inselectFirstFlashElementInEditor()
.
- I think that
CKEDITOR.dom.element.createFromHtml( bot.getData() );
would be much simpler inparseEditorDataToDOM()
- AFAIR
dialog.selectPage( 'properties' );
is obsolete in your case.
comment:11 Changed 11 years ago by
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.
comment:12 Changed 11 years ago by
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.
comment:13 Changed 11 years ago by
Status: | review_failed → review |
---|
Changes and tests in branch:t/7634b
comment:14 Changed 10 years ago by
Status: | review → review_failed |
---|
Pushed a single commit to branch:t/7634b.
- 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)
- 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.
- http://tests.ckeditor.dev:1030/tests/plugins/flash/fullScreen tests fail in IE8.
comment:15 Changed 10 years ago by
Owner: | changed from Artur Delura to Olek Nowodziński |
---|---|
Status: | review_failed → assigned |
comment:16 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
git:e8e46f3 landed in master.
comment:17 Changed 10 years ago by
Milestone: | → CKEditor 4.4.2 |
---|---|
Summary: | flash dialog sets only false param → Flash dialog sets only false param |
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?