Closed Bug 150590 Opened 22 years ago Closed 22 years ago

should be able to type words into <select> - incremental search for item

Categories

(Core :: DOM: UI Events & Focus Handling, enhancement)

Product:
Core
Component:
DOM: UI Events & Focus Handling
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: joost, Assigned: yuanyi21)

References

Details

Attachments

(2 files, 2 obsolete files)

sreenshot: popup menu at the wrong position when right click on a <select> drop down menu
22.02 KB, image/png
Details
new patch
7.62 KB, patch
john
: review+
bryner
: superreview+
Details | Diff | Splinter Review 
(IE on Macintosh has this behaviour, and it's nice. I think Moz should have it too.)

1. Open any page with an HTML form that has SELECT fields
2. use tab to go to the select field
3. enter a character a-z. input jumps to option starting with character

4. enter words like 'mozilla' in the input. input jumps to options starting with
'm', 'o', 'z' etcetera. Better behaviour would be to jump to the option named
'mozilla' (if any).

The delay could be set to 0.5 sec or something.
-> keyboard navigation
Assignee: rods → aaronl
Status: UNCONFIRMED → NEW
Component: HTML Form Controls → Keyboard Navigation
Ever confirmed: true
QA Contact: tpreston → sairuh
Summary: should be able to type words into input type=select → should be able to type words into <select>
We already do this! Do you have a testcase where we don't?
Aaron: check component select box for this bug. When I focus it and write Java,
selection is jumping from actual item to Java API for DOM, then to Accessibility
APIs, then to Viewer App and finally again to Accessibility APIs.
Component: Keyboard Navigation → Accessibility APIs
Ok, right. I agree fully. However, I think we might want to do this work on
XBLFC based selects only, since the old form controls are going away.

CC'ing Sun's brower China accessibility task force, since they did similar work
for XUL selects.
Blocks: atfmeta
Summary: should be able to type words into <select> → should be able to type words into <select> - incremental search for item
Yes, I have done that for xul <tree>. It can be ported to XBL <select> easily. 
Once the XBL form control replaced the old one, I'll bring my patch here.
Assignee: aaronl → kyle.yuan
Depends on: 122038
Weird. I clicked on a <select> on this page (bugzilla.moz), then right
click on the drop down menu, for some reason the popup menu consistantly
shows up some distance away from the top-left corner of the content area
(away from where the pointer is). I suspect this is a dupe, but couldn't
find a bug report for this. Anyone?
You mean the position of popup menu is sometimes wrong? I know that could be a 
bug of gtk module. I can reproduce that on Solaris when the sidebar opened.

Also change the component to keyboard navigation.
Component: Accessibility APIs → Keyboard Navigation
?irrevelent attachment?

moz 2002061908, New Classic 1.1 theme, win98
bug always reproduceable on this end
Could you create a new bug for comments about the popup menu being in the wrong
position, please (or use the existing one?).  They aren't relevant to this bug,
so it's just spam.  :-(
*** Bug 157569 has been marked as a duplicate of this bug. ***
*** Bug 163532 has been marked as a duplicate of this bug. ***
*** Bug 167949 has been marked as a duplicate of this bug. ***
Is it possible to borrow code from 'Type Ahead Find'? The functionality is not that 
dissimilar. And to answer post #2 - no, we don't currently do this.
Is this a dup of bug 79035
I would not say that this is a _dup_ of bug 79035 as bug 79035 also suggests
that the set of options i restricted to elements that fit the search e.g. if you
type 'c' only option beginning with c are displayed. IMHO a nice feature as the
drop down lists get more clear with less items.
We can't borrow any code for typeaheadfind for this. Sorry, they're dealing with
completely different data structures.

My earlier comment about waiting for XBLFC seems irrelevant now, since it is
unclear if we will ever use XBLFC because of performance issues.
I think this wouldn't be a difficult one. If we don't need to wait the XBLFC,
I'll try to fix this by this weekend.
Status: NEW → ASSIGNED
We don't need to wait until XBLFC, no :)
migrate some my previous work for menu/listbox/tree.

seeking r=.
Comment on attachment 99036 [details] [diff] [review]
patch to enable incremental typing navigation

Good patch, Kyle, even if the review seems long :)

>Index: html/forms/src/nsListControlFrame.cpp
>===================================================================
>@@ -3300,6 +3300,9 @@
>+  // if we are not doing a incremental finding, the mIncrementalString should be cleared
>+  PRBool isIncrementalFinding = PR_FALSE;

didIncrementalSearch is a better name I think.	And the comment here should
read more like:
// Whether we did an incremental search or another action

Transfer the comment here to the place where you actually clear
mIncrementalString.

>@@ -3408,29 +3411,50 @@
> ...
>+          mIncrementalString.SetLength(mIncrementalString.Length() - 1);

Should be mIncrementalString.Truncate(mIncrementalString.Length() - 1) ...
SetLength is deprecated.

> ...
>+        return NS_OK;
>+      }
>+
>+      static DOMTimeStamp lastKeyTime = 0;

Please make this a global variable, and clear it in ComboboxFinish(). 
Otherwise it will not get cleared when the combobox rolls up and the search
could be transferred to a new combobox.  I like the idea of making it global,
BTW, smart :)


There are enough changes in the next set of code that I felt it would be best
if I illustrated with some (untested but hopefully working) code.  Take the
code or make your own if you want, it's just a demonstration :)  Make sure you
do comments though.  The review comments with [changed above] in front of them
have been incorporated into this code.

EXAMPLE CODE
============
DOMTimeStamp keyTime;
aKeyEvent->GetTimeStamp(&keyTime);

// Incremental Search: if time elapsed is below
INCREMENTAL_SEARCH_KEYPRESS_TIME,
// append this keystroke to the search string we will use to find options and
start
// searching at the current keystroke.	Otherwise, 

// Truncate the string if it's been a long time since our last keypress.
if (key - gLastKeyTime > INCREMENTAL_SEARCH_KEYPRESS_TIME) {
  mIncrementalString.Truncate();
}

gLastKeyTime = keyTime;

// Determine where we're going to start reading the string
// If we have multiple characters to look for, we start looking *at* the
// current option.  If we have only one character to look for, we start looking
// *after* the current option.	Exception: if there is no option selected to
// start at, we always start *at* 0.
PRInt32 startIndex;
GetSelectedIndex(&startIndex);
if (startIndex == kNothingSelected) {
  startIndex = 0;
} else if (mIncrementalString.IsEmpty()) {
  startIndex++;
}

// Append this keystroke to the string.
mIncrementalString.Append(PRUnichar((unsigned char)charCode));
================

>+      nsAutoString pressKey(NS_STATIC_CAST(PRUnichar, charCode));

[changed above] Don't do that.	Just Append() the character below, or
Truncate() / Append() if necessary.

>+      if (keyTime - lastKeyTime > 1000)

[changed above] This time should be a constant, maybe #define
INCREMENTAL_SEARCH_KEYPRESS_TIME.  We need to get someone in here to advise on
how long it should be, but we can do that once the rest is ironed out.	Is 1
second normal for this kind of thing?

>+        mIncrementalString = pressKey;
>+      else if (mIncrementalString != pressKey)
>+        mIncrementalString += pressKey;

[changed above] What happens if the user presses the same key several times in
a row on purpose searching for "aaa"?  Is this really advisable?  I think one
should always append the key.

>+      lastKeyTime = keyTime;
> 
>+      PRInt32 selectedIndex = -1, start = 1;

[changed above] selectedIndex should be named startIndex, and startOffset
becomes un-needed at that point.

>       GetSelectedIndex(&selectedIndex);
>-      if (selectedIndex == kNothingSelected) {
>-        selectedIndex = 0;
>-      } else {
>-        selectedIndex = (selectedIndex+1) % numOptions;

[changed above] The trick of leaving selectedIndex at -1 and setting start to 0
is hard to read :)  Should be explicit.  Also, it's not selectedIndex in this
code anymore, it's startIndex.

>+      if (mIncrementalString.Length() > 1) {
>+        start = 0;
>+        if (selectedIndex < 0)
>+          selectedIndex = 0;
>       }

[changed above] It would be slightly more efficient if you did this stuff above
... like if you have to truncate, set startOffset = 0.	

>+      PRUint32 i, index;
>+      for (i = 0; i < numOptions; i++) {
>+        index = (i + start + selectedIndex) % numOptions;

Nice.  I like this improvement.  It will have to be modified to "index = (i +
startIndex) % numOptions" given the changes above, of course.

> ...
>+            if (Substring(text, 0, mIncrementalString.Length()) == mIncrementalString) {

This should be case-insensitive.  Substring(...).Equals(mIncrementalString,
nsCaseInsensitiveStringComparator()) should work.  You may have to #include
"nsUnicharUtils.h" to do that, if it doesn't compile.

>+              PRBool wasChanged = PerformSelection(index,
>                                                    isShift, isControl);

Please combine the last two lines into one ...

>Index: html/forms/src/nsListControlFrame.h
>===================================================================
>@@ -430,6 +430,8 @@
>+  nsString     mIncrementalString;  // for incremental typing navigation

Why not make this global along with lastKeyTime?  Or at least make it an
nsString* that you initialize the first time you use it, I hear object globals
have trouble getting initialized and destroyed properly.  There can only be one
incremental typing going on at a time anyway, and doing it the way you're doing
it here will add 12 bytes per select element.

When you do that, don't declare an actual static variable, it's apparently
non-portable to create a global object.  You can create a static nsAString&
nsListControlFrame::GetIncrementalString { static nsString incrementalString;
return incrementalString; } and it should do everything you need.
Attachment #99036 - Flags: needs-work+
jkeiser, thank you very much for the line-by-line review. Here is my comment:

> >+      if (keyTime - lastKeyTime > 1000)
> 
> [changed above] This time should be a constant, maybe #define
> INCREMENTAL_SEARCH_KEYPRESS_TIME.  We need to get someone in here to advise on
> how long it should be, but we can do that once the rest is ironed out.	Is 1
> second normal for this kind of thing?

We do have that constant which is defined in
http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsMenuPopupFrame.h#60
I'm not sure where is the best place to share that definition?

> >+        mIncrementalString = pressKey;
> >+      else if (mIncrementalString != pressKey)
> >+        mIncrementalString += pressKey;
> 
> [changed above] What happens if the user presses the same key several times in
> a row on purpose searching for "aaa"?  Is this really advisable?  I think one
> should always append the key.

This is a trick. In the normal case, when user is typing "aaa", he/she wants to
cycle through the all options begin with letter 'a' (otherwise, he/she has to
wait for 1-2 seconds to do the same work), rather than locate the option "aaa".
This is also the behavior of Windows listbox and mozilla listbox/tree/menu.

> > ...
> >+            if (Substring(text, 0, mIncrementalString.Length()) ==
mIncrementalString) {
> 
> This should be case-insensitive.  Substring(...).Equals(mIncrementalString,
> nsCaseInsensitiveStringComparator()) should work.  You may have to #include...

I did ToLowerCase for both typing char and option text, so that is already
case-insensitive.

I'll make a better patch addressing your other comments.
> We do have that constant which is defined in
> http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsMenuPopupFrame.h#60
> I'm not sure where is the best place to share that definition?

I guess duplicate it for now.  I don't think there really is a good place.  What
would be really good is, if you could put a comment next to both of them
referencing the other one so that if someone changes one they also change the other.

> > [changed above] What happens if the user presses the same key several times in
> > a row on purpose searching for "aaa"?  Is this really advisable?  I think one
> > should always append the key.

> This is a trick. In the normal case, when user is typing "aaa", he/she wants to
> cycle through the all options begin with letter 'a' (otherwise, he/she has to
> wait for 1-2 seconds to do the same work), rather than locate the option "aaa".
> This is also the behavior of Windows listbox and mozilla listbox/tree/menu.

All rightey then.  It seems like inconsistent UI behavior to me but I'm not a
trained UI dude :)

> I did ToLowerCase for both typing char and option text, so that is already
case-insensitive.

Ah yes, I see that.  That code is *horrible* ... even though it's not your
fault, could you please fix it to not call ToLower() on the string and instead
use a comparator?  It is completely unnecessary to lowercase the entire string
just in order to compare the first one or two characters :)
jkeiser, I believe I've addressed all your comments in comment 20 and comment
22. seeking r=
Attachment #99036 - Attachment is obsolete: true
Comment on attachment 99196 [details] [diff] [review]
new patch addressing jkeiser's comments

r=jkeiser with these changes:

> +      if (GetIncrementalString().Length() != 1 || 
> +          GetIncrementalString().First() != NS_STATIC_CAST(PRUnichar, charCode))

It would be clearer to invert the if -- if (!(... == 1 && ... == ...)

> +              if (wasChanged)
>                  UpdateSelection(); // dispatch event, update combobox, etc.
> -              }

Please leave the braces around this if.

Same below in if (!didIncrementalSearch).  In general, always put braces around
ifs.  It makes things easier to read, people have to do less parsing in their
brains.

+#define INCREMENTAL_SEARCH_KEYPRESS_TIME 1000

Why are you changing this to 1s?  I think 2.5s is actually better than 1s
(maybe 2s is a good compromise).  If aaronl is OK with it, though, I'm OK too. 
Please get his approval for this change.
Attached patch new patch — — Splinter Review 
>Please leave the braces around this if.
>
>Same below in if (!didIncrementalSearch).  In general, always put braces
around
>ifs.  It makes things easier to read, people have to do less parsing in their
>brains.
Some SRs don't like this style, I was asked to get rid of the braces around the
1-line statement :( But anyway, I would like to keep the present code style in
this patch.

>Why are you changing this to 1s?  I think 2.5s is actually better than 1s
>(maybe 2s is a good compromise).  If aaronl is OK with it, though, I'm OK too.

>Please get his approval for this change.
This value is mpt's suggestion in bug 133366 comment 35, and it hasn't got
objection from Aaron :)
Attachment #99196 - Attachment is obsolete: true
Aaron, are you OK with having only 1 second between keystrokes to indicate that
a user is using incremental search?  (i.e. "type A, wait .5s, type B" searches
for AB but "type A, wait 1s, type B" searches for B)

Kyle: I'm OK with the 1s change assuming Aaron is, but not the brace change. 
The sr is not the point here.  Module style reigns for bracing and whitespace,
and this is where module style is going (yes, it is currently inconsistent
within the module but this is where I want it to go).  Please change the bracing
style.  sr's might have asked you to change it in the past because the main
style of the module was to not have braces around one line ifs.  That is not so
here; we have a coherent direction (even if the code does not yet match it).  I
feel bad that different module have different styles, but some of us are going
in this direction.  Forms are one of them.  I especially don't want the brace
changes to go *backwards* to no braces as this patch is doing.

r=jkeiser just with the brace change.  Go ahead and ask for super-review with
just that change, consider it an automatic review :)
However we determine what the timeout is for XUL menus -- we should do the same
thing here.
jkeiser, in my new patch, (attachment 99235 [details] [diff] [review]), I've already followed your style
by adding braces for *every* |if|/|else| statement.

aaron, now I unified the all timeout value to 1s, are you okay with that?
the reason is bug 133366 comment 35.
Comment on attachment 99235 [details] [diff] [review]
new patch

r=jkeiser
I apologize, I inferred from your statement that you had not changed the
bracing.  Sorry for the rant :)
Attachment #99235 - Flags: review+
sorry for my bad expression in comment 25. When I said "keep the _present_ code
style", it meant "keep _your_ code style" :)
I kind of wish the timeout was globally configurable, but I'm okay with it at 1s
for now
Comment on attachment 99235 [details] [diff] [review]
new patch

>+nsAString& 
>+nsListControlFrame::GetIncrementalString()
>+{ 
>+  static nsString incrementalString;
>+  return incrementalString; 
>+}
>+

I would just make this a (private) static class variable instead of using a
getter, since it's not exposed to any other classes.

>+      gLastKeyTime = keyTime;
>+
>+      // Append this keystroke to the string. 
>+      // Exception: if user repeats typing a same key, we'll cycle through the all options 
>+      // begin with that char, rather than append it.

Grammar nit- I think this sounds better:

Exception: If the user types the same key repeatedly, we'll cycle through all
options beginning with that char, rather than appending it.


>               }
>>   } // switch
>+
>+  // If we didn't an incremental search, clear the string

"didn't do"

>--- xul/base/src/nsMenuPopupFrame.h	7 Sep 2002 05:37:39 -0000	1.56
>+++ xul/base/src/nsMenuPopupFrame.h	15 Sep 2002 02:32:32 -0000
>@@ -57,8 +57,12 @@
> 
> #include "nsITimer.h"
> 
>-#define INC_TYP_INTERVAL  2500  // 2.5s. If the interval of two typings is shorter than this, 
>+#define INC_TYP_INTERVAL  1000  // 1s. If the interval of two typings is shorter than this, 

I know you didn't introduce this, but this comment sort of bugs me. Maybe
change it to "If the interval between two keypresses is shorter than this, ..."


sr=bryner with those changes.
Attachment #99235 - Flags: superreview+
> I would just make this a (private) static class variable instead of using a
> getter, since it's not exposed to any other classes.

bryner, in comment 20, jkeiser asked me to use the getter instead of using a 
class variable so that we can save 12 bytes for each <select>. if you are okay 
with this, I'll go on to checkin this fix.

thanks for correcting my bad english :)
bryner: also, when I suggested making a static nsAutoString outside a function
jag, dmose, alecf, etc. slapped me down and said to use it in a function for
cross-platform happiness.  Otherwise there are apparently issues with
initialization and destruction or something.
I can imagine such a problem happening with a static |nsCOMPtr|, but I don't see
how the order of static constructors and destructors could have an effect for
strings.  Are the string classes not to be used before XPCOM is initialized or
after it's shut down?
Blocks: 161449
Perhaps it's because the initializer for the AutoString won't run at all. 
Really not sure.  jag, alecf, dmose, jst, any comment on the use of "static
nsAutoString gMyString;"?  One of you told me not to do this but I can't
remember who, the discussion was long and involuted :)
its not complicated: constructors dont always run when a DLL is loaded
dynamically...

the simple solution is to have something like

static nsAutoString* gString;

nsAString& getGlobalFoo() {
  if (!gString)
    gString = new nsAutoString("blah");
  return *gString;
}

and you could do something similar for setting the string.

Then you can access it like
CallSomething(getGlobalFoo())
or 
pos = getGlobalFoo().Find(...);

alec, now the problem is: 
what do you think about the following code? I use a private static class member
as a "getter".
+class nsListControlFrame {
+  ...
+  static nsAString& GetIncrementalString ();
+};

+nsAString& 
+nsListControlFrame::GetIncrementalString()
+{ 
+  static nsString incrementalString;
+  return incrementalString; 
+}
+

or as alternative, does this global variable also work?
+static nsAutoString gIncrementalString;

Just so this doesn't get lost. If you do what alecf suggested you'll need to
make sure the global heap allocated ns(*)String is deleted on shutdown.
kyle: I think that might work, actually. I don't know why I didn't think of it :)
OK, so it seems like our simplest solution is the one already in the patch :)
checked in!
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
where is the UI for this feature documented? I didn't see anything on

    http://www.mozilla.org/projects/ui/accessibility/

In particular, I'm wondering if there is a means of finding the next occurance.
If a menu contains something like

    a_word_1
    b_word_1
    c_word_1
    d_word_1
    e_word_1
    f_word_1
    z_really_long_word_goes_here_1
    a_word_2
    b_word_2
    c_word_2
    d_word_2
    e_word_2
    f_word_2
    z_really_long_word_goes_here_2
    a_word_3
    b_word_3
    c_word_3
    d_word_3
    e_word_3
    f_word_3
    z_really_long_word_goes_here_3

... it would be nice to type 'z' and then maybe Accel-G a couple of times to get
to the third occurance without having to completely spell out

    z_really_long_word_goes_here_3

Could Accel-G be "stolen" for this purpose while focus is within a select?

If not, is there some other means of quickly getting to that third z option?

-matt
The last posting brings back a nice idea into my mind that somebody told me: If it is possible to jump to b_word_x by typing 'b', some people would really like it if all not-b_word_x would be faded out: Like I type 'c' and the list looks like that:

    c_word_1
    c_word_2
    c_word_3

Indeed, I understand that some people don't want other option-values to be omitted. That's why the fade-out-feature should be optional.

Summarising, for me (from a simple user's perspective), the feature should show the following behaviour:

c --> all not-c_words are faded out
wait
c again --> jumps to the next occurance of c
wait
c again --> jumps to the next occurance
inst del --> complete list
a --> first occurance of a
b --> first occurance of ab

The problem is the 'wait'. When is the 'b' supposed tu follwow the 'a' and when is it supported to replace it? Furthermore, people would like to quickly jump to the next occurance rather than having to wait. 

Perhaps, we have to forget about jumping to the next occurance by re-typing the same letter and trigger this feature by the enter-key or F3 or cursor-down or some other key that is not supposed to be value of any option-tag. This allows to type words into select-fields and quickly navigate through them.

René
PS: I hope my discussion isn't off topic, but I have to admit I have not understood all the recent code-postings.
PPC: What is Accel-G?
Accel-G means "accelerator key and G"... On a PC, that's Ctrl-G. On a Mac,
that's Command-G. Etc. "Accel" is a platform-neutral way of saying "normal
keyboard shortcut character".

I like the "remove options" option. Is there a bug filed for that feature request?

Regarding the timeout question, there is an existing timelimit in the similar
Bug 30088 which could be re-used here. Then the behavior would be consistent.

-matt
Thanks for the explanation.
Concerning the time limit, please consider, that if there are long lists, and people want to jump down 20 items, they type ttttttttttttt rather than t-t-t-t-t-t-t-t-t-t-t-t-t-t ('-' indicates a small wait time). If you trigger the jump to the next occurance by F3 or enter, you can spare the wait time.

on the other hand, think about your mother in law :-) She looks for e.g. a_word and she types a (look for the next key) _ (look for the next key) w ... What I want to say: It might be difficult for some people not to exceed the time limit when typing words. Please cosider that.

René
Re: comment 44, please see comment 21. You can go to the 3rd z_.... just by
typing 'z' three times. 
Re: comment 45, the fade-out feature is good, but it may not be suitable for
every situation, for example, if there are some options like:
ja
jb
...
jz
ka
then someone may want to go jz by pressing 'k' then 'up arrow'. Your new feature
will let us lose this 'shortcut'.
If you really like this feature, you can file a new bug instead of discussing it
in a fixed bug :)
typing z three times fails to go to the third z_ if there is a 'zzz' in the
select. Is there a method which doesn't break in situations like that?

-matt

I don't think there is any method can deal with the all situations. But we must
guarantee that this feature is suitable for the most common usage.
http://www.mozilla.org/projects/ui/accessibility/typeaheadfind.html

that feature has the same problem, and provides a solution (Accel-G to find
again). I think borrowing Accel-G within the menu search would be an ideal
solution. If not that, then it at least shows that a solution is possible, and
some other method could be used.

Not providing a solution just leaves a user wondering "why did they solve this
problem in that context, and then ignore it here... it's the same problem".

Broken UI is worst when it is only broken in some contexts. At least if it is
broken everywhere, you can learn to expect it not to work.

-matt
Blocks: 144303
there's a problem with our current implementation: see bug 144303
We will need an option in drop-down XUL for overriding the default
fade-out wait period, e.g. set it to zero.
vrfy'd fixed with 2002.10.15.08 comm trunk builds.
Status: RESOLVED → VERIFIED
Component: Keyboard: Navigation → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: