Opened 12 years ago
Closed 12 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 12 years ago by
Status: | new → review |
---|
comment:3 Changed 12 years ago by
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 12 years ago by
Status: | review → review_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 12 years ago by
@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 12 years ago by
Status: | review_failed → review |
---|
Now exposed via CKEDITOR.focusManager._.blurDelay.
comment:7 Changed 12 years ago by
Status: | review → review_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 12 years ago by
Status: | review_passed → review |
---|
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 12 years ago by
Resolution: | → fixed |
---|---|
Status: | review → closed |
Masterised (https://github.com/ckeditor/ckeditor-dev/commit/1f5529d1ff4c7f9bc37e68f53cea9a6aa4ba79b7) after minor docs fix.
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