Summary: | onclick on option elements doesn't work | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dinu Jacob <dinu.jacob> | ||||||||||||||
Component: | Forms | Assignee: | 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
Dinu Jacob
2010-10-07 12:39:35 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 Created attachment 70266 [details]
Proposed patch for handling onclick event on OptionElement
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?
(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. (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 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? (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. Dinu, any update on the error? (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. (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? 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?
(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. Created attachment 72858 [details]
First pass of manual test; updates based on comments
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. Created attachment 79063 [details]
With change and manual test case
(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. (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 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? Created attachment 80937 [details]
With changes from review comments
(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? (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 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. 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? Created attachment 116029 [details]
layout test
layout test to send click events.
(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. (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. Created attachment 116944 [details]
proposed patch
proposed patch
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? That should certainly be mentioned in ChangeLog. Comment on attachment 116944 [details] proposed patch Attachment 116944 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10685156 (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. (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. 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 on attachment 116944 [details]
proposed patch
We can easily simulate clicks from DRT. grep for tests which use eventSender.
(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. 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. |