Bug 45192 – Provide a way to trigger a <select multiple> onchange event on changes

• Component: WebKit API
• Status: RESOLVED FIXED

[Joseph Pecoraro]

<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.

[Joseph Pecoraro]

Created attachment 66532 [details]
[PATCH] Proposed Change

[Darin Adler]

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?

[Joseph Pecoraro]

> 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.

[Joseph Pecoraro]

You're absolutely right.* My grammar on Bugzilla is atrocious.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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?

[Darin Adler]

(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.

[Darin Adler]

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.

[Darin Adler]

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?

[Joseph Pecoraro]

> 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.

[Joseph Pecoraro]

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>.

[Joseph Pecoraro]

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.

[Eric Seidel (no email)]

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.

[Joseph Pecoraro]

Darin could you review the updated patches?

[Joseph Pecoraro]

Thanks! Does that apply to both patches?

I also removed some unused JavaScript code from the LayoutTests I added.

[Darin Adler]

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.

[Joseph Pecoraro]

> 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.

[Darin Adler]

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.

[Joseph Pecoraro]

(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.

[Darin Adler]

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.

[Joseph Pecoraro]

(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

[Joseph Pecoraro]

Created attachment 66783 [details]
[PATCH] Automated Test

[Joseph Pecoraro]

r66929 = 78464d71a5b4dfd640adff8da0faecb209c771fd
http://trac.webkit.org/changeset/66929

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