|
Description
Jonathan Druart
2013-05-22 13:48:39 UTC
Created attachment 18314 [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 Created attachment 18316 [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> Tested with FF and IE. Works for both. 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 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. (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 :) Created attachment 18317 [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 (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. Created attachment 18324 [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> Tested it again. Entering the submit button works now. Signed off. 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
(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. Created attachment 18326 [details] [review] Bug 10310: Followup Prevent submitting form with enter does not work with IE Check e.target.type instead of using jQuery. Switch back to Needs Signoff 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> 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> Nothing to say :) Back to Passed QA Pushed to master. Thanks, Jonathan! Nice fix, thanks Jonathan! This patch has been pushed to 3.12.x, will be in 3.12.1. Pushed to 3.10.x, will be in 3.10.7 Apply perfectly since the 9507 fixup. Pushed both patches to 3.8.x, will be in 3.8.14. |