Closed
Bug 136906
Opened 22 years ago
Closed 22 years ago
When invoked through a html form, the compose window appears only once
Categories
(Core :: Layout: Form Controls, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: daisuke, Assigned: john)
References
()
Details
(Whiteboard: [FIX])
Attachments
(1 file, 1 obsolete file)
3.93 KB,
patch
|
john
:
review+
john
:
superreview+
jesup
:
approval+
|
Details | Diff | Splinter Review |
From Bugzilla Helper: User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.9+) Gecko/20020410 BuildID: 20020410 When there is a form that starts up the mail composer in an html page, the composer only successfully comes up on the first invokation. From the second invokation on, the clicks to the button are swiftly ignored. However, if you reload the page, the same button works again, but only once. I think this has been happening for a while. ( last couple of weeks? ) Reproducible: Always Steps to Reproduce: 1. go to any page which contains a form like so: <form action="mailto:foo@bar.com"> <input type="submit" value="mail me!"> </form> 2. press the button. either send the mail, or just close the compose window 3. press the button again. No window comes up
Comment 1•22 years ago
|
||
jkeiser, this sounds like a possible result of your "prevent duplicate submissions" fix...
Assignee | ||
Comment 2•22 years ago
|
||
It's mine, but I don't think I'll deliberately fix it unless it's part of bug 61893. There's very little point otherwise. Still unconfirmed, and may even be working in recent builds, because I haven't had time to turn the HTML below into a testcase.
Assignee: rods → jkeiser
While I don't consider this to be a real high priority thing, I really don't think this conforms to what people would expect. Also, there is some cultural reason here... a lot of people in the Japanese online community has what's known as an "empty-mail" button on their webpages. This is used as an explicit sign that you have visited the site. It's also used as a way to communicate that you approve of certain things on their pages, in which case you hit the "empty-mail" button multiple times. I don't think this phenomenon exist in the U.S., but it's surely there. Besides, all other major browsers allow that to happen. People are already used to it. And bringing up the mail composer multiple times certainly doesn't do any major harm. So why not? I know it sounds stupid, but I'm pretty sure that other Japanese surfers like myself *will* find this a nuisance...
Assignee | ||
Comment 4•22 years ago
|
||
(1) the mail compose window *does* open. (2) if a site wants an empty mail window it can do a mailto: link rather than form post. (3) I'm not saying it's not a bug. I'm just saying I'm not spending the time to fix it until I fix mailto form submit. That said, please try a recent nightly. I know some work has gone into the tree recently that may have already fixed this.
Taking your suggestion, I tried the 20020416 build. same deal :( For your point (1),I'm not sure what you meant by "the mail compose window *does* open", because from the seond invokation on, it definitely doesn't come up for me. Do you actually get the compose window from the second invokation on? Am I missing something? For point (2), while appreciated, I think a workaround is besides the point. I would think the problem is the fact that something that used to work doesn't any more... And finally for Point (3), sorry, I guess I misread your orignal comment. I thought it was a flat out no. Understood...
Assignee | ||
Comment 6•22 years ago
|
||
(1) was the fact that it opens once. If it didn't open at all it would be worse severity.
Updated•22 years ago
|
QA Contact: madhur → tpreston
Updated•22 years ago
|
Severity: normal → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 7•22 years ago
|
||
OK, this is still happening, and I've diagnosed the problem. Mail compose from LoadURI is returning a channel, but it is not sending the START and STOP events, so since we never receive a STOP event. Based on conversations I had with Rick a couple of months ago, I believe this violates the rules of the interface and should be fixed in mail/news. Should it not be returning a channel at all? Or is it just that the channel needs to send dummy stop/start requests? Rick, Darin, Jean-Francois, which of these sounds like the right thing? Or am I off base?
Comment 8•22 years ago
|
||
hey john, it appears that the 'mailto' channel is not adding itself to a loadgroup when it is opened... take a look at: http://lxr.mozilla.org/seamonkey/source/mailnews/compose/src/nsSmtpService.cpp#439 In order for the nsIWebProgressListener to fire notifications, each channel must be added/removed from the correct loadgroup... since this channel doesn't handle loadgroups (at all), no notifications get fired. The fix is to have the mailto channel add/removed itself from the correct loadgroup (just like it fires OnStart/OnStop events) ... -- rick
Comment 9•22 years ago
|
||
I cannot really anwser that question as this chanel thing append way before message compose get invoked! cc'ing mscott who I hope should be able to answer it
Assignee | ||
Comment 10•22 years ago
|
||
Both form submit and mailto had troubles. I updated the mailto channel to add / remove itself from the load group and to report that it is *not* pending (pending is defined as "STOP has not fired yet"). And I updated form submit to check whether a request is pending before listening to web progress. All is well with mailto now, and double form submit protection still works for http.
Assignee | ||
Updated•22 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [FIX]
Comment 11•22 years ago
|
||
Comment on attachment 93265 [details] [diff] [review] Patch >Index: mailnews/compose/src/nsSmtpService.cpp >+ mLoadGroup = aLoadGroup; > return NS_OK; > } indentation fu! just make it consistent with the existing file (whatever that means) > NS_IMETHODIMP nsMailtoChannel::AsyncOpen(nsIStreamListener *listener, nsISupports *ctxt) > { >+ // Add to the load group to fire start event >+ if (mLoadGroup) { >+ mLoadGroup->AddRequest(this, ctxt); >+ } >+ > mStatus = listener->OnStartRequest(this, ctxt); > > // If OnStartRequest(...) failed, then propagate the error code... >@@ -449,6 +457,11 @@ > // Call OnStopRequest(...) for correct-ness. > (void) listener->OnStopRequest(this, ctxt, mStatus); > >+ // Remove from the load group to fire stop event >+ if (mLoadGroup) { >+ mLoadGroup->RemoveRequest(this, ctxt, NS_OK); >+ } >+ yuck... OnStartRequest, OnStopRequest are not supposed to be called recursively from AsyncOpen. that is trouble waiting to happen. maybe we're okay because of the way this channel is used, but watch out! r/sr=darin
Attachment #93265 -
Flags: review+
Comment 12•22 years ago
|
||
In this call to RemoveRequest(...) i think that 'mStatus' should be passed instead of NS_OK so listeners recieve the same status as that passed to OnStopRequest(...) + // Remove from the load group to fire stop event + if (mLoadGroup) { + mLoadGroup->RemoveRequest(this, ctxt, NS_OK); + } Other than that... it looks good. -- rick
Comment 13•22 years ago
|
||
Comment on attachment 93265 [details] [diff] [review] Patch sr=rpotts@netscape.com (with change to RemoveRequest)
Attachment #93265 -
Flags: superreview+
Assignee | ||
Comment 14•22 years ago
|
||
Fix bits from darin and rpotts
Attachment #93265 -
Attachment is obsolete: true
Assignee | ||
Comment 15•22 years ago
|
||
Comment on attachment 93472 [details] [diff] [review] Patch v1.0.1 Carrying r=darin, sr=rpotts
Attachment #93472 -
Flags: superreview+
Attachment #93472 -
Flags: review+
Comment 16•22 years ago
|
||
Comment on attachment 93472 [details] [diff] [review] Patch v1.0.1 Approved for trunk
Attachment #93472 -
Flags: approval+
Assignee | ||
Comment 17•22 years ago
|
||
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•