Fixing Menu keyboard navigation

[Pete]

While working on PopUpMenu, I noticed that there were several problems with menu keyboard navigation. These included the difficulties described in #4594, but are more extensive, so I'm filing a new ticket rather than extending that one. Hope that's the approved procedure. I'm attaching a patch which seems to fix everything, but please check it independently and extensively!

The bugs were:

1) Using Enter or Space to select an item would Invoke() an attached message, but never returned a pointer to the item if the app was waiting for it.

2) Sub-menus were not reachable via the left and right arrow keys. The _Track method would just hang in the loop that waited for mouse movement.

3) MenuBar suffered from problems similar to the above, causing #4594.

(1) was a result of fSelected being cleared before it could be returned to the caller. I solved this by using fChosenItem, which perviously was only needed by MenuBar.

(2) needed passing some extra MENU_STATE_... values around -- added to MenuPrivate.h.

(3) was cured by applying similar mods to MenuBar.

[Pete]

Well... maybe not quite there yet! Menus on Tracker windows and Terminal etc. seem to work as intended, but there are one or two apps that have trouble -- MediaPlayer in particular. In that, I can't seem to enter any menus but the first from the keyboard. Still, things are considerably better than they were.

[Pete]

With further testing, I found that ESC to cancel a menu had problems. It wouldn't work at the MenuBar level, and, if mixed with mouse navigation, would return the item even though it was supposedly cancelled.

I've revised the patch some more to make all that work. It seems to handle everything I throw at it now. (...except for MediaPlayer, where I still can't reach most menus from the keyboard. I'll blame the app for now...)

[leavengood]

Saw your email, sorry for the delay on this, it was off my radar and worse yet my Haiku development time has been non-existent for the past few months.

I still have some fixes to make to the menu code otherwise so I will go ahead and take ownership of this.

It may take me a bit to fully understand and test the patch though, so it is likely this won't make it into alpha3.

[Pete]

Replying to leavengood:

Saw your email, sorry for the delay on this, it was off my radar and worse yet my Haiku development time has been non-existent for the past few months.

Thanks for the fast response...!

I still have some fixes to make to the menu code otherwise so I will go ahead and take ownership of this. It may take me a bit to fully understand and test the patch though, so it is likely this won't make it into alpha3.

Understood. It's not something that I actually ever use myself, but I could see it being needed by others.

[stippi]

Since I am not on the ticket list anymore, I didn't see this before, sorry. I looked over the patch. While I can't confirm every detail to be correct, the changes do make sense to me. Of course the tracking code has not become less complex, but leaving keyboard navigation broken is much worse. The patch contains some coding style violations:

• The "else" goes on the same line as the closing parenthesis of the "if" block (several occurances).
• Comments should go on their own line, preferably before the line of code they refer to (several occurances).
• Never put the "if" or "else" statement on the same line as the clause (one occurance).

One minor coding style violations:

• Check for pointers using "if (pointer == NULL)" and "if (pointer != NULL)" instead of "if (!pointer)" and "if (pointer)". Makes for more readable code (sevaral occurances).

Thanks for the patch! Pete, are you going to fix the coding style issues? I can do it, but I have no time right now, perhaps Ryan takes care of it. In any case, the motivation is always much higher to apply and commit patches when only testing has to be done and no cleanup on top.

[Pete]

Replying to stippi:

Thanks for the patch! Pete, are you going to fix the coding style issues? I can do it, but I have no time right now, perhaps Ryan takes care of it. In any case, the motivation is always much higher to apply and commit patches when only testing has to be done and no cleanup on top.

Sure, I'll fix them. (I'm afraid it's still often hard for me to 'see' them, when I haven't really gotten familiar with the Guide yet. I'll have to try that Python Style Checker.)

Thanks.

[Pete]

Style fix of previous

[Pete]

Revised patch attached. I think I got all of my violations.

[stippi]

Thanks a lot, that looks close to perfect now. I only wonder what the "not if from R.Arrow" is trying to say exactly. :-) In any case, since I can use the break I'll checkout the patch in practice and will commit if I can't find any regression. Thanks for your work!

[stippi]

I've committed the latest patch in hrev42004. During my testing, I've found that there are still many remaining issues, plus one that may be a regression:

• When a particular BMenuItem is invoked for the first time via Enter, it doesn't have any effect, while subsequent invokations do work as expected. (regression?)
• Submenus should be allowed to open when the user uses the left Arrow key, and allowed to close, when the user uses the right arrow key. The reason being that sub-menus may open to either direction depending on available space. Since the user himself can't know which direction a submenu will open into, at least for opening both keys should work. For closing, it may be the key that matches the location of the submenu towards its parent.
• When hovering the mouse over a menu item, it still disturbs the keyboard navigation. This does not affect all submenus in a deeper hierarchy, however. I am not quite sure what exactly is the obervation here. Just try it with the Deskbar. When opening the Deskbar via the Menu key, navigating the entire hierachy works, when opening the menu via the mouse, not all submenus can be entered. I think submenus of the menu where the mouse is hovering are affected, but I am not sure.

In any case, thanks for your work. The above mentioned problems is why I think the whole tracking mechanism should be rewritten. The code is riddled with unforseen side-effects. The existance of two almost similar _Track() hooks in BMenu and BMenuBar makes it even worse.

[Pete]

Replying to stippi:

I've committed the latest patch in hrev42004. During my testing, I've found that there are still many remaining issues, plus one that may be a regression:

Many thanks for committing. Sorry for the slow response, but I've had a few other things to concern me... (:-/)

• When a particular BMenuItem is invoked for the first time via Enter, it doesn't have any effect, while subsequent invokations do work as expected. (regression?)

I don't see this. Can you give a forinstance? If I use Alt-Esc in, say, StyledEdit or Pe, then use the arrow keys to navigate to an item and hit Enter, the expected action occurs.

I do see aberrant behaviour in other apps, though. In MediaPlayer, for example, I can move around the top level menus, but I can only go down into the first one!

• Submenus should be allowed to open when the user uses the left Arrow key, and allowed to close, when the user uses the right arrow key. The reason being that sub-menus may open to either direction depending on available space.

Hmm. This could be a bit tricky. What happens if there is more than one level of submenu? If the first level can open to the right, say, but is hard up against the right screen-edge, then trying to use the left-arrow to go down another level is ambiguous, because the intention might actually be to go up to the previous level. I'd think it would be better to have the convention that right-arrow is always 'down', and left-arrow 'up'.

• When hovering the mouse over a menu item, it still disturbs the keyboard navigation. This does not affect all submenus in a deeper hierarchy, however. I am not quite sure what exactly is the obervation here. Just try it with the Deskbar.

There is definitely a bug or two somewhere here, but exactly where I'm not sure either. Not entirely sure that it's a mouse interaction either. I find that on occasion you can't get out of a submenu except with Esc and have to start over. (This happened even in my test app, which tries to be according to the book.)

In any case, thanks for your work. The above mentioned problems is why I think the whole tracking mechanism should be rewritten.

I suspect you're right...

[Pete]

It appears that something goes very wrong if the mouse is intermixed with K/B navigation at all. Something gets "out of sync", and once that happens you can't seem to get proper K/B control back until you restart the app. (I think this may be responsible for the problems you found.) I'll look into it.

[Pete]

Additional corrections to keyboard navigation (diff)

[Pete]

Sorry it took a while for me to get back to it, but I've made a couple of added revisions that I think fix all the remaining problems I could find. (Patch attached.)

Combining mouse and keyboard navigation was causing a lot of problems, mostly because the menu currently being tracked could be different from the one keys were manipulating! I think now there are sufficient safeguards to prevent anything nasty happening; no more lock-ups, anyway.

I noticed -- and fixed -- one other oddness. Menus with a single item could not be entered from the keyboard! I don't believe the autodecrement change I made can do any damage.

[stippi]

I have marked the notification for this attachement in my inbox as important, but I have no time for Haiku at the moment. I am making the comment just so you know that I am aware of the new patch and will try to make some time for it eventually.

[stippi]

I've looked at the patch, but I am not familiar with the code enough to have an opinion on your changes. You've removed some functionality and the related comment "Seems not needed and to work better without!" does not make me feel confident you really understood the purpose of the disabled code. Doing changes in this code without fully understanding it tends to produce unintended side effects. For example, at least in the Mouse preflet, no BMenuField works anymore by clicking once to open and clicking again to pick a menu item. So I don't feel confident in applying your newest patch while I don't understand this code fully myself and your comments don't suggest you fully understand this either. My solution to the problem would be to rewrite this whole thing properly, in an object oriented way, but I have no time. If you want to provide a patch for the one item menu problem specifically, that would be fine.

[Pete]

Replying to stippi:

I've looked at the patch, but I am not familiar with the code enough to have an opinion on your changes. You've removed some functionality and the related comment "Seems not needed and to work better without!" does not make me feel confident you really understood the purpose of the disabled code.

It's a while back now, but as I remember if I left that code in, the state could still get out of sync with the mouse. Removing it prevented that and I could find no undesirable effects.

Doing changes in this code without fully understanding it tends to produce unintended side effects. For example, at least in the Mouse preflet, no BMenuField works anymore by clicking once to open and clicking again to pick a menu item.

Hmm. I see that. However, it's not a result of this patch, because it also happens if I boot my vanilla alpha-3 stick! It's not apparent in an earlier partition (~40241), nor in one that just has my ealier popup menu patch. I suspect it came in with the first version of the keyboard navigation patch, which you applied in hrev42004, but I can't verify that.

Note though that the glitch seems exclusive to the Mouse preflet. I've tried a few others, like E-mail and Fonts, and the MenuFields work perfectly correctly in those. So the clash must be with that preflet itself.

So I don't feel confident in applying your newest patch while I don't understand this code fully myself and your comments don't suggest you fully understand this either.

I think that's probably true! (:-/) Is there anybody still around who does fully understand the menu code, though?

I suspect a regression to before hrev42004 wouldn't really help, because keyboard navigation was severely broken before. Though if nobody ever uses K/B navigation (I certainly don't) maybe having all mouse-menus work is more important for now.

My solution to the problem would be to rewrite this whole thing properly, in an object oriented way, but I have no time. If you want to provide a patch for the one item menu problem specifically, that would be fine.

I'm sure that;s the best ultimate solution, but I don't really want to get involved in the menus again right now either, I'm afraid.

[jackburton]

Replying to stippi:

I've looked at the patch, but I am not familiar with the code enough to have an opinion on your changes. You've removed some functionality and the related comment "Seems not needed and to work better without!" does not make me feel confident you really understood the purpose of the disabled code. Doing changes in this code without fully understanding it tends to produce unintended side effects. For example, at least in the Mouse preflet, no BMenuField works anymore by clicking once to open and clicking again to pick a menu item. So I don't feel confident in applying your newest patch while I don't understand this code fully myself and your comments don't suggest you fully understand this either. My solution to the problem would be to rewrite this whole thing properly, in an object oriented way, but I have no time. If you want to provide a patch for the one item menu problem specifically, that would be fine.

#1621 has an old patch of mine which reimplements BMenu tracking using the BView hooks. Could be adapted to work with current code. The only thing I couldn't manage to work correctly was BMenuFields, but since they aren't working correctly now, either, I would just go on with that route.