<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.
Created attachment 66532 [details] [PATCH] Proposed Change
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?
> 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.
You're absolutely right.* My grammar on Bugzilla is atrocious.
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.
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.
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.
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?
(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.
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 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?
> 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.
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>.
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 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.
Darin could you review the updated patches?
Thanks! Does that apply to both patches? I also removed some unused JavaScript code from the LayoutTests I added.
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.
> 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.
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.
(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.
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.
(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
Created attachment 66783 [details] [PATCH] Automated Test
r66929 = 78464d71a5b4dfd640adff8da0faecb209c771fd http://trac.webkit.org/changeset/66929 r66930 = 93c516208fcec52b1d18b82b137afc2f8b336931 http://trac.webkit.org/changeset/66930