52033 - Can't select other menu items after using accesskey for submenu

Status: NEW

(Core :: XUL, defect, P5)

Description

[Blake Ross]

Build ID: just pulled

Steps to Reproduce:
(1) Click View in the browser.
(2) Press A

The Apply Theme menu opens as expected.

(3) Mouse over other items in the menu

Result: Note that the selection keeps jumping back to Apply Theme >. Maybe some 
kind of timer problem.  You can get out of it by mousing over an item in the 
Apply Theme submenu, at which point everything works normally again.  Also: 
this isn't specific to the Apply Theme menu, it happens with every submenu in 
the product.  

cc'ing dean

Comment 1

[Peter Trudelle]

Yup, reproduced in today's verification build on Win98.  Seems totally cosmetic
since you can still select other menu items, you just may not get the correct
highlight feedback.  Due to that, and since mixing mouse and keyboard access to
 menus is not a common case, ->future

Comment 2

[:deanis74]

The Apply Theme menu item only remains "active" until I cause another submenu to
pop out.  Then everything works as expected.

Does look like some kind of timer issue, though.  Kinda like the timer isn't
initialized if an access key opens a submenu.

Comment 3

[:deanis74]

I'm on this.  Taking it from pink (I'm sure he won't mind too much).

Comment 4

[:deanis74]

Really taking it this time.

Comment 5

[:deanis74]

Slight problem with my patch for bug 29400.  I have a patch for this.  
Re-assigning back to Blake for review and/or check-in.

Comment 6

[:deanis74]

Attachment: giddyup

Comment 7

[Blake Ross]

dean: nice!  i thought this might be related to that other bug.  r=blake, i'll 
check in once brendan approves.

Comment 8

[:deanis74]

Can pink approve?  Adding him.

Mike: There was a minor problem with my fix for bug 29400.  This patch addresses 
that.

Comment 9

[Brendan Eich [:brendan]]

a=[e-mail], with obligatory over-parenthesization of == in &&
complaint.

/be

Comment 10

[:deanis74]

phbbbttt

Comment 11

[Brendan Eich [:brendan]]

(Phbbbttt away, but if you fail to observe "when in Rome" in my code, I'll drop
your excess parentheses on your front lawn!  ;-)

/be

Comment 12

[:deanis74]

It's getting to be winter.  You don't want to visit Edmonton in the winter.

Really, though, don't you find it more readable?  I guess it's just a matter of 
style.

Comment 13

[Mike Pinkerton (not reading bugmail)]

dean? am I on crack here?

if (mCurrentMenu != mTimerMenu) {
  ....
  if ( ... && mCurrentMenu == mTimerMenu )

something tells me that _can't_ ever be true.

Comment 14

[:deanis74]

You may well be on crack, but I may be too.  So why the heck does this work?  Or 
doesn't it, and I was just hallucinating?

Comment 15

[Blake Ross]

Good news: I'm on crack too.  Got room for a third?

It's not just you, this works for me as well.  When I first looked at it I only 
saw it in diff form, not in the context of the file.  But afterwards I noticed 
what pink said, and assumed (foolishly, I guess) that mCurrentMenu or 
mTimerMenu somehow changed within the if...

Comment 16

[John Morrison]

The patch works on linux. Don't ask me why.

Comment 17

[Mike Pinkerton (not reading bugmail)]

man, it must me time for me to call it quits, find myself a little cabin in the 
woods, and raise alpacas in solitude. I can't figure out how the diff could make 
any difference to the code.

dean, you've got it applied, can you break on that line in the debugger and 
verify that they are in fact equal (using your newfound debugger-foo)?

Comment 18

[:deanis74]

Sure.  I'll do this tonight when I get home.  I think that Blake may be right - 
I seem to recall my thinking process being something along the lines of "I think 
that mTimerMenu changed..."  I'll have more a little later.

Comment 19

[:deanis74]

OK, I was definitely on crack.  This never reaches that "if" and, thus, it 
breaks what I fixed in 29400.  Unfortunately, I've on my way out for a couple of 
beers.  Assigning this to me for the weekend.

Comment 20

[:deanis74]

The patch I checked in is functionally equivalent to the first patch that I 
checked in for bug 29400.  ie. it breaks multi-level menus hard

Comment 21

[Mike Pinkerton (not reading bugmail)]

wait, you guys landed this?!?! ugh!

Comment 22

[:deanis74]

Just checked lxr and no, it hasn't been landed.

Comment 23

[:deanis74]

Oh, I see... I mentioned "the patch I checked in ".  Sorry, bad terminology.  I 
meant the patch that I attached.  Sorry for any and all heart attacks.  Mike, I 
may be with you on those alpacas.

At least it makes sense how this patch could address this bug - it breaks 
something else.

Comment 24

[:deanis74]

I know why this is happening, but my mind is too numb right now to figure out 
the solution.

What's happening is that when you use an access key to select a menu that opens 
a submenu, the first menu item in that submenu is selected, as opposed to using 
the mouse when the first item isn't selected.  So the loop that I wrote to fix 
bug 29400 that drills down to the lowest menu in the submenu chain to see if it 
has a selected menu item finds a selected menu item.

Note: This behavior only manifests when the other menu item you're selecting 
doesn't open a submenu.  That's because when a new submenu is being opened, 
KillPendingTimers() is called, which closes off the currently open submenu.

Comment 25

[Mike Pinkerton (not reading bugmail)]

removing keywords, we're back to square one.

Comment 26

[:deanis74]

Yeah, we are.  The problem is that the patch to bug 29400 and something to fix 
this can't exist in unison.  The reason boils down to that when you use an 
accelerator key to pop out a submenu, it automatically selects the first menu 
item in that submenu.  That's what the patch for 29400 checks for, because 
that's what the intent of the original source was.

Comment 27

[:deanis74]

Possible work-around: When moving the mouse, see if there's a submenu open.  If 
there is, make sure that there are no menu items highlighted.  This would cause 
behavior that is slightly astray from how Windows behaves, but it would be so 
minor that probably nobody would even notice.

And thinking through it, I'm pretty sure that this won't cause 29400 to regress.

Comments?

Comment 28

[:deanis74]

I have a better way to do this, which means re-doing everything that I did for 
29400.  But it will address both issues at once.  Gimme a few days.

Comment 29

[Blake Ross]

Dean, update?

Comment 30

[:deanis74]

OK, I was off on my "few days" guess.  Work turned busy and I haven't had much 
Mozilla time lately.  But I hope to work on a couple of bugs tomorrow.

Comment 31

[:deanis74]

For me to do what I want to, I need to be able to get the current position of 
the mouse on-demand.  I remember needing this a while back for another bug, and 
I don't think it's there.  Someone (saari?) told me that he and pink were going 
to be adding it.  Or maybe it's there now and I can't find it?

I'll document my idea here, anyway.  Basically, when the timer fires, check if 
the mouse is over another menu item that's at the same level.  If it is, close 
off the submenu.  If it's not, leave the submenu open.  After playing around, 
this is the best that I can describe how Windows submenus work.  This is 
probably the proper way to address bug 29400 as well.  We don't need to step 
into all of the submenus to find out what's active.  Where we fall apart is 
depending on whether a menu item is highlighted, which I don't think will cover 
all the bases.

My logic seems really simple, though.  Can someone tear it apart?

(moving to mozilla1.0 milestone for now)

Comment 32

[:deanis74]

Or maybe the popup menu can capture NS_MOUSE_ENTER and NS_MOUSE_EXIT, and keep 
some sort of a flag...?

Comment 33

[:deanis74]

I kinda like my last idea there.  If each nsMenuFrame has a flag as to whether 
the mouse is over it, then I think it should be fairly straight-forward to solve 
this bug, while at the same time rewriting the fix to bug 29400.

Mike, what do you think?

Comment 34

[:deanis74]

Attachment: changes to layout/xul/base/public and layout/xul/base/src

Comment 35

[:deanis74]

This patch implements what I pondered in my last few comments.  It also nicely 
re-writes my patch for bug 29400 and removes a lengthy comment.  The naming of 
the functions and variables may not be the greatest, but I'm open to 
suggestions.

There's a problem with this, but it's not my fault.  It seems that sometimes an 
extra NS_MOUSE_ENTER_SYNTH is getting fired after an NS_MOUSE_EXIT_SYNTH.  I 
usually see the behavior when digging into the View > Character Coding submenu.  
I left printfs in my code on NS_MOUSE_ENTER_SYNTH and NS_MOUSE_EXIT_SYNTH so 
that you can see it too.  This would be similar to what I saw when looking at 
bug 29401 (2000-04-04 19:06 comment).

This behavior doesn't surface too often, though, so this new implementation 
behaves much better wrt this bug than the current implementation.

Comment 36

[:deanis74]

Blake, care to check this one out?

Comment 37

[:deanis74]

I'm tired of the struggle to get my patches reviewed. --> default owner

Comment 38

[Vedran Miletic]

retargeting

Comment 39

[Sylvestre Ledru [:Sylvestre]]

Decreasing the priority as no update for the last 2 years on this bug.
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage 
about the priority meaning.