Ticket #7634 (closed Bug: fixed)

Opened 3 years ago

Last modified 2 months ago

Flash dialog sets only false param

Reported by: datalink Owned by: a.nowodzinski
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

comment:1 Changed 3 years ago by j.swiderski

  • Status changed from new to pending

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 3 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 3 years ago by j.swiderski

  • Status changed from pending to 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 4 months ago by datalink

I removed allowFullScreen from array on line 90:

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

Now everyting is working fine.

comment:6 Changed 4 months ago by a.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 4 months ago by a.delura (previous) (diff)

comment:7 Changed 4 months ago by a.delura

  • Status changed from confirmed to assigned
  • Owner set to a.delura

comment:8 Changed 4 months ago by a.delura

  • Status changed from assigned to review

comment:9 Changed 3 months ago by a.nowodzinski

Rebased branches on latest master.

comment:10 Changed 3 months ago by a.nowodzinski

  • Status changed from review to review_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 3 months ago by a.nowodzinski

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 3 months ago by Reinmar

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 3 months ago by a.delura

  • Status changed from review_failed to review

Changes and tests in branch:t/7634b

comment:14 Changed 3 months ago by a.nowodzinski

  • Status changed from review to review_failed
Last edited 3 months ago by a.nowodzinski (previous) (diff)

comment:15 Changed 3 months ago by a.nowodzinski

  • Owner changed from a.delura to a.nowodzinski
  • Status changed from review_failed to assigned

comment:16 Changed 3 months ago by a.nowodzinski

  • Status changed from assigned to closed
  • Resolution set to fixed

git:e8e46f3 landed in master.

comment:17 Changed 2 months ago by Reinmar

  • Summary changed from flash dialog sets only false param to Flash dialog sets only false param
  • Milestone set to CKEditor 4.4.2
Note: See TracTickets for help on using tickets.
© 2003 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy