Bug 10310 - Prevent submit form pressing enter should be improved
Summary: Prevent submit form pressing enter should be improved
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 9507
Blocks:
  Show dependency tree / graph
 
Reported: 2013-05-22 13:48 UTC by Jonathan Druart
Modified: 2014-12-07 20:02 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:

Attachments
Bug 10310: Prevent submitting form with enter does not work with IE (1.60 KB, patch)
2013-05-22 13:51 UTC, Jonathan Druart 		Details | Diff | Splinter Review
Bug 10310: Prevent submitting form with enter does not work with IE (1.73 KB, patch)
2013-05-22 14:01 UTC, Marcel de Rooy 		Details | Diff | Splinter Review
Bug 10310: Prevent submitting form with enter does not work with IE (1.65 KB, patch)
2013-05-22 14:40 UTC, Jonathan Druart 		Details | Diff | Splinter Review
Bug 10310: Prevent submitting form with enter does not work with IE (1.75 KB, patch)
2013-05-23 06:59 UTC, Marcel de Rooy 		Details | Diff | Splinter Review
Bug 10310: Followup Prevent submitting form with enter does not work with IE (980 bytes, patch)
2013-05-23 07:40 UTC, Jonathan Druart 		Details | Diff | Splinter Review
Bug 10310: Prevent submitting form with enter does not work with IE (1.75 KB, patch)
2013-05-23 08:17 UTC, Marcel de Rooy 		Details | Diff | Splinter Review
Bug 10310: Followup Prevent submitting form with enter does not work with IE (1.05 KB, patch)
2013-05-23 08:17 UTC, Marcel de Rooy 		Details | Diff | Splinter Review
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2013-05-22 13:48:39 UTC 
Actually it does not work for IE (see Marcel's comment on bug 9507).
Comment 1 Jonathan Druart 2013-05-22 13:51:31 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2013-05-22 14:01:07 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2013-05-22 14:12:24 UTC 
QA Comment:
Very quick! fix from Jonathan as followup for 9507.
Tested neworderempty on FF and IE.
Grepped prevent_submit and checked templates for doc3 tag.
Notes textarea still worked with Enter.

Good improvement and reduction of code. Seems to be browser independent.
Two document ready functions are not a problem for jQuery too.

Passed QA
Comment 4 Owen Leonard 2013-05-22 14:16:00 UTC 
This seems to work in IE9, but I think it has an unintended consequence: You can't tab to the submit button and hit <enter> to submit.
Comment 5 Marcel de Rooy 2013-05-22 14:17:46 UTC 
(In reply to comment #4)
> This seems to work in IE9, but I think it has an unintended consequence: You
> can't tab to the submit button and hit <enter> to submit.

So true. This just went too fast :)
Comment 6 Jonathan Druart 2013-05-22 14:40:02 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2013-05-22 14:42:29 UTC 
(In reply to comment #4)
> This seems to work in IE9, but I think it has an unintended consequence: You
> can't tab to the submit button and hit <enter> to submit.

Yep good catch Owen!

The last patch returns false only if enter is pressed on input with a type != "submit". I don't find a case where a problem can occur.
Comment 8 Marcel de Rooy 2013-05-23 06:59:49 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2013-05-23 07:00:56 UTC 
Tested it again. Entering the submit button works now.
Signed off.
Comment 10 Marcel de Rooy 2013-05-23 07:12:01 UTC 
My former QA comment still holds.
Good improvement.

Just curious about this line:
 $(e.target).attr('type') != "submit"
Is there a difference with using e.target.type?
Does jQuery not guarantee already that object e contains a 'which' and 'target'?
Note that you assume that in the lines before this line when you directly refer to e.which and e.target.

This tiny inconsistency does not prevent me from passing QA on this patch (second try). Just wait for Owen :)
This patch makes it work too on IE9 which did not work. I could not test on earlier versions of IE. Not encouraging IE for staff at all btw..

Passed QA
Comment 11 Jonathan Druart 2013-05-23 07:40:06 UTC 
(In reply to comment #10)
> Just curious about this line:
>  $(e.target).attr('type') != "submit"
> Is there a difference with using e.target.type?
> Does jQuery not guarantee already that object e contains a 'which' and
> 'target'?

Yes, you are right, it is inconsistent. I will provide a followup.
Comment 12 Jonathan Druart 2013-05-23 07:40:50 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2013-05-23 07:41:48 UTC 
Switch back to Needs Signoff
Comment 14 Marcel de Rooy 2013-05-23 08:17:10 UTC 
Created attachment 18329 [details] [review]
Bug 10310: Prevent submitting form with enter does not work with IE

I am not able to test this patch with IE...
I tested it with Chromium and FF and it works great.

This patch can be tested on the neworderempty.pl page
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2013-05-23 08:17:14 UTC 
Created attachment 18330 [details] [review]
Bug 10310: Followup Prevent submitting form with enter does not work with IE

Check e.target.type instead of using jQuery.
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2013-05-23 08:18:04 UTC 
Nothing to say :) Back to Passed QA
Comment 17 Galen Charlton 2013-05-23 14:41:17 UTC 
Pushed to master.  Thanks, Jonathan!
Comment 18 Tomás Cohen Arazi (tcohen) 2013-06-03 14:19:03 UTC 
Nice fix, thanks Jonathan!

This patch has been pushed to 3.12.x, will be in 3.12.1.
Comment 19 Bernardo Gonzalez Kriegel 2013-06-06 01:15:59 UTC 
Pushed to 3.10.x, will be in 3.10.7
Comment 20 Chris Hall 2013-06-12 08:10:06 UTC 
Apply perfectly since the 9507 fixup.
Pushed both patches to 3.8.x, will be in 3.8.14.