-
Notifications
You must be signed in to change notification settings - Fork 9
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
setValue() TAB causes unintended actions #5
Comments
Note: If the decision is for d, can I suggest that the blur be moved to a new function, i.e.
This will at least allow me extend the driver, and then to override the |
It should be possible to extend this driver to modify the behaviour as required without having to rewrite substantial parts of it. For example in our use we create a new WebDriver and WebDriverFactory which extend this project. We use this to add some minor functionality specific to our project (mostly transitional as we move away from the Selenium2 driver). We can also use this layer to fix bugs before they are accepted upstream. For example with issue oleg-andreyev#5 the current implementation of `setValue` includes triggering a change event by sending a Keyboard Tab character to move to the next character. Whilst I would argue that this is a behaviour which includes undesirable consequences the project may not take this view. I am then left with the decision of whether to work around this behaviour or to fork the Driver and use what I deem to be the more correct approach. If I wish to work around this behaviour in my extension of the driver I must copy and fix the `setValue()` function, as well as `findElement()`, `clickOnElement()`, `getDateTimeFormatForRemoteDriver()`, and `executeJsOnElement()`. Whilst you could argue that there should be no need to extend the WebDriver class I would counter that it is often helpful to be able to do so in certain situations.
Hi @oleg-andreyev, Anything I can do to progress this? I’m really keen to get moved over to php-webdriver/webdriver. |
@andrewnicols haven't looked into this issue yet, I'm in the progress of stabilizing build for the main branch. |
@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 |
It should be possible to extend this driver to modify the behaviour as required without having to rewrite substantial parts of it. For example in our use we create a new WebDriver and WebDriverFactory which extend this project. We use this to add some minor functionality specific to our project (mostly transitional as we move away from the Selenium2 driver). We can also use this layer to fix bugs before they are accepted upstream. For example with issue oleg-andreyev#5 the current implementation of `setValue` includes triggering a change event by sending a Keyboard Tab character to move to the next character. Whilst I would argue that this is a behaviour which includes undesirable consequences the project may not take this view. I am then left with the decision of whether to work around this behaviour or to fork the Driver and use what I deem to be the more correct approach. If I wish to work around this behaviour in my extension of the driver I must copy and fix the `setValue()` function, as well as `findElement()`, `clickOnElement()`, `getDateTimeFormatForRemoteDriver()`, and `executeJsOnElement()`. Whilst you could argue that there should be no need to extend the WebDriver class I would counter that it is often helpful to be able to do so in certain situations.
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? |
yes, it's the exact scenarios I was thinking |
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; orc) 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:
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.
The text was updated successfully, but these errors were encountered: