[webkit-reviews] review denied: [Bug 174710] WebDriver: handle click events on option elements : [Attachment 317573] Try to fix mac builds

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Aug 8 09:26:31 PDT 2017


Brian Burg <bburg at apple.com> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 174710: WebDriver: handle click events on option elements
https://bugs.webkit.org/show_bug.cgi?id=174710

Attachment 317573: Try to fix mac builds

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




--- Comment #8 from Brian Burg <bburg at apple.com> ---
Comment on attachment 317573
  --> https://bugs.webkit.org/attachment.cgi?id=317573
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::Protoco
l::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.


More information about the webkit-reviews mailing list