remove firing redundant change event for setValue

[MikeShysh]

No description provided.

[asolntsev]

@MegaUkrainian There are some failing tests:

• integration.PageWithJQuery.setValueShoulNotTriggerOnChangeEvent
• integration.PageWithJQuery.selectByXpath
• integration.PageWithoutJQuery.setValueShoulNotTriggerOnChangeEvent

Can you fix it?

[BorisOsipov]

@asolntsev sorry, I forgot to mention, @MegaUkrainian have already fixed these tests. We disscused that in a private conversation. Thank you so much to @MegaUkrainian for his work!

But I am afraid that such changes will break end users test in a similar way as they broke some selenide tests.
aren't you?

[nick318]

Sorry, but I cannot understand why change event is redundant???
It really works and helps in many cases, because really often javascript events listen "onChange"

[MikeShysh]

The main point here is that user does not trigger change event explicitly, it is done by browser according to user actions. Sending change event after sendKeys is an overhead.
Exactly in my case (and to my mind it is actual for most systems):
We have 3 input fields on the form. We perform sending values to that fields. Change event that is triggered on the last field causes focus change to another element (following next in the DOM) and in my case this field is hidden due to window size and layout of the application crashes. Real user cant turn this case into reality. So in our case we have the situation when testing tool is the source of additional vulnerabilities. It's a bad practice. If explicit "change event" is mandatory step in your case, your can easily trigger it by js call. In all other cases change event would(should) be triggered by browser

[nick318]

Again, people already use and rely on existing change event!
If you still want to delete it, it is better to make option about sending change event or not.

[MikeShysh]

Explicitly triggering "change event" - it's a hack and non-native behaviour.

people already use and rely on existing change event!

Do you have any statistics on explicit usage of such event? Is it worth making optional such behaviour and overload the number of options in config?

[BorisOsipov]

Do you have any statistics on explicit usage of such event?
cause it is default behavior all users still use and rely on this. Maybe they don't even know this.

[MikeShysh]

cause it is default behavior all users still use and rely on this. Maybe they don't even know this.

If they even do not know, it means that it is not crucial for them. A browser will trigger this event by default.
Nevertheless, such changes will be at the documentation.

[asolntsev]

@MikeShysh @BorisOsipov As much as I see, some tests are still failing:

integration.PageWithJQuery.selectByXpath
integration.PageWithoutJQuery.setValueShouldNotTriggerOnChangeEvent

[coveralls]

Coverage decreased (-0.1%) to 64.345% when pulling fe1887e on MikeShysh:removeEventInSetValue into cab9680 on codeborne:master.

[codecov-io]

Codecov Report

Merging #596 into master will increase coverage by 1.47%.
The diff coverage is 69.51%.

@@             Coverage Diff              @@
##             master     #596      +/-   ##
============================================
+ Coverage     61.89%   63.36%   +1.47%     
- Complexity      828      861      +33     
============================================
  Files           154      154              
  Lines          2892     2981      +89     
  Branches        281      290       +9     
============================================
+ Hits           1790     1889      +99     
+ Misses          995      991       -4     
+ Partials        107      101       -6

Impacted Files	Coverage Δ	Complexity Δ
...ava/com/codeborne/selenide/ElementsCollection.java	0% <ø> (ø)	0 <0> (ø)	⬇️
.../com/codeborne/selenide/SelenideTargetLocator.java	0% <ø> (ø)	0 <0> (ø)	⬇️
...va/com/codeborne/selenide/ex/UIAssertionError.java	83.33% <ø> (ø)	9 <0> (ø)	⬇️
...e/selenide/impl/WebDriverThreadLocalContainer.java	80.14% <ø> (-2.95%)	29 <0> (-2)
...eborne/selenide/webdriver/ChromeDriverFactory.java	85.71% <ø> (-0.29%)	20 <0> (ø)
...odeborne/selenide/impl/SelenideFieldDecorator.java	83.33% <ø> (ø)	15 <0> (ø)	⬇️
...java/com/codeborne/selenide/junit/SoftAsserts.java	0% <ø> (ø)	0 <0> (ø)	⬇️
...rne/selenide/webdriver/WebDriverBinaryManager.java	33.33% <ø> (ø)	5 <0> (ø)	⬇️
...orne/selenide/webdriver/AbstractDriverFactory.java	70.45% <ø> (ø)	11 <0> (ø)	⬇️
...codeborne/selenide/webdriver/WebDriverFactory.java	83.58% <ø> (ø)	14 <0> (ø)	⬇️
... and 39 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81f64ec...0806995. Read the comment docs.

[asolntsev]

@MikeShysh @BorisOsipov I agree with you, $.sendKeys() does not send change event.
But method $.val() have been triggering. If we remove it now, it's possible that many existing tests will fail. Yes, probably these are bad tests, but we don't want to break them with the next Selenide release.

Probably we should add some flag for keeping the previous behavior (let name it deprecated if you want)?

P.S. I tried to run tests in my current projects - some of them failed. And yes, these are bad tests. :)
These tests expected that $.val() triggers change event.

[rosolko]

@asolntsev, @MikeShysh, @BorisOsipov I propose to overload setValue with extra bool parameter that will configure method behavior with "change event" triggering.
So usage will be:
$().setValue("my value") - default method will trigger as now. Nothing changes.
$().setValue("my value", true/false) - new command, where you control behavior, true - trigger, false - ignore trigger.
So we will be backward compatible and add this possibility.

[MikeShysh]

Hi,

@rosolko thank you for your interest in this PR. However, to my mind, your solution a bit excessive from usability point of view. I see special flag to disable triggering change event is more user friendly

[BorisOsipov]

@rosolko

I agree with @MikeShysh, we discussed it some time ago and decided to merge it with special flag to disable triggering.

P.S.

I propose to overload setValue with extra bool parameter

From my point of view it is bad practices to add bool parameters to method :) In most cases it indicates that something wrong with your design\api

[rosolko]

@MikeShysh @BorisOsipov
Btw base on other user feedback we can't just remove this event because it's immediately broke some tests.
Can you please rework your PR with special flag to disable triggering?

[MikeShysh]

@rosolko

Can you please rework your PR with special flag to disable triggering?

yeah, I will do it, when I have time =)

[BorisOsipov]

@MikeShysh would you mind to update your PR according requested changes?

[MikeShysh]

@BorisOsipov Yeah, I will try to find time for this weekend. If you can to do it now, feel free to update PR

[rosolko]

Closed in favor of #718