Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove syn (closes #5) #6

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Remove syn (closes #5) #6

wants to merge 3 commits into from

Conversation

uuf6429
Copy link
Member

@uuf6429 uuf6429 commented Dec 9, 2023

Fixes #5.

@@ -455,8 +455,6 @@ public function setValue(
} catch (\Throwable $e) {
throw new DriverException("Cannot set $widgetType value: {$e->getMessage()}", 0, $e);
}

$this->trigger($xpath, 'blur');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the testBlur failure, there are 4 other failures caused by removing this line.

I already described the problem here. I could bring back $this->blur($xpath) here, but I believe this behaviour (and the failing tests) are wrong. Please let's decide on this since it's unclear for me how Mink should behave in this case. @stof

Copy link
Member

@aik099 aik099 Mar 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is what I remember (it might not be too detailed, but you can look it up via Git Blame in Selenium2Driver) behind the blur event firing from the setValue call:

  1. we're sending the TAB key after a typed text to cause the native change event to be fired (if the value was actually changed)
  2. then there was a bug in Chrome 53 that caused TAB to be displayed in the input
  3. the above problem was fixed by firing the change event manually
  4. unconditional change event firing caused another problem, that in some cases the change event was fired twice (the native event and our fake event)
  5. to fix that we've replaced the change event firing with the blur event firing that in turn, if the element is still in focus, would cause the change event to be fired
  6. this made testing of the auto-complete controls impossible because the blur event caused auto-completed suggestions to disappear
  7. I don't know what happened with the auto-complete control testing, but no changes in the Mink Driver were made (probably some PR's are lying around asking to add a method, that does only sendKeys WebDriver call without any artifacts to Mink)

Current Selenium2 Driver code for the blur firing

// Remove the focus from the element if the field still has focus in
// order to trigger the change event. By doing this instead of simply
// triggering the change event for the given xpath we ensure that the
// change event will not be triggered twice for the same element if it
// has lost focus in the meanwhile. If the element has lost focus
// already then there is nothing to do as this will already have caused
// the triggering of the change event for that element.
$script = <<<JS
var node = {{ELEMENT}};
if (document.activeElement === node) {
  document.activeElement.blur();
}
JS;

// Cover case, when an element was removed from DOM after its value was
// changed (e.g. by a JavaScript of a SPA) and therefore can't be focused.
try {
    $this->executeJsOnElement($element, $script);
} catch (StaleElementReference $e) {
    // Do nothing because an element was already removed and therefore
    // blurring is not needed.
}

I'm not seeing that code in this driver anymore? Was it removed on purpose?

Copy link
Member Author

@uuf6429 uuf6429 Mar 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've read your points several times and somehow it feels more convincing not to have the old/selenium2driver behaviour.

  1. Sending tab automatically is unexpected/strange behaviour, which is bound to cause problems like point 2
  2. Points 3 to 5 sounds like a hack and further fighting with unexpected and non-standard behaviour
  3. Point 6 ("triggering blur causes problems"), seems to contradict the comment referring to it, where you seem to say that point 6 "is a good thing". Otherwise I don't get what you meant there.
  4. Regarding point 7, it is in my opinion the responsibility of the user or library vendor. Not autocomplete components behave in the same way, and it would be way too much work for us to maintain that for them.

I'm not seeing that code in this driver anymore? Was it removed on purpose?

We kept the old logic of triggering blur (before removing syn), but not checking if the element has been removed - so maybe we might be missing a test for that.
But anyway, I don't know why this logic is significant/required. I don't expect "change" or "blur" events after changing some types of fields - I don't see why the driver/mink needs to do that.

If the driver user specifically needs that, they can just call the blur() after setValue() themselves. Deciding for them means that we will unconditionally affect everyone else, including those people that don't want this non-standard behaviour.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've read your points several times and somehow it feels more convincing not to have the old/selenium2driver behaviour.

All these 7 points above are just a history to bring you up to speed with Selenium2 driver development.

As a Mink maintainer, my main concern is to make library users happier and ensure, that the library is evolving in the right direction. Ensuring that all drivers behave the same way, even if the test suite doesn't report any inconsistencies.

If the driver user specifically needs that, they can just call the blur() after setValue() themselves.

If we use raw Mink, then we probably can. I know, that the change/blur event firing thing isn't an expected behavior of the driver, but it's a good thing to have.

Maybe tab/blur/change code integrated into the setValue method is a leftover from times, when Mink was used only inside a Behat, where scenarios were saying to change field value and not to change field value and fire a change event.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting the value without a change event being fired also breaks all cases where you have JS code running on the change event. That's why our testsuite ensures that setting the value actually reports it to JS that the value has changed.

There was a discussion a few years ago (that has never been implemented) about introducing a new Mink API to write in a field (which would not remove the existing text first and would keep the focus in the field) to allow testing such autocomplete-based UI.

Copy link
Member

@aik099 aik099 Mar 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting the value without a change event being fired also breaks all cases where you have JS code running on the change event. That's why our testsuite ensures that setting the value actually reports it to JS that the value has changed.

That's the \Behat\Mink\Tests\Driver\Js\ChangeEventTest::testSetValueChangeEvent test, which confirms, that the change event is being fired once after the setValue call.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's continue the discussion here: #21 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace syn.js
3 participants