Skip to content
This repository has been archived by the owner on Dec 9, 2023. It is now read-only.

Remove syn #2

Closed
wants to merge 2 commits into from
Closed

Remove syn #2

wants to merge 2 commits into from

Conversation

uuf6429
Copy link
Owner

@uuf6429 uuf6429 commented Oct 1, 2023

No description provided.

@uuf6429
Copy link
Owner Author

uuf6429 commented Oct 1, 2023

@stof

Regarding the usage of syn.js, I'd rather keep the initial implementation free of it, marking methods as unsupported and skipping tests if needed.

My worry is that we end up with something that is unusable in the real world.

Note that we might have to deprecate the existing keypress, keyup and keydown methods as they don't reflect reality: an actual browser has no way to interact in a way that triggers only one of those events.

Why not? They do reflect the real usage, or?

The bigger problem for me is blur and focus, which actually never really happen individually (and which, by the way, are also removed when we remove syn).

@uuf6429
Copy link
Owner Author

uuf6429 commented Oct 1, 2023

The remaining 4 failures are from the lack of calling element->blur() after changing the value.

For real-world usage, changing the value of text fields doesn't immediately trigger "change" event - the user must somehow unfocus the field first (whereas the test seems to assume that it happens automatically).
From the description of setValue, it's not clear to me if the blur event should be triggered or not.

@stof
Copy link

stof commented Oct 1, 2023

@uuf6429 the various keyboard event never happen individually either.

Regarding focus and blur, the big issue with implementing them with syn is that they don't actually focus the element. syn will only simulate the event. It does not change the focused element.

@uuf6429
Copy link
Owner Author

uuf6429 commented Oct 1, 2023

@stof

@uuf6429 the various keyboard event never happen individually either.

I didn't mean how they are implemented in syn, but rather from a user/browser perspective.

A user should be able to press/hold/release a key, so I don't think it makes sense to remove that functionality (but rather implement it properly, without syn). It might make sense to rethink how that works though, for example releasing a key only makes sense when it has already been pressed.

Regarding focus and blur, the big issue with implementing them with syn is that they don't actually focus the element. syn will only simulate the event. It does not change the focused element.

That doesn't seem entirely true. There's a lot of code going on, but at some point it seems to actually call el.focus() and el.blur() (not sure about that one TBH).
But as I was trying to say before, I believe the driver shouldn't expose focus/blur functionality since it's not something browser users can actually directly do.

In conclusion I believe that:

  • as agreed, syn should be removed
  • keyXX functionality should stay in mink, but maybe reworked to be more realistic
  • blur/focus should be removed from mink

What are your thoughts about #2 (comment)? When we have a solution on that, I could finish this PR and merge it back.

@stof
Copy link

stof commented Oct 17, 2023

@uuf6429 Regarding the ability to press keys, a work has been started and stalled about adding a different API allowing to support it in a working way. See minkphp/Mink#772 and minkphp/Mink#831
The syn-based implementation has never been a working way to do that as it does not simulate the actual behavior of browsers (and so a passing test does not guarantee that a user can actually use the feature as they might not be able to trigger the corresponding interaction). There's a good reason why MinkSelenium2Driver did not manage to implement those methods using Selenium APIs and you did not manage either.

@uuf6429
Copy link
Owner Author

uuf6429 commented Oct 17, 2023

I feel that we are going around in circles.

@uuf6429 Regarding the ability to press keys, a work has been started and stalled about adding a different API allowing to support it in a working way. See minkphp/Mink#772 and minkphp/Mink#831 The syn-based implementation has never been a working way to do that as it does not simulate the actual behavior of browsers (and so a passing test does not guarantee that a user can actually use the feature as they might not be able to trigger the corresponding interaction). There's a good reason why MinkSelenium2Driver did not manage to implement those methods using Selenium APIs and you did not manage either.

In all the comments so far, I repeatedly said that syn is not a good solution. We all get that and there's no point in repeating it.

From my perspective, a browser user is still able to hold and release keys in different orders, at different times and with repeats - not just pressing them. But this is a different subject that is only relevant to mink as a whole, and not the drivers.

So for now we can shelve this discussion.


The last remaining problem in this PR is mentioned in #2 (comment) (which is about setting the field value, not key actions). I still don't have a reply or a solution for that.

  1. I feel the failing test is itself wrong. When you fill in a textfield, the blur event is not normally triggered.
    Unless it is expected for mink to behave a bit differently than normal browsers, in which case I'm a bit stuck with point (2):
  2. I cannot easily trigger a blur event without javascript (and without effectively repeating what syn did). Either way, we need to decide on point (1) first.

So the solutions I see here are:

  1. change the test to not depend on blur/change event, then this PR should start to pass as is
  2. check how syn (or better/other implementations did it) are triggering blur and only trigger it for this specific case (fill value) - without an entire external library

To be clear, the root cause is here: https://github.com/uuf6429/webdriver-classic-driver/pull/2/files#diff-eb40607e253c5d00bff80029635d63de06410cedcea1533338bd514b09873d8eL459

@uuf6429
Copy link
Owner Author

uuf6429 commented Dec 9, 2023

Moved to source repo (see minkphp#6)

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

Successfully merging this pull request may close these issues.

2 participants