pkp / pkp-lib

The library used by PKP's applications OJS, OMP and OPS, open source software for scholarly publishing.
https://pkp.sfu.ca
GNU General Public License v3.0
307 stars 447 forks source link

After Random characters in autosuggest field for categories, you'll get directly the XHR response #9884

Closed forgive38 closed 3 weeks ago

forgive38 commented 7 months ago

Describe the bug During th submission process, when you have more than 10 categories, a autosuggest field is used. And if you enter some random characters in and hit Enter, you'll get the result of XHR request.

To Reproduce

  1. Go to testdrive 3.4
  2. enable categories, add more than 10 categories
  3. make a new submission and in For the Editors tab, try it
  4. See error

What application are you using? OJS 3.4 testdrive

jardakotesovec commented 4 months ago

This was interesting to track down. Apparently there is some default browser behaviour specifically on form with one field. So hitting enter in that one field was automatically submitting the form, which was causing to display the endpoint where the form was pointing to.

But additionally if there is form with one input and some button - it will hit the button automatically?! So additional to the report, what was happening is that if you would enter valid category and than you would type something that does not exist and hit enter - it clicked on the 'remove' button of the previous category just based on that there is SOME button in the form.. Can't be more confusing than that.

What seems to be effective way to prevent it is just discard such enter event on the form level. I did test it with chrome, safari and firefox, so hopefully its good fix.

@jonasraoni Can you please do the code review?

Will check whether same problem is on 3.3 and main branch.

3.4.0 PR https://github.com/pkp/ui-library/pull/381

jardakotesovec commented 4 months ago

3.3 is using legacy form with checkboxes, so not relevant in that particular use case, but it can be problem in other vue.js form which would have just one FieldText for example

Therefore adding same improvement for 3.3 https://github.com/pkp/ui-library/pull/383

And same thing for main - its not problem in this particular scenario, because the autocomplete component from headless-ui does not let propagate enter event. But its misbehaving for example with simple FieldText.

After the code review I will also cherry pick it to the main branch.

jonasraoni commented 4 months ago

Yeah, this behavior is old. I agree with the fix 😁

The worst was the backspace behaving as a back button 🥲

jardakotesovec commented 4 months ago

Merged to 3.3 and 3.4 and cherry-picked to main. Thanks @jonasraoni

bozana commented 1 month ago

After this change it is not possible to enter a new line in the field References.

Removing @keydown.enter.prevent="" apparently fixes it.

Note:

... even the original issue why I added this was sneaky behavior when there is just one field in the form... But that hitting enter in textarea will get suppressed if I am ignoring this keydown on form element - I don't see how someone could anticipate such behavior...

... to solve this in the context of this original issue report... making sure that both scenarios works...

jardakotesovec commented 1 month ago

So this is alternative I came up with. Reasoning behind this is that we don't rely on submit event at all. We use normal button to submit form, which triggers our custom vue.js event. Therefore just preventing default behaviour on submit should be safe and it prevents the scenario, when there is one input and no button in the form to automatically triggers submit event and therefore open the 'action' url.

But thats not enough. As mentioned above - if there is one input and some button - it will automatically click on that button - no matter what that button does. So for example in categories autocomplete it was removing the categories on entry, because the remove icons are buttons.

Therefore also added <input type="submit" value="i9884" style="display: none" />. I can't think of any side effect that this might cause and now its the button which gets automatically clicked on when there is just one input. Which triggets the submit event on form and that gets ignored with the @submit.prevent="() => {}".

And this new solution is not interfering with hitting enter - which cause that issue in textarea that @bozana mentioned.

I did various testing across browsers and versions. So hopefully this is solid solution.

I will need two approvals for 3.3 and 3.4, can I ask @blesildaramirez and @ewhanson ?

main: ui-library: https://github.com/pkp/ui-library/pull/432 ojs (tests): https://github.com/pkp/ojs/pull/4485 https://github.com/pkp/ojs/pull/4491

3.4: ui-library: https://github.com/pkp/ui-library/pull/433 ojs (tests): https://github.com/pkp/ojs/pull/4486 https://github.com/pkp/ojs/pull/4492

3.3 ui-library: https://github.com/pkp/ui-library/pull/434 ojs (tests): https://github.com/pkp/ojs/pull/4487 https://github.com/pkp/ojs/pull/4493

Note from Blessie: OJS test links have been changed due to correction on commit message, to point to correct branch names in ui-library. The ui-library branches are from Jarda's original branch, just making sure the tests in OJS points to intended changes in ui-library for accurate testing runs.

jardakotesovec commented 1 month ago

Test for main are failing.. let me check whether its all rebased

jardakotesovec commented 1 month ago

All tests are passing now.

asmecher commented 3 weeks ago

All merged (except submodules which will be handled separately), thanks!