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: Piotrek Koszuliński Owned by: Piotr Jasiun
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 Piotr Jasiun

Status: newconfirmed

comment:2 Changed 3 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: confirmedassigned

comment:3 Changed 3 years ago by Piotr Jasiun

Fixed with tests and small clean up.

Changes in t/11793 and corresponding test branch.

comment:4 Changed 3 years ago by Piotr Jasiun

Status: assignedreview

comment:5 Changed 3 years ago by Piotrek Koszuliński

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

comment:6 Changed 3 years ago by Piotr Jasiun

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 Piotr Jasiun

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 Piotrek Koszuliński

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 Piotrek Koszuliński

Status: reviewassigned

comment:10 Changed 3 years ago by Piotr Jasiun

Status: assignedreview

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 Piotrek Koszuliński

Status: reviewreview_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 Piotrek Koszuliński (previous) (diff)

comment:12 Changed 3 years ago by Piotr Jasiun

Status: review_failedreview

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

comment:13 Changed 3 years ago by Piotrek Koszuliński

Status: reviewreview_passed

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

comment:14 Changed 3 years ago by Piotr Jasiun

Resolution: fixed
Status: review_passedclosed

Done and closed.

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