setValue() TAB causes unintended actions

[andrewnicols]

I agree that it's correct to remove the Syn library. I did the same in my attempt to rewrite the Mink Driver, but I don't think that pressing TAB to trigger the change event is correct.

In a form this may cause something else to be focused which has unintended consequences.

In my early testing I've encountered two issues where this just doesn't work.

In our case, before each element, there is a "Help" icon which displays a Bootstrap popover on focus. In some cases this causes other elements in the DOM to be obscured. For example:

Likewise we have a field type, "Inplace editable", which transforms a string (i.e. section heading) into a Text input field. It saves the content on [RETURN], but a blur or [ESCAPE] cancels it.

Personally I think it wrong for the Mink driver to perform any attempt to blur the field, but the decision we have to make is: a) do we break from the Mink specification and just set the value. Leave it up to the calling code and/or browser to decide when the Blur should trigger; or b) fire a new CustomEvent('change', ...) on the Element to mimic the documented behaviour for Mink Drivers; or c) find a way to actually blur the field without changing the focus; or d) keep it as is.

The rationale you've made for using TAB is that it prevents multiple change events, but I would argue that's really something that the application should be aware of regardless - I seem to recall that there are times where a browser can cause spammy change events (older versions of IE I think).

In many ways option (b) is the least evil as this is the least unnecessary change from the documented behaviour and is easily achieved:

        $script = <<<EOF
{{ELEMENT}}.dispatchEvent(new Event("change", {
    bubbles: true,
    cancelable: false,
}));
EOF;

        $this->executeJsOnXpath($xpath, $script);

In my mind the 'correct' option is (a), but I suspect that this will cause failures all over the place for people. The setValue() function should only set a value. It should not make assumptions about the nature of page, or how interactions with that element are designed.

I'm happy to provide patches for options a or b. I don't really have any other solutions just yet for c.

[andrewnicols]

Note: If the decision is for d, can I suggest that the blur be moved to a new function, i.e.

public function setValue($xpath, $value)
{
    // ...
    $element->sendKeys($value);

    $this->blurElement($element);
}

protected function blurElement(RemoteWebElement $element)
{
    $element->sendKeys(WebDriverKeys::TAB);
}

This will at least allow me extend the driver, and then to override the blurElement function to match the documented behaviour.

[andrewnicols]

Hi @oleg-andreyev,

Anything I can do to progress this? I’m really keen to get moved over to php-webdriver/webdriver.

[oleg-andreyev]

@andrewnicols haven't looked into this issue yet, I'm in the progress of stabilizing build for the main branch.

[oleg-andreyev]

@andrewnicols I agree that sending TAB wasn't a great idea! I think it's better to go with B.

I've tried to play with "input" event instead of "change" event and few tests are failing, so let's go just with "change" event.

Would you mind creating a PR? and please create appropriate test-case to https://github.com/oleg-andreyev/driver-testsuite/tree/integration-branch

[andrewnicols]

I've created a branch for this, but I'm not sure what additional tests I should write.

The tests in tests/Js/ChangeEventTest.php should already cover this. I can add a test which ensures that the focus is still on that element, but that seems inappropriate. What do you think?

[oleg-andreyev]

I can add a test which ensures that the focus is still on that element, but that seems inappropriate. What do you think?

yes, it's the exact scenarios I was thinking