Opened 7 years ago

Closed 7 years ago

#9224 closed Task (fixed)

[IE9/Opera] focus test passes randomly

Reported by: Olek Nowodziński Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 4.0
Component: General Version: 4.0
Keywords: Cc:

Description

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

Change History (9)

comment:1 Changed 7 years ago by Garry Yao

Status: newreview

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

comment:2 Changed 7 years ago by Piotrek Koszuliński

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

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

comment:4 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

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.

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

comment:6 Changed 7 years ago by Garry Yao

Status: review_failedreview

Now exposed via CKEDITOR.focusManager._.blurDelay.

Last edited 7 years ago by Garry Yao (previous) (diff)

comment:7 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

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

comment:8 Changed 7 years ago by Garry Yao

Status: review_passedreview

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.

comment:9 Changed 7 years ago by Piotrek Koszuliński

Resolution: fixed
Status: reviewclosed
Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy