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)
SeaMonkey
Find In Page
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla1.1alpha
People
(Reporter: bugZ, Assigned: aaronlev)
References
Details
(Keywords: access, regression)
Attachments
(1 file)
This patch watches events DOMMenuBarActive, popupshown, etc. to determine when we're in menus or not
10.16 KB,
patch
|
aaronlev
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
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?
Comment 1•22 years ago
|
||
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
Assignee | ||
Comment 3•22 years ago
|
||
Seeking r=
Assignee | ||
Comment 4•22 years ago
|
||
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.
Assignee | ||
Updated•22 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.1alpha
Comment 5•22 years ago
|
||
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...
Assignee | ||
Comment 6•22 years ago
|
||
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.
Assignee | ||
Comment 7•22 years ago
|
||
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+
Assignee | ||
Comment 8•22 years ago
|
||
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 9•22 years ago
|
||
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+
Assignee | ||
Comment 10•22 years ago
|
||
checked in
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 11•22 years ago
|
||
win32 trunk build 2002090604, win98se WFM :-)
Comment 12•22 years ago
|
||
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
Updated•16 years ago
|
Product: Core → SeaMonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•