Opened 3 years ago

Closed 3 years ago

#11793 closed Bug (fixed)

Drop down is not "on" when clicking it while editor is blurred

Reported by: Reinmar Owned by: pjasiun
Priority: Normal Milestone: CKEditor 4.4.1
Component: General Version: 4.4.0
Keywords: Cc:

Description

  1. Open replacebycode sample.
  2. Click styles drop down without focus editor.
  3. Drop down opens but button is "off".

Reproduced on Chrome and IE11, should be producible on all browsers.

First bad commit git:094859c.

Change History (14)

comment:1 Changed 3 years ago by pjasiun

  • Status changed from new to confirmed

comment:2 Changed 3 years ago by pjasiun

  • Owner set to pjasiun
  • Status changed from confirmed to assigned

comment:3 Changed 3 years ago by pjasiun

Fixed with tests and small clean up.

Changes in t/11793 and corresponding test branch.

comment:4 Changed 3 years ago by pjasiun

  • Status changed from assigned to review

comment:5 Changed 3 years ago by Reinmar

Couldn't we simplify it? I pushed branch:t/11793b. What do you think about it?

comment:6 Changed 3 years ago by pjasiun

Well, in your solution there is if condition, then ternary operator which set status and then another if which may change this status. In my there is one switch with 4 cases. In my opinion one big switch is simpler, but it's a matter of taste.

comment:7 Changed 3 years ago by pjasiun

And I'm not sure if you solution works in the best way, because if editor is readOnly the status should be DISABLED even if it was ON. I know that is very unlikely to change editor to the readOnly mode when the dropdown is open, but I think that it is the good way to check if the editor is in readOnly before we check if we should leave combo in ON state.

comment:8 Changed 3 years ago by Reinmar

The branch:t/11793b adds an early return, which I think is very clear in this case. There's the validation if the function should be executed and then its the logic (not touched by this branch). Additionally, the early return saves some time, because e.g. in t/11793 setState and setValue are called even though none should be changed (if combo is ON).

Regarding the read only case - test it ;). If you have opened combo and something sets the read only to true, the combo is still opened, so the state should be on. The state may only be changed when combo is closed (although the best solution is to close the dropdown but this is out of this ticket's scope). So the t/11793b works ok, when t/11793 works incorrectly.

BTW. A test would be welcomed.

comment:9 Changed 3 years ago by Reinmar

  • Status changed from review to assigned

comment:10 Changed 3 years ago by pjasiun

  • Status changed from assigned to review

I tested you your solution and it works fine. I moved tests from t/11793 and simplified them using bot.combo. Changes in t/11793b and corresponding test branch.

Regarding the read only case - I think that it is a different problem, not related with this ticket, and the scenario is so uncommon (someone have to change readonly mode with open combo) that it has pretty low priority.

comment:11 Changed 3 years ago by Reinmar

  • Status changed from review to review_failed

Details, details, details.

  1. The test file is titled 'Plugin: stylescombo'.
  2. Uppercase letter is used in file name.
  3. The test file subtitle 'update state' is unclear. What is it supposed to contain? Note also that the readonly.html also test... updating state. I think that one file containing both tests, called state.html will make more sense.
  4. Test case title 'test clicking while blurred' is unclear too. It sounds like the button is blurred, when it means that editor has to be blurred.
Last edited 3 years ago by Reinmar (previous) (diff)

comment:12 Changed 3 years ago by pjasiun

  • Status changed from review_failed to review

I merged both tests into state.html and updated tests descriptions. Changes in 1/11793b test branch.

comment:13 Changed 3 years ago by Reinmar

  • Status changed from review to review_passed

Remove the //]]> comment from test file before closing branch.

comment:14 Changed 3 years ago by pjasiun

  • Resolution set to fixed
  • Status changed from review_passed to closed

Done and closed.

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