[webkit-reviews] review denied: [Bug 47368] onclick on option elements doesn't work : [Attachment 70266] Proposed patch for handling onclick event on OptionElement

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Dinu Jacob <dinu.jacob at Nokia.com>'s
request for review:
Bug 47368: onclick on option elements doesn't work
https://bugs.webkit.org/show_bug.cgi?id=47368

Attachment 70266: Proposed patch for handling onclick event on OptionElement
https://bugs.webkit.org/attachment.cgi?id=70266&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=70266&action=review

The code change here looks pretty good. In the WebKit project we require a
regression test along with each bug fix.

Someone needs to construct a test using eventSender to demonstrate that this
click event is sent, and include the case where there is no change to be sure a
click event is sent even when you click on the already-selected option. You
should test in other browsers to be sure that is the expected behavior, too. We
also need test coverage of both forms of select elements with a small size
(menu style) and select elements with a large size (list style).

> WebCore/html/HTMLSelectElement.cpp:92
> +    // Send click event to the item to trigger registered event listener if
any
> +    HTMLOptionElement* optionElement =
static_cast<HTMLOptionElement*>(item(optionIndex));
> +    optionElement->dispatchEvent(Event::create(eventNames().clickEvent,
false, false));

The comment here is not useful. All events go to registered event listeners, so
this adds nothing. There is no need to cast item to HTMLOptionElement* just to
call send it a click event.

    item(optionIndex)->dispatchEvent(Event::create(eventNames().clickEvent,
false, false));

But also, is it guaranteed that optionIndex is a good index? Could the item
function return 0?