Bug 47368 – onclick on option elements doesn't work

• Component: Forms
• Status: ASSIGNED

[Dinu Jacob]

onclick event on option elements in drop-down list (Select Element) is not triggered.

[Dinu Jacob]

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

[Dinu Jacob]

Created attachment 70266 [details]
Proposed patch for handling onclick event on OptionElement

[Darin Adler]

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?

[Dinu Jacob]

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

[Alexey Proskuryakov]

Looks like a duplicate of bug 35533 and/or bug 17516.

[Dinu Jacob]

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

[Darin Adler]

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?

[Dinu Jacob]

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

[Suresh Voruganti]

Dinu, any update on the error?

[Dinu Jacob]

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

[Dinu Jacob]

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

[Alexey Proskuryakov]

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?

[Dinu Jacob]

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

[Dinu Jacob]

Created attachment 72858 [details]
First pass of manual test; updates based on comments

[Eric Seidel (no email)]

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.

[Dinu Jacob]

Created attachment 79063 [details]
With change and manual test case

[Dinu Jacob]

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

[Dinu Jacob]

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

[Dimitri Glazkov (Google)]

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?

[Dinu Jacob]

Created attachment 80937 [details]
With changes from review comments

[Ryosuke Niwa]

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

[Dinu Jacob]

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

[Adam Barth]

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.

[Antaryami Pandia]

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?

[Antaryami Pandia]

Created attachment 116029 [details]
layout test

layout test to send click events.

[Tony Chang]

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

[Antaryami Pandia (apandia)]

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

[Antaryami Pandia (apandia)]

Created attachment 116944 [details]
proposed patch

proposed patch

[Dinu Jacob]

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?

[Alexey Proskuryakov]

That should certainly be mentioned in ChangeLog.

[WebKit Review Bot]

Comment on attachment 116944 [details]
proposed patch

Attachment 116944 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10685156

[Antaryami Pandia (apandia)]

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

[Antaryami Pandia (apandia)]

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

[Antaryami Pandia (apandia)]

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.

[Eric Seidel (no email)]

Comment on attachment 116944 [details]
proposed patch

We can easily simulate clicks from DRT.  grep for tests which use eventSender.

[Ryosuke Niwa]

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

[Ahmad Saleem]

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.