38425 - Using accelerator in context menu causes highlight upon reopening menu

Status: VERIFIED WORKSFORME

(Core :: XUL, defect, P3)

Description

[Blake Ross]

Build ID: 20000050608

Please resummarize - it wasn't easy coming up with a clear one for this bug.

Steps to Reproduce:

(1) Go to any page (i.e. http://www.mozilla.org)
(2) Right-click on plain text to bring up the context menu.
(3) Press L (the accelerator for Select All) to select all text, as expected.
(4) Right click again [on plain text]

"Select All" is now by default highlighted.  It will remain highlighted until 
you pass over it and then something else (with the cursor)

Comment 1

[sairuh (rarely reading bugmail)]

i wonder if this is the same thing as bug 23698? (ie, the end result is the
same.)

Comment 2

[Mike Pinkerton (not reading bugmail)]

cute.

Comment 3

[Peter Trudelle]

Mass-moving all M20-M30 XPToolkit bugs to Future

Comment 4

[Blake Ross]

reassigning to myself

Comment 5

[sairuh (rarely reading bugmail)]

*spam*: transferring current XP Menu bugs over to jrgm, the new component owner.
feel free to add me to the cc list (unless am the Reporter) of any of these, if
you have any questions/etc.

Comment 6

[Blake Ross]

accepting

Comment 7

[:deanis74]

I was actually looking for bug 23698 - I knew it was out there - and I found 
this one.  Blake, want a hand on this?  Should be pretty simple.  (Famous last 
words...)

As an aside, do this for a bunch of menu items.  Right now I have Back, Forward, 
Reload, Save Page As, Select All, and Copy all highlighted when I right-click.  
Funky.

Comment 8

[Blake Ross]

Dean: sure, if you don't mind.  To be honest, I haven't even begun to look at 
this because my building process is currently in shambles (should be remedied 
when my new computer comes in a week or two...)  But if you want to get started 
before then, be my guest.  Thanks.

Comment 9

[:deanis74]

Sure, assign it over to me if you want to.  Then it'll be easier for me to keep 
track of.

Comment 10

[Blake Ross]

k -- reassigning

Comment 11

[:deanis74]

Attachment: patch attached

Comment 12

[:deanis74]

I've attached a patch that fixes this, although I'm not sure if this is the 
right way or if I should delve into DismissChain().  Anyone care to comment?

Comment 13

[:deanis74]

Oh.  Mebbe I should accept this.

Comment 14

[:deanis74]

Blake, any thoughts on this?

Comment 15

[:deanis74]

*** Bug 51236 has been marked as a duplicate of this bug. ***

Comment 16

[Blake Ross]

I'll try your patch when I get a minute.  By the way, I notice also that using 
the View | Apply Theme menu to dynamically change themes doesn't clear the 
selected theme item in that menu (when you reopen the submenu, the item is 
still selected, same problem as this bug reports).  Probably related.

Comment 17

[:deanis74]

Nope, my patch doesn't affect this.  I think this is a different problem.  Using 
the new Modern skin, I see that after selecting a theme from the Apply Theme 
menu, the View menu on the menubar is still depressed on mouseover.  Did that 
make sense?  Maybe not.  Try these steps:

1. Open the View > Apply Theme menu.
2. Select Modern
3. Mouse over the top-level menu items in the menu bar.

"View" is depressed while all the other items look normal.  Almost like 
something isn't processing through to completion.

Comment 18

[:deanis74]

Filed this behavior as bug 51996.  I've done a bit of digging, and there's a 
little more info in the description of that bug.

Comment 19

[:deanis74]

Blake, hold off on this.  Once I get a build up-and-running (finally switching 
from optimized to debug -- got tired of printf), I'm going to go down the 
nsMenuPopupFrame::DismissChain path.  Should be a no-brainer, and not a kludge 
like I'm starting to feel my current patch is.

Comment 20

[:deanis74]

And, as per usual, I'm having a helluva time getting a build up-and-running.  
I'm sure you'll hear more from me before this process is done.

Comment 21

[:deanis74]

Attachment: Oh yeah, this one's much better (meaning, it's not a hack).

Comment 22

[:deanis74]

Nominating rtm (I know, slim chance) and adding pink to the cc list in hopes of 
a quick review.

Comment 23

[Blake Ross]

This will never ever ever ever get rtm++.  PDT is on a vendetta ;-)  Removing 
the keyword because I'm 100% sure (dataloss/crashing bugs ONLY), but you can 
readd it if you'd like...I would like to see this get into the trunk soon, 
though.

Comment 24

[:deanis74]

Fine with me - I had to try, though!  How does this get into the trunk?  Still 
needs a review, but then can you add it?

Comment 25

[Blake Ross]

It needs a review, which Mike should provide, and then a super review 
(approval), which someone at http://www.mozilla.org/hacking/reviewers.html 
should provide, presumably brendan for lack of a better choice. Then I can 
check it in.

Comment 26

[selmer (gone)]

rtm-, no harmful side-effect whatsoever.  Please get the patch into the trunk 
though.

Comment 27

[:deanis74]

milestone --> mozilla1.0
remove "rtm" keyword

Comment 28

[:deanis74]

Brendan or Hyatt, care to review?

Comment 29

[:deanis74]

Sorry, should have checked a recent build first.  This no longer happens.  
Marking WFM.

Comment 30

[Mike Pinkerton (not reading bugmail)]

looks reasonable. r=pink.

Comment 31

[:deanis74]

Verifying WFM. Build 2001021304