Bug 47368

Summary: onclick on option elements doesn't work
Product: WebKit Reporter: Dinu Jacob <dinu.jacob>
Component: FormsAssignee: Dinu Jacob <dinu.jacob>
Status: ASSIGNED ---    
Severity: Normal CC: ahmad.saleem792, akeerthi, annevk, antaryami.pandia, ap, cshu, dglazkov, karlcow, rniwa, suresh.voruganti, tkent, tony, webkit.review.bot, yael
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed patch for handling onclick event on OptionElement
darin: review-
First pass of manual test; updates based on comments
eric: review-
With change and manual test case
dglazkov: review-, dglazkov: commit-queue-
With changes from review comments
abarth: review-
layout test
none
proposed patch eric: review-, webkit.review.bot: commit-queue-

Description Dinu Jacob 2010-10-07 12:39:35 PDT
onclick event on option elements in drop-down list (Select Element) is not triggered.
Comment 1 Dinu Jacob 2010-10-08 10:06:41 PDT
Mouse events are not sent to OptionElement and hence, the registered event listeners are not called either. So, if the item has an onclick event registered, it is never triggered. 

Test case: http://testsuite.nokia-boston.com/content/esmp_Sec10/optionObject/option_onClick1.html
Comment 2 Dinu Jacob 2010-10-08 10:08:14 PDT
Created attachment 70266 [details]
Proposed patch for handling onclick event on OptionElement
Comment 3 Darin Adler 2010-10-08 10:24:52 PDT
Comment on attachment 70266 [details]
Proposed patch for handling onclick event on OptionElement

Should preventDefault or returning a value from the click event handler prevent click handling?
Comment 4 Dinu Jacob 2010-10-08 11:32:39 PDT
(In reply to comment #3)
> (From update of attachment 70266 [details])
> Should preventDefault or returning a value from the click event handler prevent click handling?

I hope I am understanding this correctly... I think even is the click event handler returns a value (i.e is handled), click handling should not be prevented as the basic functionality there is changing the item being selected.

Not sure which event preventDefault is referring to?  By the time we reach HTMLSelectElement::setSelectedIndexByUser, the mouse event has already been handled.
Comment 5 Alexey Proskuryakov 2010-10-08 19:46:01 PDT
Looks like a duplicate of bug 35533 and/or bug 17516.
Comment 6 Dinu Jacob 2010-10-09 05:42:35 PDT
(In reply to comment #5)
> Looks like a duplicate of bug 35533 and/or bug 17516.

Bug 35533 is about onclick event on the select tag. This bug is about onclick on the option tag. I couldn't verify bug 17516 (I couldn't follow through the test case on that site - it might have changed as the bug is old).
Comment 7 Darin Adler 2010-10-13 17:34:29 PDT
Comment on attachment 70266 [details]
Proposed patch for handling onclick event on OptionElement

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?
Comment 8 Dinu Jacob 2010-10-14 00:24:06 PDT
(In reply to comment #7)
> (From update of attachment 70266 [details])
> 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?

Thanks for the comments. Will make the changes and add required test cases.
Comment 9 Suresh Voruganti 2010-10-26 12:37:29 PDT
Dinu, any update on the error?
Comment 10 Dinu Jacob 2010-11-01 13:29:40 PDT
(In reply to comment #7)
> (From update of attachment 70266 [details])
> 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).
When the same option is selected, other browsers trigger onclick. There are existing tests that cover list-style: 
LayoutTests/fast/forms/ option-mouseevents.html

> 
> > 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));
> 
Will remove the comment.

> But also, is it guaranteed that optionIndex is a good index? Could the item function return 0?
optionIndex is validated before we reach this function.
Comment 11 Dinu Jacob 2010-11-01 13:32:34 PDT
(In reply to comment #7)
> (From update of attachment 70266 [details])
> 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).

Studied the existing tests to create a test to send a click event to the select element intended for an option element in the list (which is what happens when the user selects an option from the list). 

To send a click event for an option element, need to know the position of the option element. In the case of menu-list, the list expands only on clicking on the select element. However, it is not possible to programmatically expand the select list (by sending a click event to the select). I have a test page where a click event is sent to the select element but even though it receives the click event, the menu list is not expanded (it is the same in all the browsers). 

I can send a click event directly to the option element but that will not test the use case as that is not what happens in the code (the event doesn't go directly to the option element). 

I am not sure how to proceed in this case and would like your input. Thank you very much for your help and time.

> 
> > 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?
Comment 12 Alexey Proskuryakov 2010-11-01 22:57:12 PDT
Unless I missed some improvement, there is no way to interact with pop-up menus in DumpRenderTree. You could make a manual test in WebCore/manual-tests.

But also, this change doesn't look exclusive to pop-ups. Does it change behavior for multi-line selects? Are there two click events dispatched now for those?

> Not sure which event preventDefault is referring to?  By the time we reach HTMLSelectElement::setSelectedIndexByUser, the mouse event has already been handled.

I don't understand what you're saying here. With your patch, a click event is dispatched from HTMLSelectElement::setSelectedIndexByUser(). Should calling preventDefault() in its handler have any effect? What do other browsers do?

I don't see any discussion of making a selection with keyboard. Should a click event be dispatched on Enter or Return key press, too?
Comment 13 Dinu Jacob 2010-11-02 09:09:54 PDT
(In reply to comment #12)
> Unless I missed some improvement, there is no way to interact with pop-up menus in DumpRenderTree. You could make a manual test in WebCore/manual-tests.
Thanks. I was attempting that without knowing that DumpRenderTree didn't have that capability.
> 
> But also, this change doesn't look exclusive to pop-ups. Does it change behavior for multi-line selects? Are there two click events dispatched now for those?
I can add a check to dispatch the clickEvent only for menu list. In the normal list-box tests I ran, I didn't hit this function( When I look at the history of changes, the test case is selecting the items programmatically - don't know if the path is different in this case). 
> 
> > Not sure which event preventDefault is referring to?  By the time we reach HTMLSelectElement::setSelectedIndexByUser, the mouse event has already been handled.
> 
> I don't understand what you're saying here. With your patch, a click event is dispatched from HTMLSelectElement::setSelectedIndexByUser(). Should calling preventDefault() in its handler have any effect? What do other browsers do?
Verified on Mozilla that the default action is taking place even when the click event handler returns false.
> 
> I don't see any discussion of making a selection with keyboard. Should a click event be dispatched on Enter or Return key press, too?
On Mac, when the return key is pressed, HTMLSelectElement is still getting a mouse press event (isKeyBoardEvent is false ). On Linux qt version, as the popup is QComboBox and it has installed event filter to grab the events, the client just gets the indication that an item has been selected. Hence, it is not possible in both cases to determine whether it was a keyboard event or mouse click.
Comment 14 Dinu Jacob 2010-11-03 13:27:23 PDT
Created attachment 72858 [details]
First pass of manual test; updates based on comments
Comment 15 Eric Seidel (no email) 2010-12-24 17:32:18 PST
Comment on attachment 72858 [details]
First pass of manual test; updates based on comments

I'm not sur ewhy you have a WebCore ChangeLog with no WebCore changes?  Also, you need expected results for your new layout test.  You can use window.eventSender to actually make the click.  It doesn't look lik eyour layout test will actually work as-is.  http://webkit.org/quality/testwriting.html   or see http://trac.webkit.org/wiki for a bunch of information about writing layout tests.
Comment 16 Dinu Jacob 2011-01-15 10:32:40 PST
Created attachment 79063 [details]
With change and manual test case
Comment 17 Dinu Jacob 2011-01-15 10:34:47 PST
(In reply to comment #15)
> (From update of attachment 72858 [details])
> I'm not sur ewhy you have a WebCore ChangeLog with no WebCore changes? 
Sorry about that - I have uploaded a patch with the change.

 Also, you need expected results for your new layout test.  You can use window.eventSender to actually make the click.  It doesn't look lik eyour layout test will actually work as-is.  http://webkit.org/quality/testwriting.html   or see http://trac.webkit.org/wiki for a bunch of information about writing layout tests.

I have to do a manual test as the pop-up is blocking and never gets the click event send. I added it incorrectly under layout tests - I have now added it under manual tests where the other select list tests are also placed.

Thank you.
Comment 18 Dinu Jacob 2011-01-15 10:38:26 PST

(In reply to comment #15)
(From update of attachment 72858 [details] [details])
> I'm not sur ewhy you have a WebCore ChangeLog with no WebCore changes? 
Sorry about that - I have uploaded a patch with the change.

> 
>  Also, you need expected results for your new layout test.  You can use >window.eventSender to actually make the click.  It doesn't look lik eyour layout >test will actually work as-is.  http://webkit.org/quality/testwriting.html   or >see http://trac.webkit.org/wiki for a bunch of information about writing layout >tests.
> 
 I have to do a manual test as the pop-up is blocking and never gets the click event sent. I added it incorrectly under layout tests - I have now added it under manual tests where the other select list tests are also placed.
 
Thank you.
Comment 19 Dimitri Glazkov (Google) 2011-01-25 21:30:17 PST
Comment on attachment 79063 [details]
With change and manual test case

View in context: https://bugs.webkit.org/attachment.cgi?id=79063&action=review

Sad that manual test is the only option.

> Source/WebCore/html/HTMLSelectElement.cpp:102
> +    item(optionIndex)->dispatchEvent(Event::create(eventNames().clickEvent, false, false));

I think you're looking for dispatchSimulatedClick?
Comment 20 Dinu Jacob 2011-02-02 11:43:57 PST
Created attachment 80937 [details]
With changes from review comments
Comment 21 Ryosuke Niwa 2011-04-11 15:48:13 PDT
(In reply to comment #19)
> (From update of attachment 79063 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79063&action=review
> 
> Sad that manual test is the only option.

Can't we use eventSender here?
Comment 22 Dinu Jacob 2011-04-12 08:43:18 PDT
(In reply to comment #21)
> (In reply to comment #19)
> > (From update of attachment 79063 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=79063&action=review
> > 
> > Sad that manual test is the only option.
> 
> Can't we use eventSender here?

It is not possible to use it with pop-up menus as pop-menus require user input.
Comment 23 Adam Barth 2011-05-05 01:06:29 PDT
Comment on attachment 80937 [details]
With changes from review comments

View in context: https://bugs.webkit.org/attachment.cgi?id=80937&action=review

> Source/WebCore/ChangeLog:12
> +        * manual-tests/select-option-click.html: Added.

Why is this a manual test?  This seems like something we could test automatically.

> Source/WebCore/html/HTMLSelectElement.cpp:102
> +    item(optionIndex)->dispatchSimulatedClick(0, false, false);

Will this fire if the user selects and option using the keyboard?  This seems wrong.
Comment 24 Antaryami Pandia 2011-11-20 22:06:19 PST
It seems, the event sender doesn't works properly with native windows, which is used by most platforms for displaying the drop-down.

I have written a layout test to send click events, but it doesn't work out as expected.I have attached the layout test for reference.Am I missing something?
Comment 25 Antaryami Pandia 2011-11-20 22:07:09 PST
Created attachment 116029 [details]
layout test

layout test to send click events.
Comment 26 Tony Chang 2011-11-28 15:33:41 PST
(In reply to comment #24)
> It seems, the event sender doesn't works properly with native windows, which is used by most platforms for displaying the drop-down.
> 
> I have written a layout test to send click events, but it doesn't work out as expected.I have attached the layout test for reference.Am I missing something?

You are correct, drop down menus are separate windows and eventSender can only send events to the current window.  It wouldn't be reliable anyway since the position of the drop down menu is platform dependent (e.g., OSX shows menu items above and below the selected item).

We have manual tests for most tests involving drop down menus.
Comment 27 Antaryami Pandia (apandia) 2011-11-29 04:16:08 PST
(In reply to comment #26)
> You are correct, drop down menus are separate windows and eventSender can only send events to the current window.  It wouldn't be reliable anyway since the position of the drop down menu is platform dependent (e.g., OSX shows menu items above and below the selected item).
> 
> We have manual tests for most tests involving drop down menus.

Thanks for the information.
Comment 28 Antaryami Pandia (apandia) 2011-11-29 04:21:51 PST
Created attachment 116944 [details]
proposed patch

proposed patch
Comment 29 Dinu Jacob 2011-11-29 06:27:39 PST
This patch seems to be the same as what I had earlier  including (except for merging to the latest code), and without any reference to my changes :(. Was anything changed?
Comment 30 Alexey Proskuryakov 2011-11-29 09:01:59 PST
That should certainly be mentioned in ChangeLog.
Comment 31 WebKit Review Bot 2011-11-29 10:12:25 PST
Comment on attachment 116944 [details]
proposed patch

Attachment 116944 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10685156
Comment 32 Antaryami Pandia (apandia) 2011-11-30 02:04:47 PST
(In reply to comment #30)
> That should certainly be mentioned in ChangeLog.

yes, you are right. I was also thinking that.But was not sure how to do that.As per you suggestion I will now mention it in the changelog.
Comment 33 Antaryami Pandia (apandia) 2011-11-30 10:07:18 PST
(In reply to comment #23)
> (From update of attachment 80937 [details])
> Will this fire if the user selects and option using the keyboard?  This seems wrong.

Yes, you are right.verified with firefox also.I have missed this point in the patch.Will work on this and update.
Comment 34 Antaryami Pandia (apandia) 2011-12-04 21:11:32 PST
With the current "valueChanged" of PopupMenuClient, it is not possible to figure out, whether this selection made by user is done on key events or mouse events.The current "valueChanged" is as follows:-
    valueChanged(unsigned listIndex, bool fireEvents = true);

I think, we can add an extra argument for this purpose.But since this is an change in the interface, I would like to get some feedback before proceeding with the change.
Comment 35 Eric Seidel (no email) 2012-02-13 17:58:56 PST
Comment on attachment 116944 [details]
proposed patch

We can easily simulate clicks from DRT.  grep for tests which use eventSender.
Comment 36 Ryosuke Niwa 2012-02-13 18:00:21 PST
(In reply to comment #35)
> (From update of attachment 116944 [details])
> We can easily simulate clicks from DRT.  grep for tests which use eventSender.

We can't because popup menus of select elements are not rendered by WebKit.
Comment 37 Ahmad Saleem 2023-05-23 02:24:29 PDT
I am still able to reproduce this bug in Safari 16.5 and WebKit ToT whereas Chrome Canary 115 and Firefox Nightly 115 both send 'o'clock' event and show following:

PASS: onClick on Option element is successfull.