press letters to select item in XUL menu, menulist

VERIFIED FIXED in mozilla1.0

Status

()

Core
Keyboard: Navigation
P3
major
VERIFIED FIXED
17 years ago
16 years ago

People

(Reporter: Jesse Ruderman, Assigned: Aaron Leventhal)

Tracking

({access})

Trunk
mozilla1.0
x86
All
access
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt3] - very nice to have)

Attachments

(1 attachment, 10 obsolete attachments)

(Reporter)

Description

17 years ago
Noticed while investigating bug 92451:

1. Edit > Prefs > Appearance > Fonts.
2. Focus the Serif drop-down listbox.
3. Press 'm' on the keyboard.
Result: nothing happens
Expected: the first option with 'm', "MS Serif", should be selected.

Pressing 'm' multiple times should cycle through all the items starting with
'm', in order, even if the items aren't adjacent in the listbox.  This should
apply to both listboxes and drop-down lists.

See also bug 42858, automatically assign mnemonics to items in Bookmarks menu.
(Reporter)

Comment 1

17 years ago
By the way, in native Windows drop-down lists, the keys work both when the list
is dropped down and when it's collapsed, and don't affect the dropped/collapsed
state.
(Assignee)

Updated

17 years ago
Keywords: access
(Assignee)

Comment 2

17 years ago
In most Windows lists:

If you press the same letter twice or more, it goes to the nth item that starts
with that letter. 
If you press several unique letters, it goes to the first item that starts with
that sequence.
If you press backspace it undoes the last press.
Assignee: aaronl → jaggernaut

Comment 3

17 years ago
font lists and things that have too much commonality have been given a 
different behavior by most vendors, I use wordperfect which lets me type more 
than one char to try to better complete a font name. I recall the others do 
too.

this was discussed in another bug. I can't find it right now, dean?

Comment 4

17 years ago
timeless, bug 67952.

There are two different keyboard behaviors for combo boxes / list boxes in
Windows, and it boils down to whether the text is sorted or not.  If the
combo/list box has the sort style bit on, incremental searching such as timeless
described is available.  If it is not, then you can only search using the first
letter of the word as Jesse described.


(Assignee)

Comment 5

17 years ago
Dean, is it correct that in the sorted case, where incremental searching works,
that repeated keystrokes will also work to move to consectuive items that begin
with the same character?

Also, I believe that a timer is used - if the user hasn't pressed a key for a
while, the buffer holding their current search characters is cleared.

Comment 6

17 years ago
Right on both accounts, although sometimes I find that timer interminably long!

Comment 7

17 years ago
Dupe of bug 64157.

*** This bug has been marked as a duplicate of 64157 ***
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → DUPLICATE

Comment 8

17 years ago
Oops, no it's not.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Target Milestone: --- → mozilla1.0

Updated

17 years ago
Severity: minor → major
(Reporter)

Comment 9

17 years ago
I don't think there should be a timer.  I might be reading other options that 
start with the letter I pressed.

See also bug 79035, the same bug for HTML form controls.

Comment 10

17 years ago
Comment I posted earlier for those not subscribed to bug 79035:

Please note- this is not the same bug.  This bug [ bug 79035 ] involved
autocompleting for the drop down boxes, not just jumping to an item based on the
first letter.

---

Steps to Reproduce:
Click a drop down box with countries listed, type "C" and try to find Canada.

Actual Results:  Repeatedly hitting "C" goes through all the C countries

Expected Results:  It should pop up a box containing entries like:

Cambodia
Canada
Columbia

etc...

Hitting the next key should narrow the box down again.

This is a feature not available in any browser I've seen so far - it would
definately be a good way to set Mozilla apart.



(Assignee)

Comment 11

17 years ago
For accessibility reasons I recommend be consistent with the platform widgets'
keyboard UIs.

Comment 12

17 years ago
How does the upcoming introduction of ListBox affect this bug?  Keyboard 
navigation in select lists is critical for the success of XUL as an application 
development language.

Comment 13

17 years ago
*** Bug 92451 has been marked as a duplicate of this bug. ***

Comment 14

17 years ago
-> hewitt (thanks man!)
Assignee: jaggernaut → hewitt
Status: REOPENED → NEW
Target Milestone: mozilla1.0 → ---

Updated

17 years ago
Severity: major → enhancement
Status: NEW → ASSIGNED
Priority: -- → P3
Summary: press first letter to select item in xbl drop-down or listbox → press first letter to select item in outliner, menulist, and listbox
Target Milestone: --- → mozilla0.9.9

Comment 15

17 years ago
*** Bug 110444 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 16

17 years ago
*** Bug 110586 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 17

17 years ago
major accessibility bug!
Severity: enhancement → major

Updated

17 years ago
Target Milestone: mozilla0.9.9 → mozilla1.0
(Assignee)

Updated

17 years ago
Keywords: nsbeta1
(Reporter)

Updated

16 years ago
Blocks: 64656

Comment 18

16 years ago
nsbeta1- per Nav triage team.  Nice to have, but AaronL sez this is not required
for US Section 508 guidelines.
Keywords: nsbeta1 → nsbeta1-

Updated

16 years ago
Target Milestone: mozilla1.0 → mozilla1.2
(Assignee)

Updated

16 years ago
Depends on: 127812

Updated

16 years ago
No longer depends on: 127812

Updated

16 years ago
Blocks: 127812

Comment 19

16 years ago
nsbeta1+ per ADT triage team, ->aaronl/1.0
Assignee: hewitt → aaronl
Status: ASSIGNED → NEW
Keywords: nsbeta1- → nsbeta1+
Target Milestone: mozilla1.2 → mozilla1.0

Comment 20

16 years ago
fwiw w2k listboxes show the cursor as you incrementally search across a listbox, i'll try to provide pictures and logic analysis from home.

Comment 21

16 years ago
timeless: i'd make that a new bug, dependent on this one.

Comment 22

16 years ago
Created attachment 74021 [details] [diff] [review]
patch


I've mainly implemented the keyboard navigation with such features:

1. If you press one letter, the first item that starts with that letter
   will be selected;
2. If you press one letter repeatly, the selection will go circularly within
   the all items that starts with that letter;
3. If you press several unique letters, the selection will go to the first 
   item that starts with that sequence.

Actually, this nav feature will be turned on only when the widget hasn't 
any shortcut keys. 

The patch implemented this feature only for the XUL-based popup-menu and
drop-down 
combobox. HTML-based listbox and combobox already have their own key-nav
feature.

And also, this is a big patch which consists of 10 files, because I need pass
the
nsIDOMKetEvent parameter to the navigation function to let it can measure the
interval
of two typings.
(Assignee)

Comment 23

16 years ago
Kyle, the behavior sounds almost correct.

> Actually, this nav feature will be turned on only when the widget hasn't 
any shortcut keys. 

We should remove that condition - it should work when there are other shortcut keys.

Look at the Start menu in windows or Favorities in Internet Explorer. You may
have other items that start with the same letter as one of the menu item's
accesskey. Pressing a letter multiple times will cycle through all the items
that use that accesskey plus all the items that start with that letter. We need
the same thing to work for our Bookmarks menu, for example.

Can you do this too?

Comment 24

16 years ago
OK, I should adjust the nav rule as following:

assume user press 'A', and:
1. the list has neither shortcut key 'A', nor first letter 'A' 
   -- do nothing
2. the list has only one item which has either shortcut key 'A' or first 
letter 'A' 
   -- select it and do its action
3. the list has more than one item that has either shortcut key 'A' or first 
letter 'A' 
   -- go circularly within them but do not act them

For condition 3, we should also support incremental typing (IE's Favorities 
does not support).

Comment 25

16 years ago
I also found that in a normal Windows application popup menu, if its items have 
both shortcut 'A' and first letter 'A', when user press 'A', it always execute 
the item with shortcut 'A'.

(Reporter)

Comment 26

16 years ago
You should only look at the element's first letter if the element doesn't have a
mnemonic specified.  That would be consistent with how IE's favorites menu
works, and it would avoid breaking menus where all the mnemonics are specified.

Comment 27

16 years ago
*** Bug 67952 has been marked as a duplicate of this bug. ***

Comment 28

16 years ago
Created attachment 74264 [details] [diff] [review]
patch 2


I use the new rules except the condition 2. I adjusted this rule to:
only do action when the selected item has shortcut key.

Updated

16 years ago
Attachment #74021 - Attachment is obsolete: true

Comment 29

16 years ago
I prefer the "original" #2 to the "new" #2.  If an item doesn't have an
accesskey explicitly defined, at least in Windows, the first letter is assumed
to be the accesskey.  Thus "the list has only one item which has either shortcut
key 'A' or first letter 'A'" becomes the same thing, a list with only one item
with an accesskey of 'A'.

Comment 30

16 years ago
Hi, Aaron

I reposted the patch, can you take a review?
(Assignee)

Comment 31

16 years ago
Changing summary, to more accurately reflect what this will do.
(The user can also press several letters to narrow down to the item that starts
with those letters).
Summary: press first letter to select item in outliner, menulist, and listbox → press letters to select item in outliner, menulist, and listbox

Comment 32

16 years ago
I would like to see comment 29 addressed.

Comment 33

16 years ago
adding dave and joe for r/sr on this bug. Already sent them an email requesting
the reviews.

Comment 34

16 years ago
I don't think this is ready for r=/sr= until I at least get a response to
comment 29.

Comment 35

16 years ago
Hi, Dean

Sorry for late response. I think your concern is "whether do action
when a sole item was selected by key-navigation". I think if it is 
a menu item which shortcut key matched the key press, no doubt it 
should be acted. But if it is in a combo-box list, user should prefer
continuing navigating the list rather than selecting a item, because 
when a item was selected the whole list will be closed.

Comment 36

16 years ago
Kyle: Yes, that's pretty much what I was saying.  If your patch accounts for
both situations, then I'm ok with it.
(Assignee)

Comment 37

16 years ago
Thanks for the patch, looks like a good start. Here are my comments:

* I would like to test this patch. Please create patches from the parent
directory to mozilla. It's much easier to apply multi-directory patches from there.

* I don't like having the timer, because some users with physical disabilities
can only type 1 letter every 5 seconds or slower. I like leaving the code in
there, though, so that we know how to add it later if we want to. How about we
make the default timer value 10 seconds for now? We can always change it later.

* What about <outliner> support? Should we file a separate bug on that so we can
get this in too?

* Don't use variable names like aLetter or aTime unless they're arguments to the
method. The prefix letter a indicates that they are. If you're declaring them in
the body of the method, don't use a prefix letter.

* For casts, use NS_STATIC_CAST(NewType, variable); That's our cross-platform
portable version of casting. See Mozilla's C++ portability guide for more info:
http://www.mozilla.org/hacking/portable-cpp.html

* For simplicity, I would change 
doAction = matchCount > 1 ? PR_FALSE: PR_TRUE;
to
doAction = (matchCount <= 1);

* I appreciate the comments you're putting in the code. Some people are getting
picky about having correct English grammar and spelling in comments. I'm not,
unless it's difficult to understand. However, be aware that someone may ask you
to fix some things related to that.

Looking forward to getting a new patch that I can apply and test.
(Assignee)

Comment 38

16 years ago
Just spoke with Marlon Bishop from the UI team. He wants the timer in there, but
agrees it should be reasonably long. Let's start with 2.5 seconds and see how
that feels.
(Assignee)

Comment 39

16 years ago
Kyle, here is how you make a patch that everyone can apply:

First, always make sure POSIXLY_CORRECT=1 on you system. I don't know if this
affects cvs diff, but it does affect the patch utility.

cd to the mozilla directory where client.mak lives
cd ..
cvs diff -u mozilla/layout/xul/base > diff.txt

or

cvs diff -uN   (if you want new files in there as well)

or

cvs diff -u7   (or any other #, to increase the number of lines of context
before and after changes. Makes some patches easier to read)

To apply one of these patches:
Go to the same directory described above.
patch -p0 < diff.txt

- Aaron 
(Assignee)

Comment 40

16 years ago
This patch also doesn't work for <listbox>.
Perhaps we'll need to open up separate bugs for <outliner>/<tree> and <listbox>

Outliners and trees are in the midst of a rewrite, so I think listbox should be
worked on next.

Comment 41

16 years ago
Created attachment 75338 [details] [diff] [review]
patch 3


I've done some minor changes, such as var name, NS_STATIC_CAST,
time interval value, etc. And the patch was also regenerated as
your desire.

I'm sure that this patch is only for menulist/combobox. Now, I'm
looking for the event handler of outliner and listbox, and will
provide the new patch for them a few days later. So I suggest that
we should separate this bug into 2 or 3 bugs. One for menu, one for
outliner and one for listbox.
Attachment #74264 - Attachment is obsolete: true

Comment 42

16 years ago
I've completed the "first letter navigation" for outliner and listbox.
But only the *first* letter can be supported. because I don't like to
add global variable in js/xml file.
I also need modify a IDL file to support a new interface function.

Aaron, should we file a new bug for these two widgets? Then I can
upload the patch immediately.
(Assignee)

Comment 43

16 years ago
Changing summary, to more accurately reflect what this bug is about.

Spun off new bugs for other widgets:
XUL listbox: bug 133365
XUL outliner: bug 133366
Summary: press letters to select item in outliner, menulist, and listbox → press letters to select item in XUL menu, menulist
(Assignee)

Comment 44

16 years ago
I just tested this patch, and it is a good start.

There are 2 problems that I noticed:

First, when I press the same number or letter multiple times, it does not cycle
between all of the items that start with that. For example, I opened the combo
box under Preferences -> Appearances -> Fonts -> Size. Typing '1' many times
should cycle among the options 10, 11, 12, 13, ...

Second, I tried to type the letter 'P' in my Bookmarks menu. The only item that
starts with 'P' for me is Personal Toolbar Folder. It focused on that, but it
didn't open the submenu. It is supposed to activate the item when there is only
1 that starts with it or has the accesskey.

Comment 45

16 years ago
Hi, Aaron

I found that the menulist/combobox of current version 
(Build ID: 2002032603, win32) has some bugs.

To reproduce:
Preferences->Appearance->Font, drop down the "Serif"
(or Sans-serif) list, move mouse over the list, no
item can be highlighted.

So my patch can not work properly before this bug 
was fixed.

For the second problem, I explained my implementation
in Comment #35. If you think this style is not good,
I can make change.
(Assignee)

Comment 46

16 years ago
Kyle, thanks for the reply.

Bug 62583 relates to the problem navigating lists when the mouse is hovering
over it. How does this affect your ability to work on this bug? Aren't they two
separate issues? For your testing, you can move the mouse out of the way, no?

As far as standane drop down menulists like those in preferences, I agree with
your comment #35. However, for pull down menus in the menubar it should activate
the item. Correct me if I am wrong -- I believe this is consistent with other
Windows programs. Check out how the code at the bottom of
nsMenuPopupFrame::FindMenuWithShortcut() determines whether it is a drop down
from the menubar so that it knows whether to beep or not.

Blocks: 122038

Comment 47

16 years ago
I think this is a newer bug, because the menu works properly in
the 2002031500 build on Solaris. For 2002032603 Windows version,
sometimes, I can not use keyboard to select the menu item, wherever 
the mouse is.  
(Assignee)

Comment 48

16 years ago
Hmmm ... I still haven't see the second half of my comment #46 addressed in a patch.

I'm looking for a patch that acts differently for pulldown menus from the
menubar, vs. comboboxes.

Is that still happening?

Comment 49

16 years ago
Created attachment 76708 [details] [diff] [review]
patch 4


Hi, Aaron

Patch was revised as your desire. I treat pull down menu and combo box
separately. But there is still a puzzle -- the Bookmarks button on the toolbar.
Now I treat it as same as menu. You can see my comments in the code.
Attachment #75338 - Attachment is obsolete: true
(Assignee)

Comment 50

16 years ago
Works better, when menu item has underlined accesskey set.

However, for items with no underlined accesskey, it is not activating the item
in pull down menus when there is only 1 of them.

For example, press Alt+B for bookmarks, then P - that should highlight and open
the Personal Toolbar Foldeer.

Comment 51

16 years ago
Strange, my new patch is right for this issue. I'm very sure that the "Personal 
Toolbar Foldeer" will be opened when you press P, on my platform, both Windows 
and Solaris. Maybe someone broke menu again?
(Assignee)

Comment 52

16 years ago
Are you using bookmarks from the toolbar or bookmarks from the menubar? I think
it should work in both places.

For some reason, I get the following CVS error when I try to apply your patch on
a current build:

patching file `layout/xul/base/public/nsIMenuFrame.h'
patching file `layout/xul/base/src/nsIMenuParent.h'
patching file `layout/xul/base/src/nsMenuBarFrame.cpp'
patching file `layout/xul/base/src/nsMenuBarFrame.h'
patch: **** can't rename `/cygdrive/c/DOCUME~1/aaronl/LOCALS~1/Temp/po001092' to
 `layout/xul/base/src/nsMenuBarFrame.h' : Permission denied

I think this has something to do with some recent outliner checkins. Can you put
up a new patch? I'd prefer if the patch's origin directory is the parent
directory to mozilla.

Comment 53

16 years ago
Created attachment 77159 [details] [diff] [review]
patch 5


I didn't change anything, just regenerated the patch with recent trunk version.
There should not be any warnings when patch. Hope it can work.
Attachment #76708 - Attachment is obsolete: true

Comment 54

16 years ago
Created attachment 77161 [details] [diff] [review]
patch 6


sorry, patch 5 was wrong.
Attachment #77159 - Attachment is obsolete: true
(Assignee)

Comment 55

16 years ago
I finally got your patch to apply. Not sure why I had errors before.

Anyway, when I apply your patch and build, and press P for bookmarks, it
highlights P, but doesn't open the submenu up automatically. You have to then
press Enter.

Comment 56

16 years ago
Quick comments as I look over this at work...

Remove these:

+            commandAttr.Assign(NS_LITERAL_STRING(""));
+            menuAttr.Assign(NS_LITERAL_STRING(""));
+

They are unnecessary, and were removed a while back.

+      current->GetAttr(kNameSpaceID_None, nsXULAtoms::accesskey, textKey);
+      // If failed, get the text/label attribute

Remove this comment, it's obvious what you're doing.

+      if (textKey.Length() <= 0) { // No shortcut, try first letter
+        current->GetAttr(kNameSpaceID_None, nsXULAtoms::label, textKey);
+        if (textKey.Length() <= 0) // No label, try another attribute (value)

You should be able to use "if (!textKey)" in both comparisons.

+      if (textKey.Find(mIncrementalString) == 0) {
+        // Match

Remove this comment.

Comment 57

16 years ago
Aaron, I'm very sorry that maybe I put a wrong patch here.
Dean, many thanks for your comments.

I'll put a new patch ASAP.

Comment 58

16 years ago
Created attachment 77790 [details] [diff] [review]
patch 7


new patch! I'll crazy if it still cannot work. 

Aaron, If "P" still can not open the "Personal Toolbar folder" (both on menubar
and toolbar), please send me your source code under
mozilla/layout/xul/base/src. I want to know what's happen in your machine.
Attachment #77161 - Attachment is obsolete: true

Comment 59

16 years ago
I don't have the same problem as Aaron with this patch.  I have a "Keyword
Searches" folder in my bookmarks menu, the only item starting with 'K', and it
opens properly.

My main contention with this is that I have a menu with these items:
  Google Search
  Google Groups
  LXR File
  LXR Identifier
  LXR Text

Pressing 'G' repeatedly cycles between the two Google menu items.  But if I
press 'L' right after pressing 'G', I get a beep and nothing happens.  What
should happen, based on Windows experience, is 'LXR File' is selected.  If it
was the only 'L' menu, its method would be executed.  So if you don't find a
match, I say restart the search with a blank incremental string instead of just
quitting.

Also...

Index: mozilla/layout/xul/base/public/nsIMenuFrame.h
===================================================================
+class nsIDOMKeyEvent;

Is this needed?  I removed it and I seemed to compile without any problems.  I
don't see why you need to add it.


Index: mozilla/layout/xul/base/src/nsMenuFrame.h
===================================================================
+#include "nsMenuBarListener.h"

I didn't need this to compile.


Index: mozilla/layout/xul/base/src/nsMenuPopupFrame.cpp
===================================================================
+  if (keyTime - lastKeyTime > INC_TYP_INTERVAL) // Interval too long, treat as
new typing
+   mIncrementalString = pressKey;

Need another space before at the start of this line.

+  else {
+    if (mIncrementalString.Length() != 1 ||
+        GetCharAt(mIncrementalString, 0) != GetCharAt(pressKey, 0))
+        // If user typed the same key more than once, we should do a cycled
one-key navigation
+     mIncrementalString.do_AppendFromElement(GetCharAt(pressKey, 0));

Ditto.

+          if (! foundActive) {
+            if (! frameBefore)
+            if (! frameAfter)

Remove the space after '!', to be consistent with the rest of the file.

Comment 60

16 years ago
Hi, Dean, thanks for your careful check and review.

For the first issue, when you typed "G", then typed "L", it seems like you want 
to find "GL...". if the typing interval between "G" and "L" is longer than 2.5s
(comment 38), the program will start a new searching "L..."

I'll verify other issues soon.

Comment 61

16 years ago
I made up a sample menu when I tested that, which behaves differently on Windows
than a menulist (combo box).  On Windows, combo boxes have incremental searching
while menus do not.  Menus cycle by the first letter only, if an access key is
not defined, including the behavior I described in comment 59.

Comment 62

16 years ago
I'm re-compiling the whole mozilla to verify whether we needn't some forward 
definitions. 

Aaron, do you agree with Dean? That suggestion is we do incremental searching 
only for combobox, and do first-letter searching for menu.

Comment 63

16 years ago
verified.

class nsIDOMKeyEvent; in nsIMenuFrame.h can not be omited, because compiling 
nsCSSFrameConstructor.cpp will be failed.

#include "nsMenuBarListener.h" isn't needed.
(Assignee)

Comment 64

16 years ago
Yes, Dean is correct.

Comment 65

16 years ago
Created attachment 77825 [details] [diff] [review]
patch 8


minor changes. 

Now menu supports only first-letter navigation, combobox still supports
incremental typing navigation.

Updated

16 years ago
Attachment #77790 - Attachment is obsolete: true
(Assignee)

Comment 66

16 years ago
Rather than .8 seconds, for the keyboard repeat delay on Windows I think we're
supposed to use:

GetSystemParametersInfo()
SPI_GETKEYBOARDDELAY Get settings for delay time of key inputs.
(Assignee)

Comment 67

16 years ago
Looks pretty good. Two more problems for drop downs:

1. For drop downs, listboxes and trees, if there are characters in the search
buffer, backspace should clear the last character typed, and go back to the last
item selected.

2. For drop downs, listboxes and trees, pressing any other key, such as an arrow
key, home/end, etc., should clear the search buffer.

Comment 68

16 years ago
re: .8 seconds, see bug 133366 comment 32.
(Assignee)

Updated

16 years ago
Whiteboard: [adt3] - very nice to have

Comment 69

16 years ago
Hi, Aaron, I'll resolve these two problems on Monday.

As far as typing interval, I agree with Dean. I don't think the keyboard repeat 
delay and the incremental search interval are same things.

Comment 70

16 years ago
Created attachment 78164 [details] [diff] [review]
patch 9 (support BACKSPACE and clean up buffer when other key pressed)


By testing on Windows, I found when backspace was pressed the selection didn't
go to the previous selected item. So in my implementation, I just trim the
incremental string and don't move the selection to last selected item.
Attachment #77825 - Attachment is obsolete: true

Comment 71

16 years ago
In my testing, pressing backspace does actually go to the previous item, but
only after it's pressed a second time.

eg.
c
ca
car
cat

Typing c-a-t will select "cat".  Pressing backspace will keep "cat" selected,
but I can press 'r' and "car" will be selected.

Typing c-a-t again, then pressing backspace twice will display "ca", but typing
'r' will not select "car".  It seems like the display is always one step behind
the search term.

I don't think this is highly important to duplicate, and is probably something
we could handle after the original implementation.
(Assignee)

Comment 72

16 years ago
I think we should get it working completely right before it gets r=,sr=.
(Assignee)

Comment 73

16 years ago
After reading Kyle's comments, and looking at Window's behavior correctly, he
may have this correct. I will proceed with the review.
(Assignee)

Comment 74

16 years ago
almost ready to give r=

We should determine which is better. If you see scc or jag ask them.
if (stringB.Find(stringA) == 0) {/* do something */}
if (Substring(stringB, 0, stringA.length()).Equals(stringA)) {/* do something */}

Use IsEmpty() instead of Length()<=0

Can you use GetCharAt(mIncrementalSring,0) != charCode
instead
of
!= GetCharAt(pressKey,0)

Finally, please add some comments to describe your loop strategy. It is very
clever, but will be hard for other codes to understand if they need to fix
something.

Comment 75

16 years ago
"Use IsEmpty() instead of Length()<=0"

Argh, sorry Kyle, that's what I meant in my first comment from 2002-04-04, not a
null check.  Sorry if I caused you any frustration.
(Assignee)

Comment 76

16 years ago
Just spoke with Scott Collins (scc@mozilla.org) about some of the string stuff.

If you want to determine if |stringA| is a prefix of |stringB|, then do this:

  if ( Substring(stringB, 0, stringA.Length()) == stringA )
    // do something 

Comment 77

16 years ago
> Can you use GetCharAt(mIncrementalSring,0) != charCode
> instead
> of
> != GetCharAt(pressKey,0)

I can't do that because I called ToUpperCase(pressKey) to
uppercase the charCode. I need an ignore-case comparison.
(Assignee)

Comment 78

16 years ago
>> Can you use GetCharAt(mIncrementalSring,0) != charCode
>> instead
>> of
>> != GetCharAt(pressKey,0)

> I can't do that because I called ToUpperCase(pressKey) to
> uppercase the charCode. I need an ignore-case comparison.

Okay - right. Very good.

Comment 79

16 years ago
Created attachment 78278 [details] [diff] [review]
patch 10 (minor changes)


add more comments, re-write some nsString relative code, fix two small bugs:
1. return nsnull when the searching buffer was cleaned by backspace;
2. rollback the last typing if it can not match anything
Attachment #78164 - Attachment is obsolete: true
(Assignee)

Comment 80

16 years ago
Comment on attachment 78278 [details] [diff] [review]
patch 10 (minor changes)

r=aaronl
Attachment #78278 - Flags: review+

Comment 81

16 years ago
Created attachment 78690 [details] [diff] [review]
patch 11


wrong behavior when charCode == 0. fixed!
Aaron, can you r= again?
Attachment #78278 - Attachment is obsolete: true
(Assignee)

Comment 82

16 years ago
Comment on attachment 78690 [details] [diff] [review]
patch 11

r=aaronl
Attachment #78690 - Flags: review+

Comment 83

16 years ago
Comment on attachment 78690 [details] [diff] [review]
patch 11

sr=hyatt.  Test thoroughly please.
Attachment #78690 - Flags: superreview+

Comment 84

16 years ago
Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email adt@netscape.com.  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.
Keywords: nsbeta1-

Comment 85

16 years ago
Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email adt@netscape.com.  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.
Keywords: nsbeta1+

Comment 87

16 years ago
checked into trunk.
thanks for pete.zha's help.
Status: NEW → RESOLVED
Last Resolved: 17 years ago16 years ago
Resolution: --- → FIXED

Comment 88

16 years ago
Did you check it into the branch as well?

Comment 89

16 years ago
I didn't, but will do that soon.

Comment 90

16 years ago
ok, checked into branch!
(Reporter)

Comment 91

16 years ago
*** Bug 139790 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 92

16 years ago
Caused regression: bug 139877, "right-click context menu hotkeys require 'enter'
to perform action".

Comment 93

16 years ago
why this got checked into branch without adt1.0.0+ ?
the patch are one of the suspect of causing bug 140983
I don't see any verification on the trunk neither.

Comment 94

16 years ago
what kind of testing have been pass before you land this patch into the branch?
I don't see any QA verify the fix on the trunk. 

Comment 95

16 years ago
I thought adt related only to Netscape, not Mozilla?  This patch had a= from a
driver.

Comment 96

16 years ago
forget about the comment related to bug 140983. It seems the problem happended
on the trunk before it land .

Comment 97

16 years ago
adding fixed1.0.0 keyword (branch resolution). This bug has comments saying it
was fixed on the 1.0 branch and a bonsai checkin comment that agrees. To verify
the bug has been fixed on the 1.0 branch please replace the fixed1.0.0 keyword
with verified1.0.0.
Keywords: fixed1.0.0
n/a on mac --but this works on win2k and linux (2002.06.17.07 branch comm bits).
i need to hit the down arrow to expand the droplist (in dlgs, like the Fonts
panel) before letter keys'll work. and, the main menus work fine as well.
Status: RESOLVED → VERIFIED
OS: Windows NT → All

Comment 99

16 years ago
It would be cool to have it working w/o pressing the down arrow.
Is there a bug filed for this ?

Comment 100

16 years ago
Why doesn't work on mac? 

Jan, I think sairuh means that we must pull down the combobox's dropdown list 
first, then can use key-navigation.

Comment 101

16 years ago
Yeah, I know
But I meant to have keyboard navigation even without showing menupopup.
I saw that on windows with a native menulist widget.

Comment 102

16 years ago
You shouldn't have to drop down a combo box before keyboard navigation will
work.  Right now the behavior is inconsistent between HTML selects, which don't
have to be dropped down, and XUL menulists, which do have to be dropped down. 
Separate bug?

Comment 103

16 years ago
=> bug 64157 Should be able to pick options in <menulist> when it's not dropped 
down
this is n/a on mac because xul access keys are turned off for that platform.

kyle, thanks for the ref to bug 64157.

really marking verified on branch (kw-wise).
Keywords: fixed1.0.0 → verified1.0.0
You need to log in before you can comment on or make changes to this bug.