Opened 10 years ago

Closed 10 years ago

Last modified 7 years ago

#1838 closed Bug (fixed)

Context-menu doesn't aways dissapear after selecting an option

Reported by: Brian Klug Owned by: Martin Kou
Priority: Normal Milestone: FCKeditor 2.6
Component: UI : Context Menu Version: SVN (FCKeditor) - Retired
Keywords: Confirmed Firefox Review+ Cc:

Description (last modified by Alfonso Martínez de Lizarrondo)

Tested with Firefox:

  1. Load the nightly demo at
  2. Insert a table
  3. Right click in a cell, choose Table Properties
  4. Repeat #3 up to ten times (usually happens on fourth try?)

Observation: the right click menu doesn't go away after selecting Table Properties

Attachments (2)

1838.patch (2.0 KB) - added by Martin Kou 10 years ago.
1838_2.patch (2.1 KB) - added by Martin Kou 10 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 10 years ago by Alfonso Martínez de Lizarrondo

Description: modified (diff)
Keywords: Confirmed Firefox added
Version: SVN

Confirmed with Firefox, seems to work fine with the rest of browsers.

Instead of several tries, I've failed as soon as I clicked for example in the "copy" option (that was disabled), and then I couldn't make the context menu go away

comment:2 Changed 10 years ago by Alfonso Martínez de Lizarrondo

Milestone: FCKeditor 2.6

It's annoying, targeting for 2.6

comment:3 Changed 10 years ago by Alfonso Martínez de Lizarrondo

#1976 has been marked as dup

comment:4 Changed 10 years ago by Martin Kou

Owner: set to Martin Kou
Status: newassigned

comment:5 Changed 10 years ago by Brian Klug

We've gotten six bug reports about this at PBwiki, where we have the Beta deployed in our Beta :)

comment:6 Changed 10 years ago by Martin Kou

I've seen it happening a few times on Windows/Firefox2 (not once on Mac/Firefox2) today, but whenever I wanted to reproduce that, it just seems to not happen :/

There's definitely a problem here, but reproducing it consistently is kinda hard. This might take a few days to fix.

comment:7 Changed 10 years ago by Martin Kou

I think I've got better luck reproducing the issue when my mouse hovered above the "Cell", "Row" and "Column" before clicking on "Table Properties". But still I'm only getting the bug every 20 or 30 trials.

But I guess I've got the gist of the issue now, it seems there's a synchronization problem with the FCKPanel::_LockCounter variable when it's being incremented and decremented quickly from different events - the pattern of counter values I've got from the debug messages of the counter looks very much like the "too much milk problem" counter taught in threaded programming classes.

comment:8 Changed 10 years ago by Martin Kou

I still haven't solved the bug today (just doing tests takes a lot of time already). But now it seems it is not a synchronization issue on the FCKPanel::_LockCounter property now.

Some more tests today showed that whenever the bug occurred, FCKPanel::Lock() gets called 4 times while FCKPanel::Unlock() gets called only 3 times, leave FCKPanel::_LockCounter = 1 when the Table Properties item is clicked and thus the menu wouldn't close. Normally, both Lock() and Unlock() should get called only 3 times (1 time for each open/closing of the three submenus above "Table Properties"). I still haven't pinned down how come Lock() would get called the fourth time.

comment:9 Changed 10 years ago by Martin Kou

I think I've finally found out why the menus aren't closing and derived a fix for that. Although with this kind of bug it can never be 100% sure since I don't know whether it will appear again at the 1000th trial to reproduce it despite having the fix. Anyway, here's my theory on why it is happening, based on what I've seen in today's code traces:

If you look into FCKPanel::Show() you'll see there's a little closure called "resizeFunc()", which sets the flag FCKPanel::_IsOpened to true. It is not executed directly by FCKPanel::Show(), however. It is only executed with a setTimeout() with a 1ms delay. So there's a 1ms delay (probably much longer, as you know the JavaScript timer isn't very accurate) during which the _IsOpened state is desynced from the rest of the FCKDialog object's states.

Now what can be checking that _IsOpened flag during the 1ms window? My code trace today showed that FCKMenuBlockPanel::Show(), which is called when opening the Row, Cell, Column submenus, would check it by calling FCKPanel::CheckIsOpened(). What happened, at least during the code traces I've seen, was that FCKMenuBlockPanel::Show() may get called multiple times even for the same submenu while your mouse hovered above the submenu's parent item quickly. The first time it is called, the submenu's FCKPanel::Show() is called, a timer is set to run resizeFunc() and the parent menu's _LockCounter is incremented. The second time it is called, it finds that the submenu is still not opened (since the timer has not run resizeFunc() yet), and thus called FCKPanel::Show() for the submenu again. Which, subsequently, causes the _LockCounter of the parent menu to be incorrectly incremented by 1 again.

So the fix to this, would be to move the _IsOpened = true assignment out of the resizeFunc() closure and make it executed with the rest of the code in FCKPanel::Show(). This creates a potential problem, however, that it makes it possible for a submenu to appear after it's FCKPanel::Hide() had been executed during the 1ms window. So we need to take care of that case in FCKPanel::Hide() by clearing the timer used to execute resizeFunc().

Changed 10 years ago by Martin Kou

Attachment: 1838.patch added

comment:10 Changed 10 years ago by Martin Kou

Keywords: Review? added

Changed 10 years ago by Martin Kou

Attachment: 1838_2.patch added

comment:11 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

comment:12 Changed 10 years ago by Martin Kou

Resolution: fixed
Status: assignedclosed

Fixed with [1730].

Click here for more info about our SVN system.

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