Closed Bug 72906 Opened 24 years ago Closed 23 years ago

Double click on submit causes form to submit twice[form sub]

Categories

(Core :: DOM: Core & HTML, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.0

People

(Reporter: pollmann, Assigned: john)

References

Details

(Keywords: topembed+)

Attachments

(4 files, 6 obsolete files)

In bug 72773, Brendan notes that clicking on the submit button twice will cause the form to be submitted twice. We need to disable form submission if a submission is already in progress. To reproduce the problem: - View the attached test case - Click on the submit button Note the Request ID you are returned (a) - Double click on the submit button Note the new Request ID you are returned (b) Observed result: b-a == 2 Expected result: b-a == 1 The Request ID is incremented once per request, so if the request ID has gone up by two, your double-click resulted in two form submissions, when it should only have resulted in one.
Oops, to reproduce the problem: - View the attached test case - Click on the submit button Note the Request ID you are returned (a) **- Click on the "back" button** - Double click on the submit button Note the new Request ID you are returned (b)
Status: NEW → ASSIGNED
Attached file test case (deleted) —
Nominating. I'd like to nominate for 0.9, cuz I've done this twice by accident, but maybe it falls off the priority list. /be
Keywords: mozilla1.0
Attached patch patch (obsolete) (deleted) — Splinter Review
The problem is that, since form submission is not synchronous, every time a form submit event occurs (could be initiated by JS, clicking on submit button, pressing Enter in a text input, ...), a new load event will be thrown on the queue. There is nothing preventing multiple events from being thrown on the queue, as long as they are all added before one of the load events causes the document to go away. This patch ignores any requests after the first one. This should not cause a problem with the case where someone fires a submit event but cancels it in a JS event handler, because in this case, the event will be cancelled before nsFormFrame::OnSubmit is called.
Whiteboard: fix in hand
Target Milestone: --- → mozilla0.9
That patch looks out of date w.r.t. the trunk top revision of nsFormFrame.cpp. How about using PRPackedBool instead of adjacent PRBools in a class or struct? Forgive my ignorance, but what if the form targets another window, or otherwise does not lead to its own destruction on submit? Won't the mAlreadySubmitted flag be stuck set, and you'll never submit again without reloading the page? I didn't see code other than in the dtor to clear mAlreadySubmitted. /be
Oy, both very good points, I'll have to think about this more. The problem with targetting another frame is going to be tricky... thanks for pointing that out!
Whiteboard: fix in hand
When any of (1) response from server (or JS engine if form's action URL is javascript:...) comes back; (2) submit times out; (3) user aborts you want to clear that flag. Also, I think you need to cue the front-end to make the submit button appear insensitive or disabled when setting the flag, and re-enable it when clearing the flag. /be
Target Milestone: mozilla0.9 → mozilla0.9.1
Tom, shouldn't we be getting a single "double click" event instead of two click events and a "double click" event?
Will this fix prevent double submission at http://www.ncbi.nlm.nih.gov/entrez/query.fcgi ? If you hit return twice, or double click "Go", the current result is overlapping content in single window. This disables Mozilla from responding to any input.
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Target Milestone: mozilla0.9.2 → mozilla0.9.1
Priority: -- → P1
Pushing back - end of milestone reality check.
Target Milestone: mozilla0.9.1 → mozilla0.9.2
*** Bug 76146 has been marked as a duplicate of this bug. ***
Added crash keyword because Bug 76146 which has been marked as a dup of the bug results in a crash
Keywords: crash
Severity: normal → major
Keywords: nsBranch
Target Milestone: mozilla0.9.2 → mozilla0.9.3
Removing crash keyword. I'm not seeing a crash on the bug that was marked a dup anymore.
Keywords: crash
Missed 0.9.3.
Target Milestone: mozilla0.9.3 → mozilla0.9.4
This is P1 | Major for TM 0.9.4, and pollmann is no longer with us? Asa, any ideas on who should get this one? --> trudelle
Assignee: pollmann → trudelle
Status: ASSIGNED → NEW
cc'ing rods 'cause it's a form control; saari because of events.
I've got nothing to do with form submission. ->rods?
Assignee: trudelle → rods
Set milestone to mozilla0.9.5
Target Milestone: mozilla0.9.4 → mozilla0.9.5
over to eric
Assignee: rods → evaughan
Blocks: 99225
Marking nsbranch-.
Keywords: nsbranch-
removed keyword nsbranch since it now has nsbranch-, per pdt mtg.
Keywords: nsbranch
taking bug, I have a fix for this with a different bug
Assignee: evaughan → rods
Attachment #28433 - Attachment is obsolete: true
See patch in Bug 85286, it uses the nsIWebProgressListener to know if the submit was successful.
Status: NEW → ASSIGNED
Yes, the Request IDs are now correct, one after the other.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Fixed
verifying on 2001-01-10 branch
Status: RESOLVED → VERIFIED
verifying on 2001-01-10 branch on windows 98
I had to back out the fix
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
moving to 0.9.6
Target Milestone: mozilla0.9.5 → mozilla0.9.6
reassinging to new owner of form submission
Assignee: rods → alexsavulov
Status: REOPENED → NEW
Summary: Double click on submit causes form to submit twice → Double click on submit causes form to submit twice[form sub]
Blocks: 107067
Keywords: nsbranch-
retargeting to 0.9.8
Target Milestone: mozilla0.9.6 → mozilla0.9.8
it think that's a matter of event handling if not, reassign back. thx!
Assignee: alexsavulov → joki
Component: Form Submission → Event Handling
QA Contact: vladimire → madhur
Alex, I don't think there's any way for the button to know whether the form is currently in the process of being submitted ... they could disable the button after submit, but then there are other submit buttons on the screen that the user can click, and there is JavaScript form.submit() that can trigger it. I really think it's best to handle this at the form submit level.
well the botton could prevent a second click anyway. if it was clicked once it could deactivate itself be cause is not needed anymore.
If we just disable the button when the user clicks it, we have to reenable it in case the submit fails, the user cancels, or the submit completes in another frame. It's just as hard to do there as it is in form submit, and it gives more benefits when done in form submit.
But otherwise we have to work with flags, observers and so on to prevent double submitting and my feeling tells me that we're going to run into troubles. I know, dissabling the button is not a problem free solution either. particulary the case where the user cancels the submiting or as John mentioned above where submission fails is where we have to renable the button. John: If you feel that you have a solution let me know please. Also feel free to take ownership if you want.
The proper solution is to set an isSubmitting flag in the form to TRUE when you start the submit and have a listener find out when the LoadURI() finishes for any reason, who then sets the flag to FALSE. See Eric's patch (which was fine as far as it went) for the first part of the solution. Events have to do precisely the same thing, it's just that their "flag" is the disabled attribute on the button.
ok! convinced :-) re-taking
Assignee: joki → alexsavulov
Component: Event Handling → Form Submission
QA Contact: madhur → vladimire
You guys realize that developers all over the world have spent thousands of hours of effort to develop server side and JavaScript solutions to prevent users from double clicking their way to purchasing two copies of the same item, etc? It's great if you can build a solution into the browser but, in my not so honest and eminently ignorable opinion, this one should have a low low priority, even if Mr. Eich is prone to double submitting patches. ;-)
oh man! we still want to fix this one. even though IE does exactly what we do now :-)
this patch is ment to go in the docshell. i don't really know if this will be accepted, but it works pretty nice and does not need a listener. take a look at it folks an tell me what you think. I will attach a testcase that simulates pretty much all kind of problems. unfortuantely the response server is available only within the firewall. sorry 'bout that. what does this patch do? is adding a registry object to keep track of every submission recording the referrer URL and DocShell (the place where the user clicks on submitt) and the target URL and DocShell (the place where the resulting page will be displayed). If there is a submission going on, and something (user or JS) tries to submitt until the submission didn't finish loading, the entry will be found and the submission canceled. when the document ends loading, all the entries related to the target docshell will be removed so that other submissions can be done. Folks: please take a look at this and tell me what you think. I need to know what can be done better here. Also tell me where do you think that the new created objects should go + there si a forced cast from nsIDocShell to nsDocShell that may be written differently and so on. I know that this patchis still ugly and needs a bunch of cosmetic improvements. I didn't wanted to make a final patch be cause I'm not sure if everybody accepts this solution. thx
ps: do you think I should rather use nsXPIDLCString instead of autostring?
Attached file testcase for the patch (deleted) —
My two cents: I would prefer to have it in the form with a listener personally, for many reasons (readability and performance high among them). I don't really like having a registry like that--it'll have some strange quirky behaviors, for one thing, because it works with URLs rather than forms. And doing it in DocShell means we waste time iterating over the form controls in nsFormSubmitter before we find out we really don't want to submit. I looked into it. Listeners really aren't that bad :) You implement nsIObserverService (or extend nsObserverService) in DocShell and implement nsIObserver in the form and wallah, listeners ready-made. You can think of it as just one guy calling a method on a bunch of other guys when an event occurs. It doesn't even have to be a true service--you don't have to export it to anybody and deal with any of that crap. Just call the AddObserver() method on the DocShell directly from the form. You won't be the only one to use the observer thingy in DocShell, too :) I strongly suspect I am going to use the service (with an extra event for document unload) to help implement bug 108309. And it looks like there's already a call in EndPageLoad() that is essentially a notification to the mDocumentViewer.
John: but we have to keep track of which DocShell initiated a submit. Don't we? I tried to imagine an observer service that gets notified everytime something and i cannot go around this kind of list or whatever keeps the information alive about the referrer and target shell. I agree with you: implementing an observer is an elegant solution. Btw: do you think to have the DocShell itself to be the observer? What if is getting destroyed? (ok, i suppose that it won't submit anymore :-) Maybe the observer should maintain that list and live independent of any DocShell. I don't know yet. I'll try to change the patch. (BIG HMMMM....)
from john's comment: "And doing it in DocShell means we waste time iterating over the form controls in nsFormSubmitter before we find out we really don't want to submit." Yeah, that's where an observer is the bests way to go!
I don't think the form would have to remember which DocShell initiated the submit. It just has to add itself as an observer to the DocShell that it calls LoadURI() on. Then it can sit back, relax and wait for the stop_load event to roll in. (Essentially the DocShell is keeping track of which *form* initiated the submit--in its list of observers.) ObserverService is actually the equivalent of Java Observable. It is the guy who is sending out the events (in this case the DocShell). He is responsible for remembering who was interested in the events so that he can tell them when the events happen. The Observer is the guy who receives the events (in this case the form). Doesn't DocShell stay around for the entire period of the browser / tab / frame? I admit ignorance to this object, it was my impression that it was a single object that may contain (or work with) many documents. So if the DocShell is being destroyed, that means our document is going away and therefore, so is the form. So we really have no trouble :) It is only when the DocShell still lives that we care about it. But you bring up a good point--which DocShell do we listen to to find out when the page stops loading? If the start happens on one object (like the current document) and the end happens on another (like the target frame) then does the EndPageLoad() method happen on the targetted frame's DocShell or the form's DocShell? (It may be both.) In that case a possible solution is to add an nsIObserver parameter to LoadURI() that specifies the observer to be notified when the final document is finished loading. I really think this will turn out to be useful to others. It seems like a common operation.
Yes! If the target of an action is for example "_blank", the target DocShell that ends the load (executes EndPageLoad) is another one. take a look at the following code: NS_IMETHODIMP nsDocShell::InternalLoad(nsIURI * aURI, nsIURI * aReferrer, nsISupports * aOwner, PRBool aInheritOwner, const PRUnichar *aWindowTarget, nsIInputStream * aPostData, nsIInputStream * aHeadersData, PRUint32 aLoadType, nsISHEntry * aSHEntry) { .... if (aWindowTarget && *aWindowTarget) { .... if (!targetDocShell) { rv = FindTarget(name.get(), &bIsNewWindow, getter_AddRefs(targetDocShell)); } .... if (targetDocShell) { .... rv = targetDocShell->InternalLoad(aURI, aReferrer, .... return rv; } .... rv = DoURILoad(aURI, aReferrer, owner, aPostData, aHeadersData); } If we don't add that information, the target DocShell would never know who is waiting for the end of the load. On the other side only the initiating DocShell knows for sure which is the target DocShell. The form cannot know that in advance if the target is another one than the current DocShell. But there is a big problem in all the implementations: Is when the target docshell is getting destroyed before it can finish its job. If EndPageLoad is not called, the listener needs something like a timeuot or so to make the submission possible again. (The proposed registry object does not remove the blocking entry). Sometimes it can happen that we get a hang (very often seen in Mozilla :-), the throbber "throbbs", and nothing happens. The user gets angry and clicks on the X of the new window and destroys the DocShell. From that point, goodbye my EndPageLoad. Now if the DocShell would notify the listener before is getting destroyed, that might solve the problem. (also the registry could be cleaned up in the destructor ofthe docshell). John, could you send me a sketch example about how do you think to create the observer and observer service? I didn't get to the point of finding out how to do that. Thx!
sorry i didn't finish a phrase: "if the target is another one than the current DocShell..." the only thing the form knows is the fact that the target DocShell will be another one at the submitting point.
Ok John, i got it! - I will create a new topic for the existent nsIObserverService "@mozilla.org/ observer-service;1" called [whatever]; - As you said, I add/implement the nsIObserver to the form; - I add a new member to the DocShell called mReferrerForm (stores the form pointer or whatever we have to identify the form) and a method to set it; - In DocShell::InternalLoad I set the mReferrerForm of the target DocShell; - At the beginning of every submission, I set a mIsSubmitting in the form that can be reset only by the future Observe method of the form What do you think about his?
oops, i forgot: - in nsDocShell::EndPageLoad I notify the observers (in this case, the form) the load has finished for the mReferrerForm. Every Observer looks and the one that matches, resets his mIsSubmitting.
You pretty much got it! ... but instead of storing it in a member variable I'd add it to the list of observers on that topic. Otherwise the mechanism is still non-general. Here's what I'd do: - add a member mIsSubmitting to nsHTMLFormElement - implement nsIObserver in nsHTMLFormElement (sets mIsSubmitting to false and calls RemoveObserver(this) on the DocShell's observer service) - set mIsSubmitting to TRUE at the beginning of form submit - add a new topic for "end_page_load" to the observer service in DocShell - add a parameter nsIObserver* endLoadListener to LoadURI() or whatever function is being called - call the target DocShell observer service's AddObserver(endLoadListener, "end_page_load") when you find out the target doc shell - on EndPageLoad(), call the observer service's NotifyObservers(new PRSupportsInt32(aStatus), "end_page_load", "") (Note: I am not 100% certain that the 1st and 3rd parameters to NotifyObservers there are right. I wanted to pass aStatus so that we can replace some code that already exists in EndPageLoad()--see that call where it calls out to another place with aStatus as the parameter?) Essentially AddObserver takes the place of mReferringForm. NotifyObservers() will call Observe() on everyone who is listening on that topic.
Attached file patch to fix the bug (obsolete) (deleted) —
this is the observer / observer service version of the patch. What does this? - Is extending the class nsHTMLFormElement with nsIObserver and nsSupportsWeakReference. - Is implementing an own version of ::Observe that reacts on "end_page_load" - Is adding an nsObserverService member to the nsDocShell - Is calling AddObserver with a pointer to the referrer nsHTMLFormElement in the method ::InternalLoad (the method arg list had to be extended with one argument) - Is calling NotifyObservers in the method nsDocShell::EndPageLoad; be cause the observer supports weak references, if the observer is not there the notification won't occur - the class nsObserverService declartion is adapted to export the methods (compiles on Win32, but will it compile on Mac / Linux? not sure) need r= / sr=
Attachment #61548 - Attachment is obsolete: true
Attached file replaces the prevoius patch (obsolete) (deleted) —
i forgot to notify the observers in the destructor
Attachment #62158 - Attachment is obsolete: true
same descrpition as aabove the only dofference is that in this one we create an enumerator class so the DocShell does not have to inherit from nsObserverService over the module boudary needs r= / sr=
Blocks: 104417
we're not yet ready with it re-setting milestone
Target Milestone: mozilla0.9.8 → mozilla1.1
Blocks: 85286
re-setting milestone (was wrong) reassigning to john
Assignee: alexsavulov → jkeiser
Target Milestone: mozilla1.1 → mozilla1.0
Blocks: 76605
Status: NEW → ASSIGNED
Marking nsbeta1+
Keywords: nsbeta1+
Keywords: topembed
Marking topembed+
Keywords: topembedtopembed+
Blocks: 69483
No longer blocks: 69483
Attached patch Patch (nsIWebProgressListener) (deleted) — Splinter Review
- created an nsIRequestor interface who is notified when the DocShell creates a request. - extended nsIWebProgressListener in nsHTMLFormElement so that we can find out when our request is finished. 1a. When form submission starts, set mIsSubmitting to true. Disallow all submissions from this form while it is true. 1b. The form (in the form of an nsIRequestor) is passed to the link click handler on the stack. 2a. DocShell/WebShell handle the request and, when they create a channel, pass the nsIRequest* to the form (using RequestCreated()) 2b. The form remembers the nsIRequest* and registers itself as an nsIWebProgressListener on the DocShell. 3a. The nsIWebProgressListener eventually sends OnStateChanged(STATE_STOP) to signify that, for whatever reason, the submit is finished. 3b. The form unregisters itself as an nsIWebProgressListener and sets mIsSubmitting to false. One can submit again if one so chooses.
Attachment #62232 - Attachment is obsolete: true
Attachment #62282 - Attachment is obsolete: true
I have tested this patch on normal submissions, the "double submit with ENTER" problem, and with a form that opens new windows on submit. All do as expected: they don't submit twice unless the previous submit has completed.
cc'ing alecf for review of the new dependency of uriloader in layout. note: Rick's mods to nsIWebProgressListener in http://bugzilla.mozilla.org/show_bug.cgi?id=46856 will affect the nsIWebProgressListener impl and usage in this patch. if rick lands first, be sure to change your impl. I'll add a similar note to rick's bug so he knows to catch this if you land first.
Comment on attachment 71613 [details] [diff] [review] Patch (nsIWebProgressListener) this dependency is disappointing to me mainly because it seems really strange for the form element to be an nsIWebProgressListener.. it seems like this should be handled at the webshell/docshell level - i.e. catching the interruption of a 2nd submit to the same URL from the same docshell... but maybe at that level you can't tell the difference between a form submit and some other form of GET/POST? I guess having a DOM element be a web progress listener is just strange to me, especially given the amount of overhead (5 extra methods added to nsHTMLFormElement) on a totally unrelated note, I'm also saddened to see that nsWebShell has nsString*'s as member variables (mURLSpec and mTargetSpec) as opposed to just being nsString.. any chance you can fix that (since you switched us to nsCOMPtr right there anyway)
This cannot be handled at the nsWebShell/nsDocShell level alone--not to be done right, anyway. One form can submit to multiple DocShells (in fact, it *will* do this if its target is _blank). The only place to reliably block a second submit is in the form. Here's a way around the dependency if that's the problem. If we had nsDocShell remember the form that submitted a given request, it could check for that when the request is finished, and call SetSubmitting(PR_FALSE) on the form. I don't think I like having form-specific stuff in DocShell, though. This really doesn't feel like DocShell's responsibility. DocShell has way more than enough stuff to worry about without having to mother forms too. nsString*: I can change nsString* to nsString on the next iteration, if that's what you're after?
One other current reason to use this solution is bug 49361: there is a request out there for a form submit observer method that tells the observer when the channel was opened. This solution could fulfill that request easily. As for future reasons, it is a presumption of mine that nsIRequestor will be useful. I imagine there must be others out there who would be interested in narrowly focused listening to only their own request and no others.
I have no meaningful insight into the form machinery, but "nsIRequestor" seems like a pretty bad name. What are you requesting? nsIDocShellRequestObserver would seem like a better name, though I then wonder if we can't make do with one of our existing nsIObserver-esque interfaces.
Good point about the name, I'll change that. However, I have a personal dislike for nsIObserver; the nature of the interface doesn't tell you much about why you are extending it or what events you'll be receiving from it. It also makes it harder to send arbitrary bits of data across. I prefer listener classes a la Java, and since there are other places in the code that do it that way (i.e. nsIWebProgressListener) that's how I'd prefer to do it.
I don't like nsIObserver either, to be honest, and I'm perfectly happy to hear you say "I looked at it, and it doesn't work for me". I just don't like _inadvertent_ reinvention. =)
ok, so I spent some more time with this patch, and here are my impressions: 1) we need something like nsIWebProgressListener that has more granular information about the request, such as the nsIRequest that necko started with, and that seems to be where nsIRequestor comes in. 2) there are times when nsIWebProgressListener doesn't fire, in places that this code needs it to 3) we're not actually using the "aRequestId" parameter It really seems like we ought to somehow extend the semantics of nsIWebProgressListener, or make it fire more often. the other thing that I'm confused about is how we're uniquely identifying requests - I see a pointer-comparisons between nsIRequests. Is the requestID stored in nsIRequest? I also notice that necko has this concept of nsIRequestObserver - and the onStopRequest has a "statusCode" result which might tell us why the request stopped. I guess that independent of the dependency issue, I'm seeing yet-another-specialized-listener-interface for network connections/requests/etc.. are we sure that there isn't already a mechanism in place (or one that could be slightly tweaked) for the notifications?
1. If I understand you, yes. nsIRequestor fills in the blanks *before* nsIWebProgressListener picks up the phone. Essentially, you have to be able to associate what you give to DocShell (the thing that will eventually generate a request) with an nsIRequest. But at least LinkClickThis is a general pattern that can (and probably will) be used by others who are now merely guessing which request is theirs. 2. In what places does nsIWebProgressListener not fire that it needs to? Talking to rpotts, it sounded like we should always get start/stop events as long as OpenURI() fulfills its contract, but if it doesn't it will return failure code and I will fire nsIRequestor::RequestFailed(), which means I am no longer *expecting* a stop event. I only expect a stop event if RequestCreated() happens but RequestFailed() does not. And wouldn't nsIWebProgressListener not firing affect the throbber and everything else? Issues with nsIWebProgressListener need to be fixed there; if it breaks form submission it's actually a good thing, because maybe it will get more attention. If the design of the nsIWebProgressListener interface itself is broken, I would like to know about that before using it :) 3. We're not using aRequestId for this particular use of nsIRequestor, but in the general case I expect we will. It exists so that you can ask for multiple requests at once and identify which nsIRequest is which. 4. aRequestId is solely to identify the request to the nsIRequestor. It is not even unique across nsIRequestors. From then on, the unique ID for a request is the nsIRequest* itself, AFAICT. That is actually the entire purpose of this interface: to get me the nsIRequest* so I can filter requests on that. 5. As to tweaking another mechanism: if nsIRequestObserver always fires *before* STATE_START, then we could tweak it and add two parameters to OnStartRequest: the event source (nsISupports*) and a request ID unique for that source. Then the source could add itself as a listener on that source and filter where source == this. One big potential hole in this is, we don't even know which DocShell we will end up firing the request from. It may even be a new window that hasn't been created yet. So we definitely won't be registered on that source for nsIRequestObserver notifications. We'd still need a notification that tells us which window our request eventually got created on.
ok, it's good to get this all sorted out. I didn't realize that this stuff was happening before nsIWebProgressListener starts doing things. I didn't mean that it was broken, just that it didn't (per its contract) put out the notifications that you were interested in. And so, what I'm wondering is if nsIRequestObserver would fill your need? It's much less frozen than nsIWebProgressListener (Which is UNDER_REVIEW, so could also be changed to meet your need) but I don't know enough about it either, maybe we can get darin in on this? Seems like it might simplify your code if you didn't have to set up a listener, in order to set up another listener :)
nsIRequestObserver is UNDER_REVIEW and really shouldn't be messed with :-) that being said, i'd like to understand better why it doesn't satisfy as is. in the patch, a nsIRequestor is implemented by the caller of LoadURI. they want to know when the Load starts and when the load finishes. they want to know if there was a problem. this is the kind of thing that nsIRequestObserver was designed to handle. nsIRequestObserver::OnStartReqeust(aRequest, aContext) can replace nsIRequestor::RequestCreated(aRequest, aDocShell, aRequestID) you'd want to dream up some interface w/ aDocShell and aRequestID getters. likewise, nsIRequestObserver::OnStopRequest(aRequest, aContext, aStatus) can replace nsIRequestor::RequestFailed(aRequest, aDocShell, aRequestID) nsIRequestor::RequestNotCreated(aRequestID) just come up with meaningful nsresult's pass as aStatus that correspond to the various event chains (e.g., "RequestCreated -> RequestFailed -> RequestNotCreated") it could work like this: 1) always fire OnStartRequest to indicate that things are starting to happen 2) fire OnStopRequest(..., NS_OK) // to indicate success or OnStopRequest(..., NS_ERROR_XXX_REQUEST_NOT_CREATED) // to indicate that the request could not be created or OnStopRequest(..., NS_ERROR_XXX_REQUEST_FAILED) // to indicate that the request is created but fails such that no // nsIWebProgressListener calls will be made. in fact, why do you need to observe nsIWebProgressListener events? why not just make OnStopRequest fire when the nsIRequest is done? isn't that the only event you really care about? it looks like you're only observing the OnStateChange(STATE_STOP) event. keep in mind: nsIRequestObserver requires OnStartRequest and OnStopRequest to come in pairs. finally, why can't you use the nsIRequest in place of a numeric aRequestID? i guess it depends on who is creating aRequestID... hmm.
The problems here are twofold: 1. I don't know which DocShell I'll be listening to since I could be submitting to a new or different window 2. A single DocShell can handle many requests at once, and I need to know just when the actual submit request finishes (not, for example, images or other content). Perhaps aContext could handle the second problem? What do we do about the first? aRequestId is generated by the requestor, so that I can identify which nsIRequest coming back from the DocShell is which in nsIRequestor in case I send out multiple requests.
Oh, and another thing I'm worried about: can I even *add* myself as an nsIRequestObserver on the form? Some observer patterns are 1-1--multiple people cannot listen at once. I haven't found the place where RequestObservers are added to whoever is firing them.
Attached patch Patch (synchronous click) (obsolete) (deleted) — Splinter Review
This version does not need a new interface. Instead it has a synchronous LinkClick method on nsILinkClickHandler that we call and get back the nsIRequest* and nsIDocShell* immediately. Also fixed other issues of reporters. uriloader dependency still exists. nsIWebProgressListener still exists. Either the form has to listen to the request or else docshell has to listen and tell the form what it hears. I prefer this method; it's more explicit about what we're doing.
After a once-over on the patch, I'm definitely happier with this solution, even though the dependency still exists. It seems more explicit what's going on here.. I'll review this today...
Attached patch Patch (synchronous click) (deleted) — Splinter Review
The previous version of this patch was missing a diff from webshell. That is included at the end of the patch, everything else is identical.
Attachment #72024 - Attachment is obsolete: true
Is this bug asking to modify the behavior of all forms, or just post forms? Also, is it about double-clicks submitting twice, or is it about two separate clicks submitting twice? I often click the submit button a second time when a search result page doesn't start to load. I would be annoyed if that stopped working.
If it can't contact the server at all, then you will have to click stop before you can submit again. If it submits to the same window and takes a long time to load, when you hit back submit will work again.
Comment on attachment 72899 [details] [diff] [review] Patch (synchronous click) ok, I guess I can't see any way around the dependency right now, but I'm glad we cleaned this up anyway... the patch looks fine, sr=alecf but I'd like to see reviews from both darin and a layout person (just so everyone is happy on both ends)
Attachment #72899 - Flags: superreview+
Comment on attachment 72899 [details] [diff] [review] Patch (synchronous click) r=darin... looks good to me.
Attachment #72899 - Flags: review+
Blocks: 129196
layout is happy (particulary me :-) r=alexsavulov
Comment on attachment 72899 [details] [diff] [review] Patch (synchronous click) a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #72899 - Flags: approval+
Fix checked in. Bug 130491 is filed for visual representation of the non-submittable state.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago23 years ago
Resolution: --- → FIXED
Verifying on windows 98 build 2002-03-13-08-trunk and mac OSX build 2002-03-13-08-trunk
Status: RESOLVED → VERIFIED
*** Bug 130579 has been marked as a duplicate of this bug. ***
verifying on build 2002-03-13-09-trunk linux RedHat 7.0 as well
Caused regression: bug 133895, offline cache miss on form submission breaks form permanently.
I'm seeing other wierd issues with certain forms where a normal submission results in a cache warning (i.e. for instance stuff about POSTDATA expiring even though I'm submitting the firm, not hitting forward/back/etc)
alec: i'm seeing that too, especially w/ the 2002-03-26-08 build under linux. it seems to happen very often when i focus on another window before the form submission completes.
Hmmm ... I suggest filing another bug, that sounds like it might be related but might not too. (I am sort of skeptical, as bug 72906 is only passive--it only listens, and does not change the character of the first submit. Subsequent submits just don't happen--there's no chance for a partial submit or anything like that.)
Component: HTML: Form Submission → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: