Bug 45192 - Provide a way to trigger a <select multiple> onchange event on changes
Summary: Provide a way to trigger a <select multiple> onchange event on changes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-03 12:07 PDT by Joseph Pecoraro
Modified: 2010-09-07 16:35 PDT (History)
4 users (show)

See Also:


Attachments
[PATCH] Proposed Change (4.58 KB, patch)
2010-09-03 12:15 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[TEST] Autofill Test Case (734 bytes, text/html)
2010-09-03 14:42 PDT, Joseph Pecoraro
no flags Details
[PATCH] Generically Handle listbox <select>s (3.28 KB, patch)
2010-09-03 14:44 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] -[DOMHTMLSelectElement _activateItemAtIndex:allowMultipleSelection:] (7.00 KB, patch)
2010-09-03 17:21 PDT, Joseph Pecoraro
darin: review+
Details | Formatted Diff | Diff
[PATCH] Automated Test (18.27 KB, patch)
2010-09-03 17:22 PDT, Joseph Pecoraro
darin: review-
Details | Formatted Diff | Diff
[PATCH] Automated Test (9.93 KB, patch)
2010-09-07 16:14 PDT, Joseph Pecoraro
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2010-09-03 12:07:35 PDT
<select multiple> when a listbox, triggers onchange events via mouse down events.
That means programmatically in a WebKit client wants to change a <select multiple>'s
selectedIndexes  through a bindings interface the "change" event won't fire.
Comment 1 Joseph Pecoraro 2010-09-03 12:15:23 PDT
Created attachment 66532 [details]
[PATCH] Proposed Change
Comment 2 Darin Adler 2010-09-03 13:01:00 PDT
Comment on attachment 66532 [details]
[PATCH] Proposed Change

> +    // On a <select multiple> this needs to be handled slightly differently since onchange events
> +    // would have been fired via the mouse events on list box selects.
> +    if (WebCore::HTMLSelectElement* select = core(self)) {
> +        if (!select->multiple())
> +            select->setSelectedIndexByUser(index, true, true);
> +        else
> +            select->setSelectedIndexOnMultipleByUser(index, true);
> +    }

It seems strange that we need to call two different functions. Can we instead just change the setSelectedIndexByUser in WebCore to do the right thing when multiple() is true and usesMenuList() is false? Do we know of any specific problems caused if we do that?
Comment 3 Joseph Pecoraro 2010-09-03 13:27:50 PDT
> It seems strange that we need to call two different functions. Can we instead
> just change the setSelectedIndexByUser in WebCore to do the right thing
> when multiple() is true and usesMenuList() is false? Do we know of any specific
> problems caused if we do that?

You're absolute right. That would simplify this patch quite a bit as well.
Comment 4 Joseph Pecoraro 2010-09-03 13:28:51 PDT
You're absolutely right.* My grammar on Bugzilla is atrocious.
Comment 5 Joseph Pecoraro 2010-09-03 14:42:38 PDT
Created attachment 66545 [details]
[TEST] Autofill Test Case

This is a test case for Safari's Autofill. In my case, autofill selects California
and only triggers the "change" event for the bottom, single <select>. However,
the other <selects> are populated correctly.

After my patch all 3 selects trigger their "change" events, and are populated
correctly. Running autofill multiple times works as expected. Submitting the
form (it submits to itself) produces the values you would expect.
Comment 6 Joseph Pecoraro 2010-09-03 14:44:49 PDT
Created attachment 66546 [details]
[PATCH] Generically Handle listbox <select>s

Dan Bernstein pointed out that listboxs could also be made by
<select size="{value > 1}">. That exhibited the same problem.
This patch should handles that case as well.
Comment 7 Joseph Pecoraro 2010-09-03 14:46:56 PDT
For the [TEST]. Apparently putting a <form> in an attachment on Bugzilla is not
a good idea. You should be able to save the html file locally and test it.
Comment 8 Joseph Pecoraro 2010-09-03 15:11:15 PDT
Hmm, there is a minor issue in this patch. The "allowsMultiply" (which I think should be
"allowsMultiple") parameter should be passed in at the outer level at the _activate function.
To give clients the option of selecting multiple items in a <select multiple> or not.

Can the function's signature be changed without breaking browsers?
Comment 9 Darin Adler 2010-09-03 15:12:04 PDT
(In reply to comment #8)
> Can the function's signature be changed without breaking browsers?

What function exactly? In C++ and Objective-C you can add a new argument with overloading.
Comment 10 Darin Adler 2010-09-03 15:14:14 PDT
If you’re referring to the Mac SPI method, _activateItemAtIndex:, then sure we can add an argument but we can’t remove the existing method until we get rid of all callers. For example, open source contributors use the old version of Safari, so need the old method still around.

We should only add the argument if we are sure we need to actually set it. We should not add it just in theory.
Comment 11 Darin Adler 2010-09-03 15:14:40 PDT
Comment on attachment 66546 [details]
[PATCH] Generically Handle listbox <select>s

Is there any way to test this? Could you please add a manual test if not an automated one?
Comment 12 Joseph Pecoraro 2010-09-03 15:18:07 PDT
> Is there any way to test this? Could you please add a manual test
> if not an automated one?

Sure.


> We should only add the argument if we are sure we need to actually set it.
> We should not add it just in theory.

Yes, there is a case where this is necessary.
Comment 13 Joseph Pecoraro 2010-09-03 17:21:58 PDT
Created attachment 66569 [details]
[PATCH] -[DOMHTMLSelectElement _activateItemAtIndex:allowMultipleSelection:]

This is the patch, adding the new method to the interface to also allow
multiple selections in a <select multiple>.
Comment 14 Joseph Pecoraro 2010-09-03 17:22:48 PDT
Created attachment 66570 [details]
[PATCH] Automated Test

I separated this out into its own patch because this has a lot
of boilerplate that would crowd the real changes.
Comment 15 Eric Seidel (no email) 2010-09-07 03:20:01 PDT
Comment on attachment 66546 [details]
[PATCH] Generically Handle listbox <select>s

Cleared Darin Adler's review+ from obsolete attachment 66546 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 16 Joseph Pecoraro 2010-09-07 12:01:44 PDT
Darin could you review the updated patches?
Comment 17 Joseph Pecoraro 2010-09-07 13:18:05 PDT
Thanks! Does that apply to both patches?

I also removed some unused JavaScript code from the LayoutTests I added.
Comment 18 Darin Adler 2010-09-07 13:18:44 PDT
Comment on attachment 66570 [details]
[PATCH] Automated Test

Great to add a test for this!

This DOMHTMLController object doesn’t seem to have too much to do with DOM, except inasmuch all JavaScript functions are part of "DOM". We have lots of functions like this on the main layout test controller. I’m not sure why we are making a new object for these functions. For example, the execCommand, isCommandEnabled, elementDoesAutoCompleteForElementWithId, counterValueForElementById, computedStyleIncludingVisitedInfo, and markerTextForListItem functions.

Maybe it would be better to make this part of a "form testing" controller, or a "user input simulating" type controller, or maybe make it part of LayoutTestController since many other functions like this are already there.

HTML should be in all caps (or all lower case in strange cases), not spelled "Html".

The DOM prefix is reserved for WebKit use for the Objective-C DOM bindings, so it’s not great to have DumpRenderTree use this prefix for one of its classes.
Comment 19 Joseph Pecoraro 2010-09-07 13:36:17 PDT
> The DOM prefix is reserved for WebKit use for the Objective-C DOM bindings, so
> it’s not great to have DumpRenderTree use this prefix for one of its classes.

This was meant to parts of the Objective-C DOM bindings. In this case, the
special DOMHTMLSelectElement functions defined in DOMPrivate.h. And, being
Obj-C specific it seemed natural to me to make it similar to the ObjCController.
I made the test platform/mac specific.


> Maybe it would be better to make this part of a "form testing" controller,
> or a "user input simulating" type controller, or maybe make it part of
> LayoutTestController since many other functions like this are already there.

If I move this to LayoutTestController.cpp all other ports would have an empty
implementation. I like the idea of a form testing controller but I still see this
as being unique because it deals with the Obj-C bindings.
Comment 20 Darin Adler 2010-09-07 14:05:16 PDT
If your intent is to test ObjC bindings then you probably want to look at the objCController rather than adding a new object. We can also use the bridging between JavaScript and ObjC to test things directly in some cases.
Comment 21 Joseph Pecoraro 2010-09-07 14:11:17 PDT
(In reply to comment #20)
> If your intent is to test ObjC bindings then you probably want to look at the
> objCController rather than adding a new object. We can also use the bridging
> between JavaScript and ObjC to test things directly in some cases.

Okay. I originally thought of adding onto that. I'll move these there, and
try to simplify the test a bit.

I also remember seeing this comment in EventSendingController:
http://trac.webkit.org/browser/trunk/WebKitTools/DumpRenderTree/mac/EventSendingController.mm#L810

>     810  // FIXME: It's not good to have a test hard-wired into this controller like this.
>     811  // Instead we need to get testing framework based on the Objective-C bindings
>     812  // to work well enough that we can test that way instead.

For -[EventSendingController fireKeyboardEventsToElement:] so I thought it might
make sense to make a new controller. But now I think it is complaining more about
the controller's implementation.
Comment 22 Darin Adler 2010-09-07 14:21:44 PDT
Joe, here’s an example:

We could add a function that allows you to call any Objective-C method with a set of arguments of a given type. On the Objective-C controller. In fact, I thought we had this a while back, but I guess not.

    objCController.callMethod(select, "_activateItemAtIndex:", 1);

We could probably find a way to make this work for at least simple cases.
Comment 23 Joseph Pecoraro 2010-09-07 16:13:31 PDT
(In reply to comment #22)
> Joe, here’s an example:
> 
> We could add a function that allows you to call any Objective-C method with a set of arguments of a given type. On the Objective-C controller. In fact, I thought we had this a while back, but I guess not.
> 
>     objCController.callMethod(select, "_activateItemAtIndex:", 1);
> 
> We could probably find a way to make this work for at least simple cases.

For simple cases this should be easy. But creating a generic version to support
multiple arguments, of various types (primitives and pointers) seemed to
trip up my implementations using performSelector and then NSInvocation.
I'd be interested to see a solution. My attempt used a WebScriptObject array
to pass in the arguments, so it looked something like:

  objCController.performSelector(obj, selector, [arg1, arg2]);

I walked the array and pulled out the (Obj-C) values using webScriptObjectAtIndex.
Passing these by address into NSInvocation gave me unexpected values.
My Obj-C knowledge is still pretty basic. If someone is interested in writing
something like this, the following links may be useful:

http://developer.apple.com/mac/library/documentation/Cocoa/Conceptual/ObjectiveC/Articles/ocSelectors.html
http://blog.jayway.com/2010/03/30/performing-any-selector-on-the-main-thread/
http://cocoawithlove.com/2008/03/construct-nsinvocation-for-any-message.html
http://stackoverflow.com/questions/313400/nsinvocation-for-dummies
Comment 24 Joseph Pecoraro 2010-09-07 16:14:50 PDT
Created attachment 66783 [details]
[PATCH] Automated Test
Comment 25 Joseph Pecoraro 2010-09-07 16:35:50 PDT
r66929 = 78464d71a5b4dfd640adff8da0faecb209c771fd
http://trac.webkit.org/changeset/66929

r66930 = 93c516208fcec52b1d18b82b137afc2f8b336931
http://trac.webkit.org/changeset/66930