Not quite sure what is happening but it looks like event.stopPropagation() applied to the TR rows is breaking core tabledrag functionality (and also any other js which has events applied in a TR element).

Removing it fixes the issue and the menu still seems to work just fine.

CommentFileSizeAuthor
#7 mouseengine_cleanup-1704338-7.patch2.67 KBmuhleder
#1 event_stopPropagation.1704338.2.patch540 bytesmuhleder

Comments

muhleder’s picture

muhleder commented
Status: Active » Needs review
StatusFileSize
new event_stopPropagation.1704338.2.patch540 bytes
donquixote’s picture

donquixote commented

Thanks for helping out on these things!
I did not find the time to take sufficient care of this module in D7.

Patches are always welcome.

donquixote’s picture

donquixote commented

I committed all your other patches to 7.x-1.x, but this one is in a separate branch now.
(7.x-1.x-dontStopPropagation)

I still need to do a bit of testing.
The js mouseover stuff is probably the most tricky part of the module. It underwent a lot of changes until I was happy with the performance etc. So I am not 100% sure which consequences this patch will have.

Also, there was an issue in Chromium on Linux, which is described here,
#1664920: Clicking a select element in the page opens the first dropdown submenu (Issue in Chrome/Chromium)
https://code.google.com/p/chromium/issues/detail?id=135109
(Probably totally unrelated, so I guess it won't be killed with this patch..)

muhleder’s picture

muhleder commented

Yup, I didn't really dig deep enough into that code to see what it was doing but it definitely breaks tabledrag functionality. The core menu item admin screens are broken for example at admin/structure/menu/manage/my-menu-name

One thing I did spot though was that the arguments provided didn't seem to match the arguments expected in mouseMove() for the simple and superfish engines.

dqx_adminmenu.js#531 and dqx_adminmenu.js#542

mouse_engine.mouseMove(event.pageX, event.pageY, isDifferentItem);

dqx_adminmenu.js#232 MouseEngine_simple

this.mouseMove = function($element, x, y) {

dqx_adminmenu.js#271 MouseEngine_superfish

this.mouseMove = function($element, x, y) {
donquixote’s picture

donquixote commented

I checked, and the event.stopPropagation() indeed needs to be removed.
The event is only attached to $('body').mouseMove(), so we don't need to worry about redundant calls on nested elements.
I introduced this technique for performance reasons, and to not have tons of event handlers in memory.

The d.MouseEngine_superfish and d.MouseEngine_simple are there for historic reasons. The idea was some kind of pluggability for the mouse engine, if only for debugging. In normal operation, the mouse engine is immediately replaced by d.MouseEngine_directional.
However, at some point I did change the "API", and since then the other "mouse engines" are only there fir decoration..

Any idea to clean this up?
For now I just put a comment in there with "TODO".

donquixote’s picture

donquixote commented

7.x-1.0-unstable3 has
- all the fixes you suggested
- a few more inline submenus, and some menu items renamed. E.g. "Appearance > List" is now "Appearance > List available themes"

Some feedback would be welcome, but better in a new issue :)

muhleder’s picture

muhleder commented
StatusFileSize
new mouseengine_cleanup-1704338-7.patch2.67 KB

The menu seems to work just fine if we take out the _simple and _superfish functions entirely, taking out those ~ 80 lines of code isn't going to make what's left more readable as well.

It's pretty simple change obviously but here's a patch anyway.

What might be good generally is a dqx_adminmenu.api.php file documenting the hooks.
http://stackoverflow.com/questions/4999138/drupal-are-hook-functions-in-...

Everyone here really likes the menu and we've been using it on our D7 site for a couple of months now, but it was pretty hard initially to work out how to use the hooks (for me at least).

donquixote’s picture

donquixote commented

Good idea with the documentation file.

Btw, would you be interested in co-maintaining? I have the feeling, I won't be able to give this module as much time as it deserves. The D7 version has been sitting in this rough-edge state for too long.

muhleder’s picture

muhleder commented

Hi there, yeah I could help co-maintain this. Won't have a bunch of time, but I do have a long term client that uses it so if we find any issues I can certainly fix them. It's actually been pretty good apart from these few issues we've gathered over the past couple of days. It's had a good couple of months testing by us.

donquixote’s picture

donquixote commented

Now have a look at the 7.x-2.x. A fishy surprise is waiting!