WordPress.org

Make WordPress Core

Opened 23 months ago

Last modified 3 months ago

#41545 assigned enhancement

Allow cmd/ctrl-enter to submit comment forms

Reported by: helen Owned by: adamsilverstein
Milestone: 5.3 Priority: normal
Severity: minor Version:
Component: Comments Keywords: good-first-bug needs-testing has-patch needs-refresh
Focuses: javascript Cc:

Description

It's become fairly common for comment/reply forms on the web to be submitted with cmd/ctrl-enter. Some quick examples include Twitter and GitHub, neither of which tells you as far as I've seen but rather just lets you discover it works. I think it'd be a nice enhancement for comment forms in both the admin and front end to submit this way. In particular, it would make managing and responding to comments a little smoother.

Attachments (4)

41545.diff (59.2 KB) - added by Lindstromer 23 months ago.
Added support for Ctrl/Cmd+Enter to submit replies in wp-admin and frontend. + code style for the specified js-files.
41545.2.diff (2.6 KB) - added by Lindstromer 23 months ago.
PHPStorm bugged with code style messing up files in previous diff. Also added html-file that was missed.
41545.3.diff (2.6 KB) - added by xyfi 10 months ago.
41545.4.diff (768 bytes) - added by adamsilverstein 4 months ago.

Download all attachments as: .zip

Change History (28)

#1 @Lindstromer
23 months ago

I'll try and submit a patch for it tomorrow, the wp-admin part is solved but front end needs a little more work.

@Lindstromer
23 months ago

Added support for Ctrl/Cmd+Enter to submit replies in wp-admin and frontend. + code style for the specified js-files.

#2 @Lindstromer
23 months ago

  • Keywords needs-testing has-patch added; needs-patch removed

The patch is functioning in Firefox, Safari and Chrome on OS X when using Ctrl or Cmd + Enter. Needs testing on different operating systems to see that it picks upp the key kombination as expected.

The solution isn't to clean for the frontend part, where I had to workaround the fact that submit() is overwritten by the submit-button id.

An alternative would be to refactor the whole form output and give it a better id but I'm guessing that it could create issues.

@Lindstromer
23 months ago

PHPStorm bugged with code style messing up files in previous diff. Also added html-file that was missed.

#3 @DrewAPicture
16 months ago

  • Owner set to lindstromer
  • Status changed from new to assigned

Assigning to mark the good-first-bug as "claimed".

#4 @adamsilverstein
12 months ago

  • Owner changed from lindstromer to adamsilverstein

#5 @adamsilverstein
12 months ago

  • Milestone changed from Awaiting Review to 4.9.7

#6 @desrosj
12 months ago

  • Keywords needs-refresh added

Patch needs a refresh to work with the new JavaScript file locations/build process.

#7 @ocean90
12 months ago

  • Milestone changed from 4.9.7 to 4.9.8

4.9.7 has been released, moving to next milestone.

This ticket was mentioned in Slack in #core by jon_bossenger. View the logs.


11 months ago

This ticket was mentioned in Slack in #core by joshuawold. View the logs.


11 months ago

#10 @pento
11 months ago

@adamsilverstein: Are you likely to be able to review this in time for 4.9.8 beta 1?

#11 @adamsilverstein
11 months ago

@pento apologies i've been travelling and just saw your note. I will look at this today.

#12 @adamsilverstein
11 months ago

  • Milestone changed from 4.9.8 to 4.9.9

I took a look at this yesterday and I think it needs a little more work to be ready:

  • Let's attach the event listener in JavaScript instead of in the HTML of the element itself. This makes it easier to tell where the event is getting called from and is the common pattern we use across most of core.
  • Switch to the keyup event instead of keydown which I think is better from a usability perspective?
  • Can we find/submit the original form instead of the current approach of creating a form?

Punting to 4.9.9 while we finish up these details.

@xyfi
10 months ago

#13 @xyfi
10 months ago

I have brought the patch created by @Lindstromer up to date with the current core code. I have not yet come around to implementing @adamsilverstein 's requested changes.

#14 @pento
8 months ago

  • Milestone changed from 4.9.9 to 5.0.1

#15 @pento
6 months ago

  • Milestone changed from 5.0.1 to 5.0.2

#16 @pento
6 months ago

  • Milestone changed from 5.0.2 to 5.0.3

#17 @audrasjb
6 months ago

  • Milestone changed from 5.0.3 to 5.1

Hello,

5.0.3 is going to be released in a couple of weeks.

It doesn't appear this ticket can be handled in the next couple of weeks (needs some work and review). Let's address it in 5.1 which is coming in February. Feel free to ask for changing the milestone if you think this issue can be quickly resolved.

Cheers,

Jb

#18 @pento
5 months ago

  • Milestone changed from 5.1 to 5.2

Patch needs refreshing.

#19 @adamsilverstein
4 months ago

In 41545.4.diff:

  • Refactor to add the event listener to comment-reply init, so its set on a per form basis.
  • Inline comment explaining why we use a click() action on the button vs. calling submit() on the form

#20 follow-up: @adamsilverstein
4 months ago

@xyfi @Lindstromer can you give 41545.4.diff a test?

#21 in reply to: ↑ 20 @Lindstromer
4 months ago

Replying to adamsilverstein:

@xyfi @Lindstromer can you give 41545.4.diff a test?

Thanks for the ping. Give me a day or two and I'll look into it.

#22 @adamsilverstein
4 months ago

Thanks @Lindstromer! Note if you haven't tested a patch recently: you now need to build the javascript (try npm install && grunt build) and run WordPress from the build folder (possible this requirement may have changed) - see https://make.wordpress.org/core/2018/05/16/preparing-wordpress-for-a-javascript-future-part-1-build-step-and-folder-reorganization/ for the original post explaining the change.

#23 follow-up: @desrosj
3 months ago

  • Milestone changed from 5.2 to 5.3

@Lindstromer have you been able to test 41545.4.diff?

I am going to punt this to 5.3 since this still needs feedback. @adamsilverstein if you feel confident in this for 5.2, feel free to move it back.

#24 in reply to: ↑ 23 @Lindstromer
3 months ago

Replying to desrosj:

@Lindstromer have you been able to test 41545.4.diff?

I am going to punt this to 5.3 since this still needs feedback. @adamsilverstein if you feel confident in this for 5.2, feel free to move it back.

Hi @desrosj , I'm sorry I haven't had time to work with this at all. I'll give it a try soon.

Note: See TracTickets for help on using tickets.