Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#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 6 years ago by Piotrek Koszuliński

Status: newconfirmed

comment:2 Changed 6 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: confirmedassigned

comment:3 Changed 6 years ago by Piotr Jasiun

Status: assignedreview

Changes in t/11153.

comment:4 Changed 6 years ago by Piotrek Koszuliński

Status: reviewreview_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 in reply to:  4 Changed 6 years ago by Piotr Jasiun

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 6 years ago by Piotrek Koszuliński

  1. 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.
  1. 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.
  1. 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 6 years ago by Piotr Jasiun

Status: review_failedreview

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 6 years ago by Piotrek Koszuliński

Resolution: fixed
Status: reviewclosed

Fixed on master with cebb120 on tests. Those tests are poor, but it's not a time to rewrite them (#11254).

Last edited 6 years ago by Piotrek Koszuliński (previous) (diff)
Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy