Got confirmation that this is /not/ just a bug on webchick.net, so I'm filing here.

1. Enable upload module
2. On some node node, expand "Authoring information"
3. Start typing the name of a user on your system, hit "down" to select the name, and hit "Enter"
4. Note that the Attach button also fires at the same time, and does its thing.

Bizarre.

Doesn't seem to cause any errors, but probably worth looking into.

Comments

Gábor Hojtsy’s picture

Reproduced.

catch’s picture

Same here, very odd.

theborg’s picture

Status: Active » Needs review
FileSize
698 bytes

This problem first presented on this issue: http://drupal.org/node/214292.

To sum up, I belive that when javascript/jQuery modifies any attribute of a submit button, this button automagically gets promoted to 'defaultButton' of the form (an attribute I don't know how to control), so this shoud be part of a wider patch on D7:

a) Modify form api theme_button to allow the creation of html button type, or
b) Build a wider js function that converts all but real submit type inputs to button type.

I've sent an email to John Resig about this issue to bring some light on it.

quicksketch’s picture

If I were to take a guess, I bet that this is the fault of the jquery.form.js file. Post to subscribe. Thanks for looking into it theborg.

theborg’s picture

FileSize
5.95 KB

Further investigations

I've been using a test form built upon 'search form' to try get the reason the default button is changing. Some things I've found until now:

a) The default button of any form is the first submit button of the DOM inside that form. For that reason when webchick was pushing enter key the 'attach' button was firing.

Solution proposed: To build every form with only two 'official submit buttons' (including reset) either by adding a class/attribute to the defaults and letting theme_button decide or directly modifing all the forms.

b) When using jQuery or even with pure Javascript one submit button is removed, the form losses the default button and IE beeps. Adding another button converts the new one in the default. (weird!) BUT when the button removed is the default, IE assings the next button as default correctly.

Solution proposed: Take a look at collapse.js to see another way of adding the slide effect.

I'm attaching the form I've used to do the tests (is linking jQuery code to the official site).

KarenS’s picture

A related issue is at http://drupal.org/node/218997. Clicking on *any* textfield in a pollform will create a new add more button. Copying from that issue:

On any form that has an AHAH add more button (like a poll), clicking the enter key on any text field in the form will trigger the AHAH behavior and add a new element.

Steps to reproduce:

1) Enable the poll module and create a new poll.
2) Move to the title field and hit the enter button.
3) A new element will be added to the AHAH add more section.

Hitting the enter key in any other textfield in the form will create the same effect.

We also saw the same behavior in the D6 version of CCK when putting autocomplete forms inside an AHAH add more section and then selecting an option by hitting the enter key. Each time you hit the enter key a new add more element is added.

Nedjo is here by me looking at this and thinks the problem is nothing to do with autocomplete but instead that pressing the enter key is triggering a form submission and our javascript is reacting to that.

webchick’s picture

Priority: Normal » Critical

Ok, this is probably critical then. This sounds a bit nasty.

starbow’s picture

subscribing

KarenS’s picture

FileSize
541 bytes

Nedjo came up with a simple one line solution that works for me by changing the AHAH event from 'click' to 'mousedown' in form.inc. Patch attached.

Gábor Hojtsy’s picture

KarenS: does that fire, if you use your keyboard to tab to the button and push the button? Or when you just press enter in a one-field form, like the search block form? (Ie. when a mouse is not involved)?

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Well, it is better to test instead of asking :) I tried to reproduce the problem and it indeed appeared without the patch. With the patch however, the "Add another choice" button is flashing but it does not submit. Tested in Firefox 2.0.11 and Safari 3.0.4. The flashing happened in both browser, so it is probably not a browser specific issue, but it is so minor to hold this up.

BUT, with the patch applied, keyboard submission of forms such as the poll form results in the AHAH event firing 5 times (I get 5 more new poll options instead of 1). Without the patch, I only get 1 more option on keyboard submit. So there is indeed a keyboard side effect of this suggestion. (Reproduced in both browsers).

Gábor Hojtsy’s picture

Doh, while taking a shower, I realized that this is probably not the AHAH event firing 5 times, and indeed, it is basically not firing, but the usual form submission happens, which adds 5 new options.

So basically, as I suspected, the use of 'mousedown' instead of 'click' robs off the AHAH forms from keyboard submission. It ties AHAH submission to the mouse. Whether this is acceptable is up for debate. A better, keyboard friendly solution would be ideal IMHO. If nothing like that comes around, all we can do is to commit this for now and then reopen this bug for later fixing (I suspect that's a non-critical bug), since AHAH forms should be usable with the keyboard as well.

theborg’s picture

Status: Needs work » Needs review

As I said in #5, after some tests, the problem appears when the user press the 'enter key'.

I think its about the default button of the form. The enter key triggers submit on the first submit button it finds on the form, and modifing DOM like in collapse alters this behaviour.

The patch in #3 still is valid to these issue, please do some tests.

theborg’s picture

#3 It works for me on Karen's http://drupal.org/node/218997.

KarenS’s picture

Status: Needs review » Reviewed & tested by the community

I no longer have Nedjo at my side to comment on this, unfortunately, but but I tried the patch in #3.

The patch fixes the problem in the poll module and also the original problem with the CCK add more button, and also preserves the ability to use the enter key on the button.

I also tried to reproduce the problem in #1 and was able to reproduce the problem without the patch but the patch fixed it.

So I would say this is the right fix and is RTBC unless any of the javascript experts see any reason it might break something else. I am definitely *not* a javascript expert so I don't know that :)

keith.smith’s picture

(Very, very, minor, but note that the patch in #3 includes a code comment with the word "behaviour". We had a round earlier before the string freeze that moved these to the US spelling of "behavior". It'll be a couple of hours before I can roll a new patch so I wanted to mention it here.)

catch’s picture

FileSize
697 bytes

fake editor re-roll for the code comment eurospelling.

KarenS’s picture

FileSize
661 bytes

Rerolled the patch with the spelling fix in #16.

KarenS’s picture

Status: Reviewed & tested by the community » Needs work

Oops, I tested in Firefox and all was well. Trying now in IE7 it may not be working. I'll post back.

KarenS’s picture

Here's what I found:

In latest Firefox for Windows, everything is broken without the patch and works correctly with the patch.

In IE7 for Windows, there is no problem with the 'add more' button, with or without the patch. Hitting the enter key does not trigger a form submission, so the bad behavior doesn't happen. However, IE 7 does have the problem noted in #1 and the patch has no effect on that.

So the patch will fix some problems for some browsers, and (maybe) has no effect, good or bad, in others. We probably should test in a couple other browsers, but those are the only two I have available.

KarenS’s picture

Note that it is not just the author autocomplete box that triggers the attach button to fire, it will happen if you hit the enter key on any textfield in the form. If you happen to have a file selected in that box, it will get attached.

Nedjo noticed this yesterday when we were trying to figure out what was happening in CCK -- look at where the focus is as you move through the form. It's hard to see in some browsers but is pretty clear in IE7. Create a new story and open up the file attachments section so you can see what is going on. As soon as you click the mouse in the author textfield, the focus changes to the attach button in the file attachments section.

theborg’s picture

Confirmed, IE7 doesn't accept changing input type. Working on a new patch.

cburschka’s picture

I wasn't able to get the Attach button visibly do anything in Firefox 2 or 3. But I suspect it is what is responsible for a node not being either submitted or previewed on pressing Enter.

The patch in #18 does not fix this. What are we supposed to test?

theborg’s picture

FileSize
825 bytes

@Arancaytar. Until we have a better patch you can test this one. It's not working on IE7, but it should do ok with FF.

cburschka’s picture

Sorry, nothing, even after clearing the cache. What should actually happen - should I see a Preview page after pressing Enter in a node textfield, or should the node be submitted?

catch’s picture

Nothing should happen after pressing enter in a node textfield, apart from maybe a line break, or selecting from autocomplete options. Am I missing something?

KarenS’s picture

#1 and #6 describe two situations where the javascript is firing on the wrong action. In both of these situations, hitting the enter key in any textfield on the form triggers the javascript button action.

We're trying to find a patch that will fix this. You need to recreate the problems in #1 and #6, apply the patch, and see if it fixes them, once we have a patch that might work.

I still think the focus question is important. You can't see where the focus is in Firefox, but you can see it in IE7 and hopefully there is some Mac browser where you can see when the button has focus, or you can tweak your css somehow so it shows you which element is active. Clicking in the author textfield puts focus on the 'attach' button (which it obviously should not). Once the focus is there, hitting the enter key is obviously going to fire the button.

cburschka’s picture

#1 and #6 describe two situations where the javascript is firing on the wrong action. In both of these situations, hitting the enter key in any textfield on the form triggers the javascript button action.

How can I see this javascript action being triggered? Because nothing visible happens on the page for me. Sorry if I'm missing the point here...

catch’s picture

Reproduced and patch fixes behaviour in FF, Safari 3 and Opera on XP.

On IE7 all my fieldsets are collapsed and won't expand. Get this error: Line 30, Character 9, could not get the type property, command is not supported

edit: Arancaytar - you need to expand both the authoring information and upload fieldsets. Then use the authoring autocomplete, then enter to select a username - then you'll see it. Patch stops this from happening on everything except IE.

cburschka’s picture

Finally I was able to reproduce this! It actually happens in IE 6.0. However, I do not see this behavior in Firefox - neither in Firefox 2.0.0.11 nor in Firefox 3.0b2. The site is unpatched. Weird? I'll try again on another computer.

Gábor Hojtsy’s picture

Arancaytar: many of us reproduced it in Firefox and other browsers as well, so it looks quite weird.

keith.smith’s picture

(Not to get off track, but in Arancaytar's defense, I can't reproduce the file attach thing either using Firefox 2.0.0.11 on Vista. I can reproduce the poll thing in Firefox.)

dvessel’s picture

I can reproduce both and problem #1 also occurs in Drupal 5. Both problems could be fixed if we had a way to create a context around each button then limit key presses to that scope. ie. this button belongs to these fields.. Not quite sure how it would work though.

As theborg already mentioned. This will *always* happen when there are multiple submit buttons and will default to the first all the time. We need smarter button handling.

theborg’s picture

FileSize
1.2 KB

Only in IE type attribute is read only, so I've built a button from scratch with the attributes of the submit button to convert.
Cloning attributes in IE is also a nightmare!, the only way I found is with a propietary IE function.

After cloning the submit button and coverting it to a button type, another problem presents:
The 'default button' enter keypress behavior is lost as in #5.
So, I am cloning the 'edit-submit' button of the form to activate it.

theborg’s picture

Status: Needs work » Needs review

After lots of trial and error with the IE doomed DOM, lets mark this to review.

nedjo’s picture

Status: Needs review » Needs work

The general issue of triggering unwanted submits by hitting enter in a text input is commonly hit. Various approaches have been tried, including

* attach a keypress event handler to the form or to text inputs and prevent submission if the enter key was pressed
* attach a submit event handler to the form returning false (preventing submission) and a click handler to submit buttons programmatically submitting the form

Both of these seem involved and likely to trigger conflicts with other behaviours.

I'm not sure we're going to find a single solution that covers all cases (JS or not, etc.). For Javascript purposes. theborg's approach feels appropriate. We convert our submit button into a "push button", http://www.w3.org/TR/html4/interact/forms.html#push-button--that's what is is, a button to trigger a Javascript action.

Let's try this (I don't have a test site handy to do so):

      if ($(element_settings.element).attr('type') == 'submit') {
        $(element_settings.element).attr('type', 'button');
      }
theborg’s picture

Status: Needs work » Needs review

Not working on IE:
MSDN docs:
"As of Microsoft Internet Explorer 5, the type property is read/write-once, but only when an input element is created with the createElement method and before it is added to the document."

jQuery clone() it doesn't work.

nedjo’s picture

FileSize
744 bytes

Here's a roughed in, untested draft of what we might need to do.

Changes:

1. The logic needs to be within the iteration loop, since we need to apply it to each element.

2. If we have a select element, we clone it, change the type attribute, append the clone, then remove the original.

Whether cloning convinces IE we should be able to change the type I don't know.

I've heard anecdotally the clone method sometimes fails in IE7.

nedjo’s picture

Title: JavaScript firing on wrong objects » AHAH triggered by text input enter key press, breaks e.g. autocomplete

More descriptive title.

theborg’s picture

Title: AHAH triggered by text input enter key press, breaks e.g. autocomplete » JavaScript firing on wrong objects

@nedjo. #38 is not working on IE6/7 it gives and exception on *changing type*.

I did the same in #24, for non-IE browsers changing type of inputs is the only thing needed.

#34 still applies.

theborg’s picture

Title: JavaScript firing on wrong objects » AHAH triggered by text input enter key press, breaks e.g. autocomplete

Sorry, I didn't want to change the title.

nedjo’s picture

FileSize
909 bytes

Fix a couple of problems: Need to set the new element as the one to attach AHAH to. Still untested.

Even if it turns out to work, this cloning and appending feels clunky.

Of course, if we were only concerned with JS, we could render a button in the first place. (It's always seemed counter-intuitive to me that we have a Forms API element of type 'button' that renders, not as a button, but as a submit input, like, um, the 'submit' element.) But for non-JS usage we need the submit button.

We could consider testing for JS support on the server and rendering a button accordingly, but that would introduce a whole new set of issues (what if a user comes straight to the AHAH page and doesn't yet have a JS cookie flag? etc.).

theborg’s picture

Tested on IE6 and this code .attr('type', 'button') throughs an exception.

nedjo’s picture

Status: Needs review » Needs work

@theborg, thanks for reporting back.

#34 is enough to establish that hacking to try to get IE to work is not our approach.

I wonder if we can work with the event target--determine if the submit button was the target of the event, don't trigger ajaxSubmit if not. I'll try.

nedjo’s picture

Status: Needs work » Needs review
FileSize
2.14 KB

I tried the event target approach, but ran into another IE wall. Here's the patch I tried on ahah.js:


-  $(element_settings.element).bind(element_settings.event, function() {
-    $(element_settings.element).parents('form').ajaxSubmit(options);
-    return false;
+  $(element_settings.element).bind(element_settings.event, function(event) {
+    // Ensure this is the target element of the event.
+    // This test prevents e.g. submits triggered by text input enter
+    // key presses.
+    if (event.target == element_settings.element) {
+      $(element_settings.element).parents('form').ajaxSubmit(options);
+      return false;
+    }

Firefox nicely recognized the text input as the event target, so skipped ajax submission. But good old IE insisted the target was the submit input, so no dice.

So here's a different approach. We stick with the 'mousedown' event, which is working okay so far as it goes. Then we address the keyboard entry use case. I've done this by adding a new setting in the array we're passing, keypress. If this flag is set, ahah.js adds an additional event handler to watch for keypress and, if it was the enter key pressed, trigger the ajaxSubmit (by triggering the mousedown event).

Tested on IE and Firefox, seems to be working.

I'm not totally happy with the result. I'd really prefer not to be messing around with mousedown and keypress events. But maybe this is the cleanest we can come up with. At least it's a single cross-browser solution that doesn't resort to IE hacking.

cburschka’s picture

Patch #45 applies to includes/form.inc with 2 lines offset, but no other problems.

Testing in FF 2.0.0.11, FF 3.0b4pre, Opera 9.24, IE6. All on Win XP SP2.

First bug:

1.) Enter /node/add/page
2.) Expand File attachment and Authoring info
3.) Enter Author field, allow autocomplete to trigger, press Cursor-Down, Enter to select a name, Enter again to submit.

Bug A behavior: File attachment AHAH visibly triggers on both Enter presses.
Bug B behavior: File attachment does not trigger on first Enter to select, but does on second Enter to submit. The form still cannot be submitted by pressing Enter.

Correct behavior: The author info field gets filled with no other visible results; the form gets submitted on the second Enter press.

Test results:

             Pre-patch     Post-patch
FF 2.0.0.11        Correct       Correct
FF 3.0b4pre        Correct       Correct
Opera 9.24         Bug B         Correct
IE 6               Bug A         Correct*

*The "Attach" button visibly gets focused, but nothing else happens so we can let this slide.

Second bug:

1.) Enter /node/add/poll
2.) Focus the node title field
3.) Press enter

Bug A behavior: The AHAH triggers to add more elements to the poll.
Bug B behavior: The browser refuses the Enter keypress as an invalid command, accompanied by an alert sound.
Correct behavior: The node form is submitted. Note that 5 more choices are statically added by the page when reloading the form (in order to gracefully degrade).

             Pre-patch     Post-patch
FF 2.0.0.11        Bug A         Correct
FF 3.0b4pre        Bug A         Correct
Opera 9.24         Bug A         Correct
IE 6               Bug B         Bug B

A pox on that crappy browser. >_<

Note a buggy behavior observed for both tests in all browsers, pre and post patch: Submitting the form by pressing Enter sends a query to the server, but does not actually do anything beyond validating the form. None of the submit hooks get triggered. It should at least trigger the Preview action as a default. I think this is a bug, though perhaps not a critical one, and certainly an unrelated one.

theborg’s picture

@Arancaytar: Thanks for this detailed tests!

I've also seen the bad behaviour you noted on comments only validating but no submitting the form when pressing enter key. This is another bug.

With #46 IE bug B is not solved, probably because IE looks for the default submit (the attach button) to trigger the form.

nedjo’s picture

@Arancaytar: yes, thanks for the detailed analysis!

On the second (poll) bug, I don't get bug B. For me in IE 6 the pre-patch bug is the same in IE an in FF - the AHAH triggers to add more elements to the poll - and is resolved by the patch at #45. Could this issue be dependent on the version of IE 6?

It's not clear how or even if bug B, when it does occur, is related to the AHAH behaviour.

cburschka’s picture

Let it be noted that my version of IE is at least partially broken (well, more broken...), and my memory was bloated at the time of the test. I'll try again to make sure, and even if I do get it, I wouldn't let this stand unless someone else can reproduce it. We've established that my computer is acting weird...

cburschka’s picture

... unfortunately I've reproduced it both on my machine and another. Please try to add a poll at my test site (no registration needed, guests can create polls) to compare results.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Patch works for me in FF2, IE7 and IE6 - also tested in opera. If I keep hitting enter multiple times in the empty 'authored on' text field, I can get it to start adding poll options again, but that'd be a pretty stupid thing to do.

So since this fixes the worst aspects of both bugs, and anything else is minor (and possibly separate issues), I think it's ready.

KarenS’s picture

I tried the patch in #45 in both FF and IE7. Everything works as expected in FF -- pressing enter on a poll choice field generates a full form submission with five new fields, pressing enter on autocompletes in either the author or CCK add more forms selects the value without triggering the AHAH.

In IE7, everything works as above except that pressing enter in a poll choice triggers a warning sound instead of a form submission (seems to be the same as IE6 behaves as reported above). I checked that the form does not lose its ability to be submitted by hitting enter on the submit button, it just won't submit if you hit enter in a textfield.

I tested both FF and IE 7 to be sure that submission with the keyboard instead of the mouse still works right, both for form submission and the AHAH button submission, and it seems to work correctly.

Seems like a reasonable solution that fixes most problems, with the last one being very minor (that IE won't submit the form by hitting the enter key in a textfield). You could even make an argument that the form should not submit when you hit the enter key on the poll choice, so I don't know that that is even a problem.

KarenS’s picture

Should we add some more explanation to the code to explain why this has been done so someone doesn't decide later to clean this 'unnecessary' code out? I'd suggest some, but I doubt I know how to explain it right.

nedjo’s picture

FileSize
2.58 KB

I've added some code comments and also fixed a small error (I'd called the property keydown instead of keypress).

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review

From the looks of the latest change, it seems that the keyboard events were not tested (the fixed code fixed that).

cburschka’s picture

The bugs described were fixed by the time I tested. What is the keypress event supposed to change in the apparently working behavior? If I know what to look for, I can repeat my test tonight and include Kestrel in the list of tested browsers.

catch’s picture

Tested in FF2, IE7, Opera 9.25 and 9.5beta - works as advertised in all of them, for both bugs.

Gábor Hojtsy’s picture

Version: 6.x-dev » 7.x-dev
Status: Needs review » Reviewed & tested by the community

Thanks for retesting catch. Committed #54 to Drupal 6.x. Still RTBC for 7.x.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

effulgentsia’s picture

The fix here introduced its own problems, and still leaves some questions. Please help out on #634616: Various problems due to AJAX binding to mousedown instead of click if you're interested.