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

Why the clearing approach of "setValue" method was changed compared to the Selenium2 driver? #21

Open
aik099 opened this issue Mar 8, 2024 · 7 comments

Comments

@aik099
Copy link
Member

aik099 commented Mar 8, 2024

The Selenium2 driver was typing Backspace (for Windows) + Delete (for Mac) keys to the input/textarea in the amount needed to erase the current value. That allowed any JavaScript code of that page, that monitored input/textarea value change (e.g., to prevent certain keys from being typed) to behave as if a real human was doing this.

This driver uses the clear method of the WebDriver to clear the input/textarea value.

Does it fire necessary JS keyboard events in the process as well in every Selenium+Browser combination being tested?

P.S.

  • We're likely not having tests to verify that behavior.
  • The clear WebDriver method might be changing element focus, which might fire a duplicate focus/blur event when using clear+sendKeys together.
@uuf6429
Copy link
Member

uuf6429 commented Mar 8, 2024

Why the clearing approach of "setValue" method was changed compared to the Selenium2 driver?

The likely reason is that setValue doesn't say anything about simulating human behaviour (see also the DriverInterface), which is a case similar to focus()/blur().

Some things are clearly related to human behaviour (keyUp, keyDown etc), but others not (get/setCookie, get/setValue etc).

Does it fire necessary JS keyboard events in the process as well in every Selenium+Browser combination being tested?

Probably not.

@oleg-andreyev
Copy link

Does it fire necessary JS keyboard events in the process as well in every Selenium+Browser combination being tested?

Browser itself triggers all necessary events

@aik099
Copy link
Member Author

aik099 commented Mar 11, 2024

@oleg-andreyev , I've confirmed, that using sendKeys and clear methods doesn't fire the change event.

@uuf6429 ,

  • I'm not sure if we have a test, that checks proper key* events being fired during the setValue method use.
  • I've found the \Behat\Mink\Tests\Driver\Js\ChangeEventTest::testSetValueChangeEvent test, that confirms, that the setValue method will fire a change event once. It does fail, when the $this->trigger($xpath, 'blur'); line at the end of the setValue method is removed.

@oleg-andreyev
Copy link

oleg-andreyev commented Mar 12, 2024

@aik099 in order to fire change event, element should lose focus/blur this will happen as soon as you add TAB or switch to different element.

https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/change_event


The change event is fired for <input>, <select>, and <textarea> elements when the user modifies the element's value. Unlike the input event, the change event is not necessarily fired for each alteration to an element's value.
When the element loses focus after its value was changed: for elements where the user's interaction is typing rather than selection, such as a <textarea> or the text, search, url, tel, email, or password types of the <input> element.

I personally completely against emulating/triggering events on PHP side, they should happen on browser side as "real user" .

@uuf6429
Copy link
Member

uuf6429 commented Mar 12, 2024

@aik099

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.

  • As a library user, I don't want the library to do something that I don't expect it to. Setting the value of a field is not supposed to trigger any event, because that's how DriverInterface defined it and how browsers actually handle it. If I want to simulate user behaviour, I know that I should call focus()-sendKeys()-blur().
  • As a library maintainer, I don't want to write code that is difficult to maintain AND that caters only for some of our users (while completely blocks the others, which as I understand, relates to your Point 6).

In the end, I don't understand what is the objective of this API since the way Selenium2Driver implements it:

  1. does not behave like a user/browser
  2. and does not behave like a low-level API

To me it's a bad mixture of both.

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.

That's not a good justification for bad behaviour. Like I said many times in the last several months, I believe that test is wrong. I'd like to see good reasons why it should work like that.

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.

I still don't get why it is "a good thing" when all your 7 points (including point 6) point to it being bad (=> it causes unexpected behaviour, it caused various bugs, it limits functionality).

@stof

Setting the value without a change event being fired also breaks all cases where you have JS code running on the change event.

If those cases are only triggering setValue, then they're already broken.

From my perspective:

  1. sendKeys or keyXX API should trigger key events (as is already the case now)
  2. setValue should be a low-level API for just setting the value (similar to how getValue is not something that a human would do)
  3. MinkContext (or whoever is using mink) should be calling the focus()/setValue()/blur()* flow - like a human.
    * blur() would then trigger a change event natively from browser side.
  4. for autocomplete or similar scenarios, end-users should be able to selectively call any of those methods in the order they see fit (e.g. not calling blur() in order to see the autocompletion list).

With that being said, we should focus on what the facts are and how to improve/fix the situation.
To be honest, it really feels that setValue is the correct name for the functionality we want, but has bad behaviour (that is not even correctly documented in the interface).

  • If we deprecate setValue, we will need to figure out a new name.
  • If we change the behaviour (to what this driver is doing), we'd BC-break release version of Mink (and figure out how to handle this properly in tests)

@oleg-andreyev
Copy link

@uuf6429 I see it this way: webdriver is low-level client when using it you need to handle everything by yourself (tab, focus, blur and etc), mink drivers are high-level which reduces needs of doing something manual and provides Mink-interface - so if tests (common across all drivers) expects that setValue should trigger change event, then this repository should be compliant with tests.

I my initial version https://github.com/oleg-andreyev/MinkPhpWebDriver/blob/aad9b68443e8c93d4a7abcbada4bb1dd2c53f433/src/WebDriver.php#L745

I actually trigger change event.

It's not wrong or right per MDN.

@uuf6429
Copy link
Member

uuf6429 commented Mar 24, 2024

@oleg-andreyev

webdriver is low-level client when using it you need to handle everything by yourself (tab, focus, blur and etc)

There's no rule to expose the low-level driver. Behat users that must do something that is not available in mink (or that mink messed up) will have to rely on hacks such as exposing/accessing private properties.
Which, on the other hand, means that whatever mink does, it has to do it the right way as much as possible.

mink drivers are high-level which reduces needs of doing something manual

That's how I see it too, in theory. In practice, it's actually doing both (setCookie, for example is low level). Unfortunately, there doesn't seem to be any documentation or indication of which parts are low-level and which parts are high-level.

so if tests (common across all drivers) expects that setValue should trigger change event, then this repository should be compliant with tests.

There's only one driver - Selenium2Driver - many driver factories are instantiating that or a javascript-less driver (which means events are skipped). Your driver is based on that one, so it stands to reason that it might have inherited bad behaviour.

I actually trigger change event.

That's not saying anything - you just made it pass the test.

It's not wrong or right per MDN.

MDN is not saying how browsers or humans should behave, it says how browsers are behaving. You can write javascript that sets a field's value without triggering any event, or triggering specific or custom events - it's probably a bad idea, but it's not wrong in itself.


The point is, why are we triggering the change event?

  • The native flow triggers way more events - some of which are much more important (Vue, for example, depends on the input event, not the change event, for input/textarea). In that case, @aik099 is right about keyXX events (and I'd argue other events as well).
  • If we just want to set the value of a field then I don't see why we should trigger change (and not blur or input).

Feel free to play around with this: https://jsfiddle.net/ka074hmf/1/
Here are some observations (on chrome):

  • change is almost always triggered before blur, but only when the field has really been changed
    • for example, typing something and then removing, (e.g. undo) doesn't trigger change
    • in case of color, change is triggered after blur
  • select/checkbox triggers input and change immediately without blur, but can still be blurred later
  • all fields trigger input as soon as possible, e.g. text/textarea on each keystroke and color on each click in the color preview

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

No branches or pull requests

3 participants