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)

x86
Windows 2000
defect
Not set
minor

Tracking

()

RESOLVED FIXED

People

(Reporter: daisuke, Assigned: john)

References

()

Details

(Whiteboard: [FIX])

Attachments

(1 file, 1 obsolete file)

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
jkeiser, this sounds like a possible result of your "prevent duplicate
submissions" fix...
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...
(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...
(1) was the fact that it opens once.  If it didn't open at all it would be worse
severity.
QA Contact: madhur → tpreston
Severity: normal → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
Depends on: 61893
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?
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
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
Attached patch Patch (obsolete) — Splinter Review
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.
Status: NEW → ASSIGNED
Whiteboard: [FIX]
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+
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 on attachment 93265 [details] [diff] [review]
Patch

sr=rpotts@netscape.com (with change to RemoveRequest)
Attachment #93265 - Flags: superreview+
Attached patch Patch v1.0.1Splinter Review
Fix bits from darin and rpotts
Attachment #93265 - Attachment is obsolete: true
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 on attachment 93472 [details] [diff] [review]
Patch v1.0.1

Approved for trunk
Attachment #93472 - Flags: approval+
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.

Attachment

General

Created:
Updated:
Size: