Bug 9507

Just a question:

+function IEEventHandler_KeyDown() {
+    if (event.keyCode == 13 && event.srcElement.type != 'textarea' && event.srcElement.type != 'submit')
+        return false;
+    return true;
+}

But am I missing a parameter here?

(In reply to comment #3)
> Just a question:
> 
> +function IEEventHandler_KeyDown() {
> +    if (event.keyCode == 13 && event.srcElement.type != 'textarea' &&
> event.srcElement.type != 'submit')
> +        return false;
> +    return true;
> +}
> 
> But am I missing a parameter here?

Humm, I just factorize the code, so it is not a regression. I don't have IE for testing it.
In fact event is a global object, it is not necessary to pass it in parameters.
This code should work I think.

(In reply to comment #4)
> I don't have IE for testing it.
:) I would not recommend IE too for the staff client. But if you change code related to IE, it still would be an idea to test it somehow? 

> In fact event is a global object, it is not necessary to pass it in
> parameters.
Yes, you are right (of course). As a purist, you could perhaps say that it should be even better to refer to window.event. 
With Google, I came across this example for a cross-browser solution:

element.onclick = doSomething;
function doSomething(e) {
	if (!e) var e = window.event;
	// e gives access to the event in all browsers
}

(In reply to comment #5)
> With Google, I came across this example for a cross-browser solution:
> 
> element.onclick = doSomething;
> function doSomething(e) {
> 	if (!e) var e = window.event;
> 	// e gives access to the event in all browsers
> }

Yep I think the code can be improved, but the goal of this patch is only to factorize the code.
When this patch will be pushed, it will be easy to improve it: there will be only 1 place to change :)
Don't you agree with me?

(In reply to comment #6)
> Yep I think the code can be improved, but the goal of this patch is only to
> factorize the code.
> Don't you agree with me?
Yes, I agree. Will try to qa this today.
BTW As a non-native English speaker, I think you mean to refactor the code. I thought factorizing was 12=3*2*2 :-)

(In reply to comment #7)
> BTW As a non-native English speaker, I think you mean to refactor the code.
> I thought factorizing was 12=3*2*2 :-)

Ooops :-/ Thanks!

Created attachment 18310 [details] [review]
Bug 9507: prevent submit: refactor some code in a js file

This patch refactors some code in a js file.

Test plan:
On acqui/neworderempty.pl, acqui/orderreceive.pl and
serials/serials-edit.tt try to scan a barcode (or press enter) on the
form and check that it is not sent.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Have to note that the code for IE9 does not work. Before and after this patch.
If we refactor code, it would have been nice to resolve this too.
But I do not oppose pushing this patch.
The test uses 'an ancient Netscape property' window.Event (uppercase!) to make
the distinction between browers and event models. Some more documentation here
would be welcome too.

QA Comment:
See signoff.
Tested under FF. Works well.
Tested under IE9. Does not work before or after.
No complaints from qa-tools.
Passing QA but with some reluctancy as noted.

With appreciation for the intention to improve code BTW!

Pushed to master.  Thanks, Jonathan!

I too encourage a quick follow-up to improve the Javascript, now that it lives only in one place.

This patch has been pushed to 3.12.x, will be in 3.12.1.

I was reluctant to cherry picking refactoring patches, but it is needed by 10310 which is a good bugfix.

Pushed to 3.10.x, will be in 3.10.7

I tried applying this patch to 3.8.x and it came across mostly clean, the only concern I had was

 <script type="text/javascript" src="[% themelang %]/js/cataloging.js"></script>
+<script type="text/javascript" src="[% themelang %]/js/prevent_submit.js"></script>

3.8.x doesn't have cataloging.js, will prevent_submit.js work fine without it?
I didn't want to make a guess.

This patch is required for 10310 so I would quite like it for 3.8.x
Thank you for your attention.

Created attachment 18726 [details] [review]
[3.8.x] Bug 9507: prevent submit: refactor some code in a js file

This patch refactors some code in a js file.

Test plan:
On acqui/neworderempty.pl, acqui/orderreceive.pl and
serials/serials-edit.tt try to scan a barcode (or press enter) on the
form and check that it is not sent.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Have to note that the code for IE9 does not work. Before and after this patch.
If we refactor code, it would have been nice to resolve this too.
But I do not oppose pushing this patch.
The test uses 'an ancient Netscape property' window.Event (uppercase!) to make
the distinction between browers and event models. Some more documentation here
would be welcome too.

(In reply to comment #16)
Hi Chris, this last patch applies on 3.8.x branch.
js/cataloguing.js is not needed for this patch.

(In reply to comment #18)
Thank you very much for that Jonathan :)

Followed the test plan and it worked perfectly, you just have to make sure to select the form before hitting enter as the search box has focus (this is not a critique of your patch but a note to others testing it).

Pushed to 3.8.x, will be in 3.8.14.