Opened 13 years ago

Closed 10 years ago

Last modified 10 years ago

#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 13 years ago by Jakub Ś

Status: newpending

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?

comment:2 Changed 13 years ago by datalink

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 13 years ago by Jakub Ś

Status: pendingconfirmed

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 10 years ago by datalink

I removed allowFullScreen from array on line 90:

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

Now everyting is working fine.

comment:6 Changed 10 years ago by 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.

Last edited 10 years ago by Artur Delura (previous) (diff)

comment:7 Changed 10 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

comment:8 Changed 10 years ago by Artur Delura

Status: assignedreview

comment:9 Changed 10 years ago by Olek Nowodziński

Rebased branches on latest master.

comment:10 Changed 10 years ago by Olek Nowodziński

Status: reviewreview_failed

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.
  1. 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.

  1. 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.
  1. I think that
    bot.editor.document.findOne( 'img' );
    
    would be shorter than
    bot.editor.document.getElementsByTag( 'img' ).getItem( 0 );
    
    in selectFirstFlashElementInEditor().
  1. I think that
    CKEDITOR.dom.element.createFromHtml( bot.getData() );
    
    would be much simpler in parseEditorDataToDOM()
  1. AFAIR dialog.selectPage( 'properties' ); is obsolete in your case.

comment:11 Changed 10 years ago by 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.

comment:12 Changed 10 years ago by 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.

comment:13 Changed 10 years ago by Artur Delura

Status: review_failedreview

Changes and tests in branch:t/7634b

comment:14 Changed 10 years ago by Olek Nowodziński

Status: reviewreview_failed
Last edited 10 years ago by Olek Nowodziński (previous) (diff)

comment:15 Changed 10 years ago by Olek Nowodziński

Owner: changed from Artur Delura to Olek Nowodziński
Status: review_failedassigned

comment:16 Changed 10 years ago by Olek Nowodziński

Resolution: fixed
Status: assignedclosed

git:e8e46f3 landed in master.

comment:17 Changed 10 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.4.2
Summary: flash dialog sets only false paramFlash dialog sets only false param
Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy