Prevent firing the "change" event twice

[hchonov]

Fixes #285

[aik099]

The #244 pull request changed TAB adding behavior that indirectly made postValue selenium call to trigger change event into into direct triggering of change event, because Chrome 53 outputed that TAB instead of loosing focus of the element. Now here you revert that behavior in hopes, that postValue Selenium call would do everything correctly without workaround code, that existed there for years.

Anyways because of massive failure of test suite on Travis CI I can't have solid proof that it doesn't break any Mink driver tests.

Please, if possible, send another PR for fixing test suite failures (only change driver code).

[hchonov]

Please, if possible, send another PR for fixing test suite failures (only change driver code).

I am sorry, but I am not sure I understand what exactly I have to do?

[aik099]

This, an and many other, projects have tests suites in place to ensure that new code changes won't break any of existing behavior. PHP-based projects like to use PHPUnit for that.

The Travis CI is website, that runs these tests for us on each PR so before merging we'll know if it breaks anything or not.

Unfortunately due rapid Selenium development lately most of the tests now fail and core maintainers of the project are also very busy.

The tests link for your PR is: https://travis-ci.org/minkphp/MinkSelenium2Driver/builds/392645545

In there there is a build for every PHP version supported. For example PHP 5.4 link is https://travis-ci.org/minkphp/MinkSelenium2Driver/jobs/392645547 and it fails with a PHP Fatal Error:

PHP Fatal error:  Call to a member function window_handles() on a non-object in /home/travis/build/minkphp/MinkSelenium2Driver/src/Selenium2Driver.php on line 492

The window_handles is virtual method of used library to communicate to Selenium. I have no idea why this method no longer works.

For PHP 7.1/7.0 the build is terribly slow (~40 minutes): https://travis-ci.org/minkphp/MinkSelenium2Driver/jobs/392645550

The PHP 7.0 build using remote Selenium instance finishes in time and no fatal errors: https://travis-ci.org/minkphp/MinkSelenium2Driver/jobs/392645553 , but some change JS event firing tests weren't passed.

[hchonov]

Hah, I am aware of all the testing stuff :). But thanks for explaining it. I only didn't understand what you mean by

PR for fixing test suite failures (only change driver code).

The build with the remote Selenium instance is testing with Firefox. As mentioned in the issue I've noticed the problem with Chrome.

Let's see what happens when we use Chrome.

[aik099]

PR for fixing test suite failures (only change driver code).

Mink is the main project. It has different repos for all of it's drivers. And there is another repo to store tests that all drivers must pass. If the test suite is failing for a particular driver, then that driver's code needs to be fixed instead of altering the test code.

[hchonov]

@aik099 I've tried what you've suggested in #243 (comment) and it fixes my test fails. \Behat\Mink\Tests\Driver\Js\ChangeEventTest fails only on one place locally, but it is the same with or without the change.

I was thinking about the problem and while I cannot really define our use case I think that it is much safer to call blur on the currently focused element instead of simply triggering the change event for the given xpath.

I think that
$this->executeScript("document.activeElement && document.activeElement.blur()");
is more secure than
$this->trigger($xpath, 'change');
as it for sure will not trigger the change event twice for an element in case the element has already lost the focus after we've set the value on it and before we've triggered the change event.

I think that we probably still have to ensure that the currently focused element is still the element we've set the value to.

[aik099]

The change event triggering via blur has a known side effect: you can't use any typeaheads, where suggestions appear as you type and you want to pick a suggestion. When doing blur the suggestions disappear usually.

[hchonov]

But the change event is triggered once you loose focus and not while typing. So why do we want to trigger the change event while typing?

[aik099]

But the change event is triggered once you loose focus and not while typing. So why do we want to trigger the change event while typing?

Now I get it. Then you're correct.

[hchonov]

I've just updated the PR so that we remove the focus from the currently focused element only in case this is still the element on which we've set the value to. If it is not, then there is nothing to do, as if the element has lost the focus in the meanwhile, then the change event will have been triggered already.

[aik099]

This PR build https://travis-ci.org/minkphp/MinkSelenium2Driver/builds/394658837 analysis:

1. All PHP 5.x builds fail with Fatal Error due non-existing window_handles method. I've created an issue for Selenium API library creator: The "window_handles" method missing on PHP 5.x instaclick/php-webdriver#85
2. We don't care about HHVM anymore (it crashed).
3. Two of three PHP 7 builds (https://travis-ci.org/minkphp/MinkSelenium2Driver/jobs/394658846 and https://travis-ci.org/minkphp/MinkSelenium2Driver/jobs/394658847) failed almost every test and hang and where terminated by Travis CI.
4. The 3rd PHP 7 build (https://travis-ci.org/minkphp/MinkSelenium2Driver/jobs/394658849) failed 2 tests, but passed all other tests, including one for onchange event being fired only once.

To conclude I think this PR is ready to be merged.

[hchonov]

To conclude I think this PR is ready to be merged.
Is there anything else I have to do?

[aik099]

Merging, thanks @hchonov .

[hchonov]

Thank you, @aik099 .

[pfrenssen]

This change is causing my tests to break that are filling in a Drupal autocomplete field. If I revert commit d8adafd then my tests are green again.

This matches what @aik099 predicted in #286 (comment) - this is a "typeahead" feature which now doesn't work any more because the field is being blurred.

[pfrenssen]

Wouldn't it be better to just trigger a change event if we detect that the element still has focus?

If the element still has focus, then a change event hasn't triggered yet, and it is safe to trigger it at that point. That approach would better preserve backwards compatibility.

if (document.activeElement === node) {
  document.activeElement.blur(); // <<< Trigger a change event instead of blurring
}

[aik099]

@pfrenssen , thanks for testing this out. I wish you had ability to do this prior to the merge. Maybe we should add typeahead test to the test suite to avoid this break again.

@hchonov , could you please test the change proposed in #286 (comment) and see if that doesn't break anything on your side. If all is fine I'll happily merge PR.

The typeahead control support isn't a feature, but an unexpected side effect of a way how change event is triggered.

[pfrenssen]

I am trying to follow the development quite closely but this wasn't on my radar unfortunately. I am happy to assist in any way I can. I can perhaps try to provide a typeahead test.

[aik099]

The test for typeahead is likely to be located in https://github.com/minkphp/driver-testsuite/blob/master/tests/Js/JavascriptTest.php , but I'm not sure if other JS-enabled drivers would pass it.

[hchonov]

P.S. I am just trying to find an optimal solution and not simply fixing the test failures ASAP. It will be best for us if we find a good solution and fix this once and forever.

[aik099]

public function setValue($xpath, $value, $textfield_trigger = 'action');

That won't go though, because this driver method is part of DriverInterface, that all drivers share.

This means that setValue should be removing the focus for everything but autocomplete inputs.

Unfortunately the Selenium should (and maybe already does to some extent) should fire change event after contents of the field was changed (input or radio button). Before we're not doing anything to make it happen. The postValue method of used WebDriver library likely wasn't causing change event to fire or was firing it several times and that's why we're doing tab/backspace trick.

[pfrenssen]

The only currently known case where the blurring should not occur is for autocomplete fields. Therefore I think it would be best if we allow for autocomplete fields to turn off the blurring.

I am actually in agreement with this. Autocomplete fields are an edge case. But unfortunately we are breaking backwards compatibility. I don't know the policy of the maintainers regarding breaking backwards compatibility for edge cases like this? For me personally it would be fine, I already have my workaround. We have to be aware that there are probably other users that will face this once we release a new version.

I've looked into the implementation of jQuery Autocomplete and it has listeners only for the following events : keydown, keypress, input, focus and blur. Yes right, they don't have a dedicated event listener for the change event.

Thanks for the research. I will use jQuery Autocomplete for the test.

Could you please do some digging why Drupal implemented the test like this in the first place? I think this would be pretty useful information.

Good idea, this is important information, so I researched this. It turns out that this test was written in September 2016 (ref Issue #2788209: Entity reference field "Autocomplete matching: Starts with" option does not work). The solution with the change event was merged in October 2016 (ref. #244). So this test simply dates from a time when the change event was not being output.

public function setValue($xpath, $value, $textfield_trigger = 'action');

That won't go though, because this driver method is part of DriverInterface, that all drivers share.

Very good point. Any other suggestions? Would you accept local state to be set?

A possible solution would be that we provide an additional interface for drivers that use Selenium, just thinking out loud here. We could then check for this interface and use a local static parameter to track which method to use:

interface SeleniumDriverInterface extends DriverInterface
{
    public function setTextFieldCompletionMethod($method = 'blur');
}

[aik099]

Prior to #244 (you can see it on the diff) the change event was natively fired thanks to the TAB being added to the end of field value. Then some bug in Chrome 53 prevented this from working as expected resulting in double-tab being sent to browser. Maybe we should really revert code to tab sending state in hopes, that in current Chrome version the issue was fixed and no fake change event sending techniques are needed at all.

[pfrenssen]

I searched the Chromium bug tracker and found the issue about the tab key not working in Chrom[eIium] 53: https://bugs.chromium.org/p/chromedriver/issues/detail?id=1484

Apparently this was already fixed less than a week after it was initially reported, here is the commit fixing the issue: https://codereview.chromium.org/2302803004, from September 2, 2016. You can see in the testSendingTabKeyMovesToNextInputElement() test in that commit that they are literally verifying now that no symbols are left behind in the field when pressing the tab key.

So it looks like this was a very short lived bug which probably only affected Chrome 53.

Once we have the test on our end we can check if reverting #244 works now for both the cases of @hchonov and me.

[pfrenssen]

I have created a test: minkphp/driver-testsuite#22

I have tested it locally and these are the results:

• On the current master it fails, because the blurring of the field causes the autocomplete results to disappear.
• When I revert this PR it passes.
• When I revert Fixed unwanted trailing tab in chrome 53 #244 it fails, because tabbing out of the field also causes the autocomplete results to disappear.
• When I run the test against the latest v1.3.1 release of MinkSelenium2Driver the test also fails, because in this release the "tab" strategy was still used.

The last point is very interesting. In the current stable release the usage of DriverInterface::setValue() and TraversableElement::fillField() is not working for autocomplete fields. This impacts on whether we can consider this a regression or not. Regressions are usually measured against the latest stable release, not in the master branch.

BTW here is the patch for reverting #244: https://gist.github.com/pfrenssen/0bf4137fb3521334e671d61b3e0f3dff

[aik099]

When I revert #244 it fails, because tabbing out of the field also causes the autocomplete results to disappear.

What happens with change event is fired once test. Doest it pass?

The last point is very interesting. In the current stable release the usage of DriverInterface::setValue() and TraversableElement::fillField() is not working for autocomplete fields. This impacts on whether we can consider this a regression or not. Regressions are usually measured against the latest stable release, not in the master branch.

So what should we do. Revert code state to broken auto-complete?

[pfrenssen]

I couldn't find a test that contains the exact wording change event is fired once but here is the result from the ChangeEventTest:

$ WEB_FIXTURES_BROWSER=chrome WEB_FIXTURES_HOST=http://localhost:8000 ./vendor/bin/phpunit vendor/mink/driver-testsuite/tests/Js/ChangeEventTest.php
PHPUnit 7.2.7 by Sebastian Bergmann and contributors.

......E........                                                   15 / 15 (100%)

Time: 5.6 seconds, Memory: 4.00MB

There was 1 error:

1) Behat\Mink\Tests\Driver\Js\ChangeEventTest::testSetValueChangeEvent with data set "file" ('the-file', 'from empty', 'from existing')
Exception: invalid argument: File not found : from empty
  (Session info: chrome=65.0.3325.146)
  (Driver info: chromedriver=2.36.540471 (9c759b81a907e70363c6312294d30b6ccccc2752),platform=Linux 4.17.8-1-ARCH x86_64)

/home/pieter/v/MinkSelenium2Driver/vendor/instaclick/php-webdriver/lib/WebDriver/Exception.php:144
/home/pieter/v/MinkSelenium2Driver/vendor/instaclick/php-webdriver/lib/WebDriver/AbstractWebDriver.php:157
/home/pieter/v/MinkSelenium2Driver/vendor/instaclick/php-webdriver/lib/WebDriver/AbstractWebDriver.php:218
/home/pieter/v/MinkSelenium2Driver/vendor/instaclick/php-webdriver/lib/WebDriver/Container.php:224
/home/pieter/v/MinkSelenium2Driver/src/Selenium2Driver.php:670
/home/pieter/v/MinkSelenium2Driver/vendor/behat/mink/src/Element/NodeElement.php:105
/home/pieter/v/MinkSelenium2Driver/vendor/mink/driver-testsuite/tests/Js/ChangeEventTest.php:50

ERRORS!
Tests: 15, Assertions: 31, Errors: 1.

This 1 error is a known failure, it has been reported in #239.

I'm getting the same results for the latest master, when reverting this PR, and when running the test on v1.3.1.

[aik099]

Great. Then change event when filling the input is indeed fired only once.

[pfrenssen]

I have proposed a workaround for our project so we have a dedicated way of filling in autocomplete fields. We are using Behat for our tests. If other people are also experiencing this problem they can refer to this: ec-europa/joinup-dev#1301

[aik099]

I'm only now realizing why auto-complete tests fail. It's because leading focus of the field to cause change event (not monitored by auto-complete controls) also causes blur event to happen (monitored by auto-complete controls to close auto-suggestions div).

As mentioned in #292 current code, that fires change event in non-atomic way cause stale element reference in some cases.

[stof]

Well, the issue with that is different expectation for this code:

• when using autocomplete, people want to type in the field, without loosing focus, because they are not done doing a change
• when setting a value itself, you want the change event to be triggered for other parts to know you are done doing a change. And browsers are triggering the change event when the focus moves away from the text field.

IMO, the issue here is expecting that setValue can be used to test the autocomplete case: in this workflow, you are not setting the value at the time the autocomplete dropdown opens (as it is precisely here to help you set it).

[aik099]

Then we can't really support both cases automatically. Global flag during driver initialization would process all input fields as auto-completes or not so that's no good. Changing setValue method signature would break compatibility with DriverInterface. :(

[JordiGiros]

I've arrived here from #292 and after opening #301.
So I understand that there is no way to have both problems solved using only one method (setValue) because we break compatibility with DriverInterface...? Is there any link to an issue on Mink project for that? Or is it possible to scale this problem to mink? It's important in order we can run green our tests again all together...

[aik099]

I've arrived here from #292 and after opening #301.
So I understand that there is no way to have both problems solved using only one method (setValue) because we break compatibility with DriverInterface...?

@JordiGiros , you're correct.

Or is it possible to scale this problem to mink? It's important in order we can run green our tests again all together...

@JordiGiros , not possible I'm afraid. Changing the DriverInterface would break all existing 3rd-party drivers. Adding more parameters to setValue method to act only for certain form controls would make its signature a mess. Also Behat just calls setValue if I'm not mistaken and it can't know if an input is of auto-complete type and doesn't need blur or not.

[stof]

@aik099 we do support adding new methods in DriverInterface, as our policy explicitly states that we expect drivers to extend CoreDriver if they want to be covered by BC (and so we can add new method if we also add them in CoreDriver as UnsupportedDriverActionException)

[stof]

and for usage in Behat, it is still possible to write another Behat step which would use a new API (it would of course require to update the Behat scenario)

[aik099]

Ah, then with the help of new method we can cover auto-complete inputs. Then PR could be created to cover this.

[JordiGiros]

As I understund, it is as easy as adding into Mink's CoreDriver.php

    /**
     * {@inheritdoc}
     */
    public function setAutocompleteValue($xpath, $value)
    {
        throw new UnsupportedDriverActionException('Setting the field value is not supported by %s', $this);
    }

And DriverInterface.php

    /**
     * Sets autocomplete element's value by it's XPath query.
     *
     * @param string            $xpath
     * @param string|bool|array $value
     *
     * @throws UnsupportedDriverActionException When operation not supported by the driver
     * @throws DriverException                  When the operation cannot be done
     *
     * @see \Behat\Mink\Element\NodeElement::setAutocompleteValue
     */
    public function setAutocompleteValue($xpath, $value);

Then we can generate that method in MinkSelenium2Driver project.

Isn't it?

[aik099]

As I understund, it is as easy as adding into Mink's CoreDriver.php

    /**
     * {@inheritdoc}
     */
    public function setAutocompleteValue($xpath, $value)
    {
        throw new UnsupportedDriverActionException('Setting the field value is not supported by %s', $this);
    }

And DriverInterface.php

    /**
     * Sets autocomplete element's value by it's XPath query.
     *
     * @param string            $xpath
     * @param string|bool|array $value
     *
     * @throws UnsupportedDriverActionException When operation not supported by the driver
     * @throws DriverException                  When the operation cannot be done
     *
     * @see \Behat\Mink\Element\NodeElement::setAutocompleteValue
     */
    public function setAutocompleteValue($xpath, $value);

Then we can generate that method in MinkSelenium2Driver project.

Isn't it?

Something, like that. Yes.

[pfrenssen]

I support this solution, this would solve it for both expected use cases, then it is just a matter of using the right method. Thanks!