#11153 closed Bug (fixed)
Error thrown in focusManager test
Reported by: | Piotrek Koszuliński | Owned by: | Piotr Jasiun |
---|---|---|---|
Priority: | Normal | Milestone: | CKEditor 4.3.1 |
Component: | General | Version: | 4.3 Beta |
Keywords: | Cc: |
Description
http://ckeditor4.t/dt/core/focusManager/focus.html
Uncaught TypeError: Cannot call method 'getType' of null
First bad commit (git:ddf9074ad).
ddf9074ad04d18d5ba6a8dcc1a9162936e455bc2 is the first bad commit commit ddf9074ad04d18d5ba6a8dcc1a9162936e455bc2 Author: Piotrek Reinmar Koszuliński <pkoszulinski@gmail.com> Date: Mon May 13 11:56:42 2013 +0200 Return null from getSelection when not in wysiwyg mode.
Change History (8)
comment:1 Changed 11 years ago by
Status: | new → confirmed |
---|
comment:2 Changed 11 years ago by
Owner: | set to Piotr Jasiun |
---|---|
Status: | confirmed → assigned |
comment:3 Changed 11 years ago by
Status: | assigned → review |
---|
comment:4 follow-up: 5 Changed 11 years ago by
Status: | review → review_failed |
---|
I've got strong doubts if that test is good. No one should lock selection when editor is not in a correct mode, and I see that this test tries to mock editor incorrectly, without setting range. Or executes a similar, incorrect scenario.
Please investigate it from the other side. Editor should not fail quietly if it's used incorrectly, and it seems that it is.
comment:5 Changed 11 years ago by
Replying to Reinmar:
I've got strong doubts if that test is good. No one should lock selection when editor is not in a correct mode, and I see that this test tries to mock editor incorrectly, without setting range. Or executes a similar, incorrect scenario.
Please investigate it from the other side. Editor should not fail quietly if it's used incorrectly, and it seems that it is.
It was my first idea (to investigate test workflow and make a fix in test, not in dev project).
But then I realized that lockSelection
works incorrectly. lockSelection
returns false
if locking the selection was not possible because of wrong type of selection, what is good. But I think that it should also return false
if there is no selection (i.e. because editor is in wrong editor mode) instead of throwing error.
With this fix test do not show any error. Because there is a mock instead of real editor there is no mode nor editable so lockSelection
will not work, but this is not what this test (focusManager
) should check.
comment:6 Changed 11 years ago by
- That test creates an editor without editable and plays with focus. This is completely unrealistic case and we found it because
lockSelection
fails. Making it silently fail in this case would hide that issue and eventually more of them in the future.
- There's a difference between no selection (which happens when there's no editable or we're not in wysiwyg mode) and selection of a type none.
- We should first fix the test and then see (e.g. based on some other real life scenarios or if corrected test will still fail) if we need that condition in
lockSelection
.
comment:7 Changed 11 years ago by
Status: | review_failed → review |
---|
There was missing clean up after first test. I've added 'editor.delete()' and now everything is fine. You can check test branch t/11153 with dev master.
comment:8 Changed 11 years ago by
Resolution: | → fixed |
---|---|
Status: | review → closed |
Fixed on master with cebb120 on tests. Those tests are poor, but it's not a time to rewrite them.
Changes in t/11153.