Remove jQuery dependency

The test was using jQuery, let's see what the testbot makes of this.

Ah. Pure JS .submit() doesn't return true.

+++ b/tests/src/FunctionalJavascript/AntibotJavaScriptTest.php
@@ -37,7 +37,7 @@ class AntibotJavaScriptTest extends WebDriverTestBase {
-jQuery('form[data-drupal-selector="user-pass"]').submit();
+document.getElementById('user-pass').submit()

This change is not 1-to-1 I wouldn't depend on id attribute for anything these days, and most of core now depends on data-drupal-selector.

Maybe try with document.querySelectorAll('form[data-drupal-selector="user-pass"]'); ?

Fair enough, regardless the test won't work using a plain JS .submit() because it won't return true in assertJsCondition().

The only way I can see to get that test working is to enable jQuery for the tests but I feel like that's the wrong thing to do. I'm not very experienced with the test suite.

A made a little change in the tests to remove the JQuery dependency.
Here is the interdiff between patch #3 and the merge request.

Reviewing this one

Unassigning from myself

Checked all tests, could not find any jQuery dependency related to it. Hence moving to RTBC.

Was going through the code changes by checking out to the branch from which MR has been made and I noticed there is a deprecation warning being shown for the 'keyCode'. Maybe we should figure out the alternative for that first and then move forward with this.

Thanks for finding that deprecation!

Standard procedure is to handle all deprecations and code standard changes not directly introduced by an issue in a separate issue.

Please open another issue if you haven't already to deal with the deprecation, it should not be dealt with in this issue, and there is no reason to postpone this work because of it.

And, thank you @gabrielda for the review! Welcome to the Drupal community :)

@guarav.kapoor apologies, I see you're a maintainer in which case maintainer's privilege overrides :)

I'll open a new ticket to handle that piece.

Ahhhhahahahah hello hubris my old friend... I really should have looked deeper before commenting. My apologies a second time.

In core this would probably still be split out into two separate issues but it's such a small change I see no reason not to add it here. Pushed that change to the MR.

@darvanen Relax!! and thank you for letting me know deprecations are handled in separate issues, I have seen issues specific to handle deprecations but I wasn't really sure about that. Thank you so much for working on this, this looks good to me, I'll be reviewing and pushing it today.

All good!

Life's too short not to be able to laugh at myself ;)

There should be a feature of displaying a 'maintainer badge' in the issue queue comments.

I noticed that, this change wasn't working:

document.body.addEventListener('keypress', function (e) {
if ((e.code == 9) || (e.code == 13)) {

as e.code returns string values such as 'Enter' or 'Tab' and not the code.

I have done this change:

-      document.body.addEventListener('keypress', function (e) {
-        if ((e.code == 9) || (e.code == 13)) {
+      document.body.addEventListener('keydown', function (e) {
+        if ((e.code == 'Tab') || (e.code == 'Enter')) {

So, that antibot script can detect if the user has pressed enter or tab key. Also, changed the event from 'keypress' to 'keydown' as 'keypress' wasn't working with the tab key.

Let me know if this isn't the right solution.

Ahhh yep, looks like keypress is deprecated too /facepalm. The change looks good to me in the comment.

I guess we should try and put in some test coverage around that funcationality. I couldn't get the FunctionalJavascript tests running on my local machine though.

Turns out there is already @todo by previous maintainers to add test coverage for this scenario. Will open up a new issue for that.

Tested this in my local, the module is working perfectly fine, didn't see any break in the functionality or any JS errors.

Thanks, everyone for working on this