Bug 174710 – WebDriver: handle click events on option elements

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

The spec explicitly says we should handle clicks on option elements in a different way. If the option item is a child of a select that allows multiple items, we should actually toggle the item selected state. See https://www.w3.org/TR/webdriver/#element-click. The main problem we have here is that client rects don't work at all for option elements in WebKit, so we are currenly always clicking at 0,0 for option elements. I guess it's because in WebKit option elements don't have a renderer, the thing is that it works in firefox and chromium. If we manage to make clicks work on option elements, maybe we can simply add Meta/Ctrl modifier to the mouse event when the option element should be toggled, since Meta/Ctrl + Click is indeed the desired behavior. I think this is causing several tests to fail, at least testIsSelectedAndToggle is failing sor sure because of this.

selenium/webdriver/common/api_example_tests.py F
================================================================================== short test summary info ===================================================================================
FAIL selenium/webdriver/common/api_example_tests.py::testIsSelectedAndToggle[WebKitGTK]

========================================================================================== FAILURES ==========================================================================================
_____________________________________________________________________________ testIsSelectedAndToggle[WebKitGTK] _____________________________________________________________________________

driver = <selenium.webdriver.webkitgtk.webdriver.WebDriver (session="4da33091-8ace-4991-bb2e-61c1c48d2dc4")>, pages = <conftest.Pages object at 0x7fa80ea2b950>

    def testIsSelectedAndToggle(driver, pages):
        pages.load("formPage.html")
        elem = driver.find_element_by_id("multi")
        option_elems = elem.find_elements_by_xpath("option")
        assert option_elems[0].is_selected()
        option_elems[0].click()
>       assert not option_elems[0].is_selected()
E       assert not True
E        +  where True = <bound method WebElement.is_selected of <selenium.webdriver.remote.webelement....4991-bb2e-61c1c48d2dc4", element="node-21CE8204-45CE-4C7C-AF6D-BF0B6A75D0D3")>>()
E        +    where <bound method WebElement.is_selected of <selenium.webdriver.remote.webelement....4991-bb2e-61c1c48d2dc4", element="node-21CE8204-45CE-4C7C-AF6D-BF0B6A75D0D3")>> = <selenium.webdriver.remote.webelement.WebElement (session="4da33091-8ace-4991-bb2e-61c1c48d2dc4", element="node-21CE8204-45CE-4C7C-AF6D-BF0B6A75D0D3")>.is_selected

selenium/webdriver/common/api_example_tests.py:178: AssertionError

[Radar WebKit Bug Importer]

<rdar://problem/33459305>

[Carlos Garcia Campos]

Actually we can't click in option elements in general, because we should be able to select also elements in a drop down menu list, or in a listbox even when the element is not visible in the scroll area. So, we need to handle option elements differently as explained in the spec.

[Carlos Garcia Campos]

Created attachment 317563 [details]
Patch

[Carlos Garcia Campos]

Created attachment 317564 [details]
Patch

[Build Bot]

Attachment 317564 [details] did not pass style-queue:


ERROR: Source/WebDriver/Session.h:138:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.h:140:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.cpp:734:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.cpp:1199:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Created attachment 317573 [details]
Try to fix mac builds

[Build Bot]

Attachment 317573 [details] did not pass style-queue:


ERROR: Source/WebDriver/Session.h:138:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.h:140:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.cpp:734:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.cpp:1199:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

Comment on attachment 317573 [details]
Try to fix mac builds

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

Two main issues we need to resolve:

- move isOptionElement to driver-side JS query
- figure out spec-correctness for DOM events

> Source/WebDriver/ChangeLog:20
> +        (WebDriver::CommandResult::httpStatusCode const): ADd ElementNotSelectable.

Nit: Add

> Source/WebKit/UIProcess/Automation/Automation.json:430
> +                { "name": "isOptionElement", "type": "boolean", "description": "If the element is an HTML option element" }

The driver should just use a small JS snippet to do this, unless I'm missing something. Unlike the other out-parameters, this can be easily computed using client JS. There are cases where safaridriver does this, for example to see if an <input> is type=file and whether it has "multiple" attribute or not. I don't want to extend the protocol every time we need to do a type test on an element. Reducing message traffic by one message is not important since the REST service and browser are using fast message passing (hopefully). On macOS, our async XPC is basically free.

> Source/WebKit/UIProcess/Automation/Automation.json:435
> +            "name": "selectOptionElement",

This seems fine, but maybe call it toggleOptionElement?

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:866
> +    auto callback = m_selectOptionElementCallbacks.take(callbackID);

(Someday I'm going to write a generic implementation of this callback mechanism, as it's something we do all over WebKit2.)

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:613
> +    String elementNotInteractableErrorType = Inspector::Protocol::AutomationHelpers::getEnumConstantValue(Inspector::Protocol::Automation::ErrorMessage::ElementNotInteractable);

This seems like the right thing to do, but I can't find it in the spec. Should I file bug?

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:626
> +    if (selectElement->isDisabledFormControl() || optionElement.isDisabledFormControl()) {

Ditto.

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:632
> +    selectElement->optionSelectedByUser(optionElement.index(), true, selectElement->multiple());

This won't fire mouseover/move/down/up/click events like the spec wants. We'll probably have to simulate those events ourselves as the only way to do it natively would be to try and click the native widgets. I think that would be problematic for dropdown menus as the interaction is a bit more subtle than the DOM events would lead you to believe.

[Carlos Garcia Campos]

Comment on attachment 317573 [details]
Try to fix mac builds

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

>> Source/WebKit/UIProcess/Automation/Automation.json:430
>> +                { "name": "isOptionElement", "type": "boolean", "description": "If the element is an HTML option element" }
> 
> The driver should just use a small JS snippet to do this, unless I'm missing something. Unlike the other out-parameters, this can be easily computed using client JS. There are cases where safaridriver does this, for example to see if an <input> is type=file and whether it has "multiple" attribute or not. I don't want to extend the protocol every time we need to do a type test on an element. Reducing message traffic by one message is not important since the REST service and browser are using fast message passing (hopefully). On macOS, our async XPC is basically free.

I didn't do this for efficiency, but just for convenience :-) the code in the driver side is a lot simpler.

>> Source/WebKit/UIProcess/Automation/Automation.json:435
>> +            "name": "selectOptionElement",
> 
> This seems fine, but maybe call it toggleOptionElement?

It only toggles when the select is multiple, otherwise it selects the element (in case of combo element).

>> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:613
>> +    String elementNotInteractableErrorType = Inspector::Protocol::AutomationHelpers::getEnumConstantValue(Inspector::Protocol::Automation::ErrorMessage::ElementNotInteractable);
> 
> This seems like the right thing to do, but I can't find it in the spec. Should I file bug?

The spec says we should do the select algorithm only for option elements not for optgroup ones, so in theory we should do the normal click for group options, but I don't think it makes sense. First, it won't have any effect if we manage to click it, but it's not easy to clikc it anyway, it could be hidden in the drop down list or off the list box scroll area. So, yes, I think the spec should mention what to do with optgroups.

>> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:626
>> +    if (selectElement->isDisabledFormControl() || optionElement.isDisabledFormControl()) {
> 
> Ditto.

Same here, the spec doesn't say anything, element not selectable error is only mentioned in the errors table. I had to add this to fix a couple of selenium test.

>> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:632
>> +    selectElement->optionSelectedByUser(optionElement.index(), true, selectElement->multiple());
> 
> This won't fire mouseover/move/down/up/click events like the spec wants. We'll probably have to simulate those events ourselves as the only way to do it natively would be to try and click the native widgets. I think that would be problematic for dropdown menus as the interaction is a bit more subtle than the DOM events would lead you to believe.

I know, even when it's in the spec I don't think selenium will expect we fire all those events, at least we fire the important ones also mentioned in the spec, the input and change events. I'll add a FIXME about the other events, but I wouldn't block this feature on those.

[Blaze Burg]

Comment on attachment 317573 [details]
Try to fix mac builds

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

>>> Source/WebKit/UIProcess/Automation/Automation.json:435
>>> +            "name": "selectOptionElement",
>> 
>> This seems fine, but maybe call it toggleOptionElement?
> 
> It only toggles when the select is multiple, otherwise it selects the element (in case of combo element).

Okay, let's leave it as is.

>>> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:632
>>> +    selectElement->optionSelectedByUser(optionElement.index(), true, selectElement->multiple());
>> 
>> This won't fire mouseover/move/down/up/click events like the spec wants. We'll probably have to simulate those events ourselves as the only way to do it natively would be to try and click the native widgets. I think that would be problematic for dropdown menus as the interaction is a bit more subtle than the DOM events would lead you to believe.
> 
> I know, even when it's in the spec I don't think selenium will expect we fire all those events, at least we fire the important ones also mentioned in the spec, the input and change events. I'll add a FIXME about the other events, but I wouldn't block this feature on those.

Agree, it seems kinda pointless and hopefully people don't depend on these events.

[Carlos Garcia Campos]

Created attachment 318034 [details]
Updated patch

[Build Bot]

Attachment 318034 [details] did not pass style-queue:


ERROR: Source/WebDriver/Session.h:140:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.cpp:1194:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.cpp:1240:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

Comment on attachment 318034 [details]
Updated patch

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

enthusiastic r=me!

> Source/WebKit/UIProcess/Automation/Automation.json:437
> +                { "name": "browsingContextHandle", "$ref": "BrowsingContextHandle", "description": "The handle for the browsing context." },

Nit: you can just omit these comments if they don't add much.

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:509
> +static WebCore::Element* elementContainer(WebCore::Element& element)

Nit: maybe containerElementForElement() to make it read better...

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:559
> +    auto* containerElement = elementContainer(*coreElement);

... down here.

[Carlos Garcia Campos]

Committed r220740: <http://trac.webkit.org/changeset/220740>