Opened 7 years ago

Closed 7 years ago

#9848 closed Bug (fixed)

Failing command test after switching off selectionChange's errorProof flag

Reported by: Piotrek Koszuliński Owned by: Olek Nowodziński
Priority: Normal Milestone: CKEditor 4.0.2
Component: General Version: 4.0.1
Keywords: Cc:

Description

  1. Switch off errorProof flag on selectionChange event (see selection.js).
  2. One test is faling on: http://ckeditor4.t/dt/core/command/command.html

Hint: it doesn't fail when ran alone - http://ckeditor4.t/dt/core/command/command.html?name=test-command-states-in-detailssummary

Change History (7)

comment:1 Changed 7 years ago by Jakub Ś

Status: newconfirmed

comment:2 Changed 7 years ago by Olek Nowodziński

Owner: set to Olek Nowodziński
Status: confirmedassigned

comment:3 Changed 7 years ago by Olek Nowodziński

Status: assignedreview

Created branch with fix in t/9848@tests-v4.

There are two reasons for this issue:

  1. editorBot.setHtmlWithSelection() is noisy and makes selectionChange fire multiple times before the right HTML is set and the right selection is done. This distracts command's refresh callback and yields bad asserts since it calls refresh on selectionChange if contextSensitive: true. This is a bad testing method for delicate, selection-dependent features.
  2. Three tests use the same editor and, once the command is defined in one test with a refresh callback containing assertions, those assertions are checked in successive tests as well yielding errors.

In fact, the problem wasn't "test command states in details>summary" (first one) but "test "contextSensitive" property" (last one). I changed the way the command is tested and eventually removed it so it's refresh() isn't executed anymore in the future.

comment:4 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewassigned

Before considering the proposed workaround, I would ask to investigate if it is possible instead to fix selectionChange, to avoid "selectionChange fire multiple times before the right HTML is set".

comment:5 Changed 7 years ago by Olek Nowodziński

Status: assignedreview

Pushed new solution to t/9848b@tests-v4.

  • Fixed 1. from comment:3 by avoiding additional selectionChange because of editor.focus() in CKTESTER.tools.setHtmlWithSelection in IE>8 and other browsers. This change seems to not interact with other tests.
  • Ported some part of solution for 2. from previous branch.

This combo is much simpler and cleaner than t/9848@tests-v4.

comment:6 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

I'm assuming this fix is not needed for IE<9.

comment:7 Changed 7 years ago by Olek Nowodziński

Resolution: fixed
Status: review_passedclosed

Added info regarding IE versions. Fixed with 89814fa@tests-v4.

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