Closed Bug 166033 Opened 22 years ago Closed 22 years ago

Keyboard menu navigation (access keys) not working due to perf 'improvement' for typeaheadfind

Categories

(SeaMonkey :: Find In Page, defect, P1)

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.1alpha

People

(Reporter: bugZ, Assigned: aaronlev)

References

Details

(Keywords: access, regression)

Attachments

(1 file)

win32 trunk build 2002083108, win98se

When the keyboard is used to access menu items, the initial keyboard shortcut
does drop down the menu, but typing the letter for a particular menu item
initiates a "find in page" for that letter instead of initiating the menu item.
 Happens with a new profile or an existing one.

Steps to reproduce:
1. Create a new profile and start the browser (should go to default home page
http://www.mozilla.org/).
2. Press ALT+e.  The Edit menu drops down as expected.
3. Press the letter "e".  The expectation is that the prefs window opens. 
Actual results is that a search for the letter "e" is done on the page.

The same results occur regardless of which menu or menu item is chosen.  Using
the up/down arrow keys once a menu has dropped down works OK, so I guess they
are a temporary workaround.  It it hard to get used to them if you are
accustomed to using the letter shortcuts, though.

The 082814 build doesn't seem to have this problem.
Any relationship to the checkin for bug 161960?
Confirming with a current CVS build and Debian Linux. This bug was also already
confirmed by comment 189 in bug 30088.
Blocks: isearch
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 98 → All
Hardware: PC → All
Summary: keyboard menu navigation stuck on "find in page" → Keyboard menu navigation not working due to typeaheadfind
note that this wasn't broken in 2002082608, but it was broken in 2002090108 and
friday's build.
Keywords: access, regression
Summary: Keyboard menu navigation not working due to typeaheadfind → Keyboard menu navigation not working due to perf 'improvement' for typeaheadfind
One more thing, I also reorganized my helpers that attach and remove listeners
into doc listeners and window listeners - the doc listeners get remove and
reattached whenever the doc changes. The window listeners stay for the lifetime
of the window.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.1alpha
Comment on attachment 97672 [details] [diff] [review]
This patch watches events DOMMenuBarActive, popupshown, etc. to determine when we're in menus or not

This is more of a drive by than a full review, I am headed out in a second, but
I wanted to point out a glaring issue:

>Index: src/nsTypeAheadFind.cpp

>+  if (eventType.Equals(NS_LITERAL_STRING("DOMMenuBarActive"))) {
>+    mIsMenuBarActive = PR_TRUE;
>+  }
>+  if (eventType.Equals(NS_LITERAL_STRING("DOMMenuBarInactive"))) {
>+    mIsMenuBarActive = PR_FALSE;
>+  }
>+  if (eventType.Equals(NS_LITERAL_STRING("popupshown"))) {
>+    mIsMenuPopupActive = PR_TRUE;
>+  }
>+  if (eventType.Equals(NS_LITERAL_STRING("popuphidden"))) {
>+    mIsMenuPopupActive = PR_FALSE;
>+  }

eventType will never be equal to more than one of these at a time, if we enter
the first if, we shouldn't even bother with the rest.  Return early or use some
|else if| love.

More to come later...
I don't think that's a glaring issue. Sometimes people leave else's out to save
on code footprint. If you want else, I'll put in else.
Comment on attachment 97672 [details] [diff] [review]
This patch watches events DOMMenuBarActive, popupshown, etc. to determine when we're in menus or not

<caillon> +
<caillon> +  // Remove selection listener
<caillon> +  nsCOMPtr<nsISelectionPrivate> selPrivate =
<caillon> +    do_QueryInterface(mFocusedDocSelection);
<caillon> +  // remove us if we are a listener
<caillon> +
<caillon> I know you're moving that around, but can you drop the comment down a
line to match up with the code its actually referring to?
<caillon> other than that, and my earlier comment, r=me.
<caillon> (please add that on the bug for me?)
Attachment #97672 - Flags: review+
Okay, I have added the else's and moved the comment - seeking sr=
Since it will be hard to detangle the patch, I'm hoping to get sr= without
submitting a new patch with those tweaks.
Comment on attachment 97672 [details] [diff] [review]
This patch watches events DOMMenuBarActive, popupshown, etc. to determine when we're in menus or not

sr=bzbarsky
Attachment #97672 - Flags: superreview+
checked in
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
win32 trunk build 2002090604, win98se

WFM :-)
vrfy'd fixed on win2k and linux rh7.2 (mac doesn't use access keys), with
2002.09.24.08 mozilla trunk.
Status: RESOLVED → VERIFIED
Summary: Keyboard menu navigation not working due to perf 'improvement' for typeaheadfind → Keyboard menu navigation (access keys) not working due to perf 'improvement' for typeaheadfind
Component: Keyboard: Navigation → Keyboard: Find as you Type
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: