291099 - Using keyboard to navigate through context menus of menu items (e.g. bookmarks) skips every other item

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Robert Strong (they/them - no direct email)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050418 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050418 Firefox/1.0+

When using the keyboard to select bookmark context menu menuitems some menuitems
are often skipped. The closest bug to this I could find is bug 266076 but that
deals with accesskeys.

Reproducible: Always

Steps to Reproduce:
1. Click Bookmarks and context menu click a bookmark
2. Use the arrow keys to select menuitems

Actual Results:  
Some enabled menuitems are skipped

Expected Results:  
Each enabled menuitem is selected in turn

This bug appears for all bookmarks and bookmark folder in the Bookmarks menu. It
appears that if the context menu is brought up from the top of the Bookmarks
Toolbar for a bookmark or bookmark folder that it is possible to select each
enabled menuitem in turn using the arrow keys on the keyboard. If the bookmark
or bookmark folder is inside of another bookmark folder in the Bookmarks Toolbar
that this bug is exhibited.

Comment 1

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050419
Firefox/1.0+

Confirmed.

the Items are picked seemingly at random
very much depends on where the mousepointer is located

Comment 2

[Phil Ringnalda (:philor)]

Looks to have regressed somewhere between 20050117 and 20050127. And at least
what I'm seeing isn't random, it's just every other menu item gets skipped.

Comment 3

[Robert Strong (they/them - no direct email)]

I noticed it wasn't random as well just after filing. It seems as if the
keypress event is running twice for popups that are a contentmenu of a popup.

Comment 4

[Robert Strong (they/them - no direct email)]

Attachment: testcase

Comment 5

[Robert Strong (they/them - no direct email)]

This does not appear to be bookmarks specific though it does affect bookmarks.
Changing the summary, product, and component accordingly though I suspect the
category may still be incorrect.

Comment 6

[Robert Strong (they/them - no direct email)]

Reassigning to [e-mail] since this is not bookmarks specific to get it
off of Vladimir's plate.

Comment 7

[Phil Ringnalda (:philor)]

Aaron, could this be a regression from your bug 282438? The window is
2005-02-17-07 to 2005-02-18-07, and even though I don't know what I'm talking
about, yours looks the most likely.

Comment 8

[Boris Zbarsky [:bzbarsky]]

The regression range in comment 7 is off by a month (compare comment 2).  I see
the bug in a 2005-01-19-08 build but not a 2005-01-18-07 build.

So bug 282438 is definitely not it.

Bug 167145, on the other hand, is quite a likely culprit, since it messed around
with key events...

Comment 9

[Aaron Slunt]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050802
Firefox/1.0+ ID:2005080207

For me, the navigation moves in the opposite direction of the key i'm pressing.
i.e. I press up and it moves down.

Comment 10

[Robert Strong (they/them - no direct email)]

Aaron - it appears that way visually because it is skipping an item.

Comment 11

[Aaron Leventhal]

Attachment: Ignore key presses that have already been dealt with. There can be more than one listener on the same node.

Comment 12

[Aaron Leventhal]

Spin off bug 303404 is a crash I found while debugging this.

Comment 13

[[e-mail]]

Attachment: Don't attach multiple listeners

Comment 14

[Aaron Leventhal]

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

Comment 15

[Aaron Leventhal]

Neil's got a patch

Comment 16

[[e-mail]]

Fix checked in to the trunk.

Comment 17

[Omer Kehri]

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

Comment 18

[Asa Dotzler [:asa]]

Comment on attachment 191845 [details] [diff] [review]
Don't attach multiple listeners

I'm nervous about this change. We can't afford any regressions this late in the
game. What kind of testing has been done here and what kind of risk does this
patch pose?

Comment 19

[Asa Dotzler [:asa]]

neil, aaron, qa, have we verified that this doesn't break anything?

Comment 20

[Marcia Knous [:marcia]]

I will check this on Windows XP, but using today's Mac trunk build (Mozilla/5.0
(Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20051011
Firefox/1.6a1), the test case doesn't work. Using today's Mac Branch build, the
test case does work.

Comment 21

[Marcia Knous [:marcia]]

Just to clarify, when I mean the test case works, you can use the arrow keys to
move up and down without skipping.

(In reply to comment #20)
> I will check this on Windows XP, but using today's Mac trunk build (Mozilla/5.0
> (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20051011
> Firefox/1.6a1), the test case doesn't work. Using today's Mac Branch build, the
> test case does work.

Comment 22

[Marcia Knous [:marcia]]

This looks ok using today's Windows Trunk build (Mozilla/5.0 (Windows; U;
Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051011 Firefox/1.6a1). If I run the
test case, I can arrow through the choices with no problem. I also did some
context clicking on bookmarks and didn't see any problems.

Comment 23

[Scott MacGregor]

plussing since we decide to take this patch.

Comment 24

[Scott MacGregor]

Neil, can you get this checked into the branch soon? If not, let us know and we
can find someone who can land it (maybe me :)).

Comment 25

[[e-mail]]

Sorry, I overlooked the a=asa.