Bug 3852

Summary: typeahead doesn't work in multiple row select boxes.
Product: WebKit Reporter: Joost de Valk (AlthA) <joost>
Component: FormsAssignee: Adele Peterson <adele>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, webkit-bugs
Priority: P2    
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
testcase
none
Patch to enable type select on List Boxes
darin: review-
New Patch to enable type select on List Boxes
none
Anothr testcase with ß as an item
none
Newer patch to enable typeselect on list boxes
none
Newest typeselect list box patch
eric: review-
More Newest Patch to enable type select on List Boxes darin: review+

Description Joost de Valk (AlthA) 2005-07-04 04:57:10 PDT
see forthcoming attachment.
Comment 1 Joost de Valk (AlthA) 2005-07-04 04:59:20 PDT
Created attachment 2790 [details]
testcase
Comment 2 Darin Adler 2005-09-24 05:05:31 PDT
I imagine that Dave get's around to doing the non-NSView version of this control he'll fix this.
Comment 3 Rosyna 2005-09-25 02:46:59 PDT
Created attachment 4036 [details]
Patch to enable type select on List Boxes

This patch uses the new UnicodeTypeSelect API in 10.4 and later (was SPI and
had different symbol names previous to that).
Comment 4 Rosyna 2005-09-26 11:45:13 PDT
review: ?
Comment 5 Dave Hyatt 2005-09-26 12:41:39 PDT
Comment on attachment 4036 [details]
Patch to enable type select on List Boxes

r=me
Comment 6 Darin Adler 2005-10-02 21:08:17 PDT
UCTypeSelectReleaseSelector needs to be called in finalize as well as dealloc.
Comment 7 Darin Adler 2005-10-02 21:13:30 PDT
Comment on attachment 4036 [details]
Patch to enable type select on List Boxes

This patch is great, but I think it still needs a little work:

I don't see any reason for the check for a refcon that's nil -- can't happen.
The check for a box of nil is required of course. But the coding style is not
quite right. The * is not supposed to be next to the type name (see the coding
style document on our website). Also I don't think the extra parentheses around
box->count() make things clearer.

Instead of calling getNSString() and casting to CFStringRef this should be
calling getCFString().

Also, if outString is 0, then we should save code by not calling itemAtIndex at
all (put that code inside the if).

The brace should be on the same line as the if. We use 0 in our code, not NULL
(see coding style document).

There's no need to use "UPP" in the code at all. This code is only used in
Mach-O and so both NewIndexToUCStringUPP and DisposeIndexToUCStringUPP are
no-ops and should be left out entirely. The code can be used as is.

There's no need to cast self to (void*) -- the cast should be left out.

The extra space before "byExtendingSelection" should be omitted.

Even though these are small things, I'm changing the review+ to review- so we
can get them fixed before landing the patch.
Comment 8 Rosyna 2005-10-03 00:33:49 PDT
Created attachment 4170 [details]
New Patch to enable type select on List Boxes

Added changes as specified by Darin. Now uses ICU to determine if the character
is in the range of graphic characters as defined by D13a of the Unicode 4.0
standard.
Comment 9 Rosyna 2005-10-03 00:37:09 PDT
Created attachment 4172 [details]
Anothr testcase with ß as an item

Added a new testcase to check against option S.
Comment 10 Rosyna 2005-10-04 18:20:02 PDT
Created attachment 4201 [details]
Newer patch to enable typeselect on list boxes

Bah, I kind of suck. Had two checks for outString. Only one is needed.
outString shall never be false much like item shall never be false.
Comment 11 Rosyna 2005-10-04 21:59:08 PDT
Created attachment 4203 [details]
Newest typeselect list box patch

Sigh, the old one was the wrong one. Apologies to the people that get emailed
bug reports.
Comment 12 Darin Adler 2005-10-06 07:17:06 PDT
Comment on attachment 4203 [details]
Newest typeselect list box patch

Looks good. There are a couple "=" and "==" in there without spaces around
them. And a "{" without a space before it. But everything else looks great.

r=me
Comment 13 Eric Seidel (no email) 2005-10-06 12:02:18 PDT
Comment on attachment 4203 [details]
Newest typeselect list box patch

Darin and I discussed.	In order to make this easier on the lander (who may not
be darin), I would like to ask you to first fix these syle issues, so tha
landing is just as simple as cvs-applying the patch.
Comment 14 Rosyna 2005-10-06 18:09:00 PDT
Created attachment 4243 [details]
More Newest Patch to enable type select on List Boxes

???????
Addresses the syle issues. I think I got them all. I'm not as sensitive to
spaces as some others.
Comment 15 Darin Adler 2005-10-07 10:05:30 PDT
Comment on attachment 4243 [details]
More Newest Patch to enable type select on List Boxes

r=me