[IE9/Opera] focus test passes randomly

[Olek Nowodziński]

The problem is here: ​http://ckeditor4.t/dt/core/focus.html (test add focus targets)

If we consider the following example:

// Run on http://ckeditor4.t/ckeditor/samples/replacebycode.html

var e1 = CKEDITOR.instances.editor1;
var e2 = CKEDITOR.instances.editor2;

function assTrue( cnd ) { console.log( !cnd ? 'Nope!' : 'Passed.' ); }

for( var i = 0 ; i < 5 ; i++ )
{
	e1.focus();
	assTrue( e1.focusManager.hasFocus && !e2.focusManager.hasFocus );
	e2.focus();
	assTrue( !e1.focusManager.hasFocus && e2.focusManager.hasFocus );
}

and put some logs in focusManager.focus|blur(), we can observe that IE9 works synchronously (correctly) and Opera is the only, truly asynchronous one.

I guess that the "timeout-nesting-thing" in this test was initially to deal w/ Opera as i.e. Chrome passes it w/o any timeouts. For unknown reason, IE9 behaves worse than Opera if we deal w/ editor.focusManager.add( element ). The browser requires extended timeouts, e.g. if we set them to 300ms, it always passes this test. Otherwise, most likely depending on the lunar phase and the humidity of the air, it either passes or fails.

I guess that we can simply extend those timeouts to get rid of this problem as, IMO, it doesn't impact on a real-life usage. Unless someone got some idea, of course.

PS. MS says that onfocus/onblur events are async. in all their products which confuses me even more: ​http://msdn.microsoft.com/en-us/library/ie/ms536909(v=vs.85).aspx ​http://msdn.microsoft.com/en-us/library/ie/ms536934(v=vs.85).aspx

[Garry Yao]

This's more of correcting the way we wrote tests, than fixing anything, as wrote above.

Opened t/213@tests, invented CKTESTER.tools.focus(focusable,callback) as a solution for this issue among tests, adapted existing tests to this new API, so that tests will not anymore assume an synchronous fact when checking focus state (which noticeably breaks Opera and sometimes, IE ).

Small change on t/213@dev as well, where the 200ms hard coded blur timeout is now part of CKEDITOR.config, thus can be reset by tests environment to "0", eventually simplified test code, and can be manipulated by users who complained it's too short/longer a value.

I have all green received on core tests for all browsers, including Opera, so we should be ok:

​http://v4.ckeditor.t/cktester/index.html?tags=core

[Piotrek Koszuliński]

Rebased to current master and renamed branches to t/9224.

[Piotrek Koszuliński]

I'm not sure if exposing config.blurDelay makes some sense. This config setting is meaningless for other devs and I can't think of any reason why someone would want to change this value. Has someone really complained that it's too short?

Another thing is this, currently ignored, test:

'/dt/core/selection/editor.html#test "selectionChange" fired on editor focus' : 'env.opera'

It fails on Opera because of the same reason and maybe should be fixed too.

[Frederico Caldeira Knabben]

The comment:3 is to be considered.

While I understand that playing with the delay value is a way to solve the problem on tests, exposing this hack in the API as a configuration option is something we don't need to do.

No one wants the current 200ms delay. It's there just because we have no control on the order of things into browsers. Even the delay amount is something totally random, without any justification other than because we think it is ok.

So, instead of exposing the delay so clearly in the API, let's have it as a focusManager property with no public documentation (but still a internal // documentation that explain the reason why) and then abuse of it in the test.

[Piotrek Koszuliński]

@private tag may/should be used to document it. IMO it's better to document properly even those internal things. Just for us, because then we can open docs and easily find it there (even having only property name), without searching through the code. After we migrated to JSDuck I'm eventually using docs during development :).

Only one thing isn't fully right about @private - it has a little bit different meaning than what I meant by "internal". However, I wanted to propose that we extend its range of use, since we really rarely have something truly private.

[Garry Yao]

Now exposed via CKEDITOR.focusManager._.blurDelay.

[Frederico Caldeira Knabben]

The tests are failing, at least with Chrome.

Additionally, considering that we'll use focus(0 mainly on the editor, let's also expose a editorBot.focus( callback ) method, as a logical shortcut for the new CKTESTER.tools.focus( editor, callback ).

[Garry Yao]

Sorry...I've done wrong a change on dev (it should be a static field) that caused the error, and the editorBot#focus proposal is good.

[Piotrek Koszuliński]

Masterised (​https://github.com/ckeditor/ckeditor-dev/commit/1f5529d1ff4c7f9bc37e68f53cea9a6aa4ba79b7) after minor docs fix.