Opened 10 years ago

Closed 10 years ago

#10267 closed defect (fixed)

[patch] [cla] dijit._Menu opens multiple times, hangs Firefox on Linux

Reported by: haysmark Owned by: bill
Priority: high Milestone: 1.5
Component: Dijit Version: 1.4.0b
Keywords: Cc:
Blocked By: Blocking:

Description

In Linux Firefox 3.5, run test_Menu and press shift+F10. You will see the context menu flash and cascade down the screen several times (at least twice if you are fast) as it opens and closes.

The root problem is that dojo.stopEvent in onkeydown cannot stop the oncontextmenu event from being created. The effect is that these two handlers compete to open the menu.

Worse, the oncontextmenu handler is particularly problematic because it synchronously opens the menu in the event dispatcher thread; Firefox repeats oncontextmenu events very rapidly, and with so much time elapsing while it opens the menu in the event dispatcher thread, Firefox sometimes never stops repeating events! The result is that you sometimes see the ContextMenus? endlessly cascading.

Attachments (2)

10267.patch (1.7 KB) - added by haysmark 10 years ago.
Fixes #10267. Pressing shift+f10 now opens a context Menu only once, instead of twice, per keydown on Linux.
linuxMenu.diff (6.5 KB) - added by bill 10 years ago.
patch against Menu.js 20690 with Mark's setTimeout() change plus some code cleanup

Download all attachments as: .zip

Change History (11)

comment:1 Changed 10 years ago by Douglas Hays

Owner: set to bill

comment:2 Changed 10 years ago by Douglas Hays

The patch looks reasonable and appears to fix the problem that causes Firefox use all the CPU on Linux. I'd like to better understand the difference between setTimeout and window.setTimeout that are used in the patch. Is 1 more performant than the other?

comment:3 Changed 10 years ago by bill

Summary: [patch] dijit._Menu opens multiple times, hangs Firefox on Linux[patch] [cla] dijit._Menu opens multiple times, hangs Firefox on Linux

I don't think there's any practical difference between setTimeout() and window.setTimeout(). One is 7 less characters :-).

But I'm confused... if the shift-F10 creates an oncontextmenu event then why do we call _openMyself() in the _contextKey() handler?

comment:4 Changed 10 years ago by Douglas Hays

The invocation in _contextKey is needed for Windows, but not Linux.

Changed 10 years ago by haysmark

Attachment: 10267.patch added

Fixes #10267. Pressing shift+f10 now opens a context Menu only once, instead of twice, per keydown on Linux.

comment:5 Changed 10 years ago by bill

Makes sense to do the timeout as a way to avoid the double events. I want to clean up menu though as over the years it's gotten spaghetti-ish, not just with this change but with the original timeout added for IE (and the faux event object) and even the keyboard vs. mouse support code (and that strange this. _contextMenuWithMouse variable). I'm attaching another patch (started from your patch) that does:

  • code to prevent double menu open on linux/FF due to shift-F10 causing both a keyboard event and an oncontextmenu event.
  • rework parameters to _openMyself() so it doesn't need the event object (because the event object is no longer valid after the setTimeout fires)
  • Remove code to check that left click was truly the left button, because it didn't have any effect: On IE onclick events always have button=0 regardless of which button is pressed (e.button does vary on mousedown events, but using IE's non-standard encoding for mouse buttons). On Mozilla/webkit, only a left click causes an onclick event. (see #5799).
  • get rid of this._contextMenuWithMouse by simply only passing coordinates to _openMyself() when there's a mouse position

I think it will still work on linux although I don't have a way to test it, can you try? I'm unclear though about which event comes first on linux, the onkeydown or the oncontextmenu. If the oncontextmenu comes first then need to rework the logic.

Changed 10 years ago by bill

Attachment: linuxMenu.diff added

patch against Menu.js 20690 with Mark's setTimeout() change plus some code cleanup

comment:6 Changed 10 years ago by Douglas Hays

Milestone: tbd1.5

dojox references need cleanup

comment:7 Changed 10 years ago by haysmark

This works for me, you should also look for dojox menu code because I believe people have copied and pasted menu code to work around this bug.

comment:8 Changed 10 years ago by bill

Thanks. I already updated TablePlugin in [20980] to remove some unneeded duplicated code. I didn't see any cut-and-pasted code in dojox besides that, but let me know if you see something.

I saw a few other references to _openMyself() in the code base so I decided to modify my patch so that _openMyself takes a single {target: DomNode, ...} parameter, somewhat similar to the event parameter it used to take. That's a less disruptive change because at least code that connects to _openMyself(e) and looks at e.target still works.

comment:9 Changed 10 years ago by bill

Resolution: fixed
Status: newclosed

(In [21019]) Menu fixes and refactor:

  • Code to prevent double menu open on linux/FF due to shift-F10 causing both a keyboard event and an oncontextmenu event.
  • Rework parameters to _openMyself() so it doesn't need the event object (because the event object is no longer valid after the setTimeout fires).
  • Remove code to check that left click was truly the left button, because it didn't have any effect: On IE onclick events always have button=0 regardless of which button is pressed (e.button does vary on mousedown events, but using IE's non-standard encoding for mouse buttons). On Mozilla/webkit, only a left click causes an onclick event. (see #5799).
  • Get rid of this._contextMenuWithMouse by simply only passing coordinates to _openMyself() when there's a mouse position.

Fixes #10267 !strict.

Note: See TracTickets for help on using tickets.