[ticket/14422] Support cmd+enter & ctrl+enter for submitting message

[marc1706]

Previous PR: #4134

Ticket: https://tracker.phpbb.com/browse/PHPBB3-14422

[callumacrae]

😂 sorry!

[callumacrae]

lgtm 👍

[callumacrae]

[marc1706]

Great, this seems to be broken to me. I just get my post content removed and the page refreshed ...
Instead of

$(this).closest('form').submit();

this seems to work

$(this).closest('form').find('input[name=post]').click();

@callumacrae what do you think?

[lavigor]

@marc1706 I confirm your situation in full reply.

Because of this PR I was forced to remove Ctrl+Enter feature from QuickReply extension that worked correctly for years.

However the added code in this PR submits the form regardless of the submit button name that is used by the server side.

And what if an extension author decides to use textarea for some other purposes without the intension to use Ctrl+Enter?
And that is not related to extensions only. Just download an attachment, click on the 'File comment' field and press Ctrl+Enter there... The same. 😉

Sorry for the next statement, but why was the problem found only after the core release?
That is the code of the very first commit of QuickReply that does the same without errors (maybe not properly supporting OS X, but I don't remember anybody complaining about that).

[marc1706]

The initial fix here is definitely incomplete and broken. It was working fine when it was merged but as you mentioned, the side effects were not fully considered.

Why this was only found after the core release? People have been using 3.1 in their dev environments for a while after this was merged. There was also a RC release. This could have been improved during review and after while testing the pre-release. Nonetheless, some minor issue can always be overseen as happened in this case.
That's why we try to use more and more functional tests. Adding them is not a trivial task and there are also other side effects to adding more and more functional tests.

Moving on from this, there should just be a new pull request that fixes it. I don't think that functional tests are worth it for this specific case (extra work needed vs. possible regressions it might fix in the future --> lot of work vs. close to zero / none).

[lavigor]

@marc1706 thank you very much for the explanation.

[callumacrae]

@marc1706 why does that fix work? I haven't tested it, but that sounds like our markup is invalid to me

[marc1706]

Yes, it might be possible that our markup is not correctly structured. I don't think we should have this side effect for every text area though?

[callumacrae]

This will only happen on textareas with submittable parent forms. If the markup is correct, this should work everywhere