When invoked through a html form, the compose window appears only once

RESOLVED FIXED

Status

()

--
minor
RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: daisuke, Assigned: john)

Tracking

Trunk
x86
Windows 2000
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [FIX], URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

17 years ago
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...
(Assignee)

Comment 2

17 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
(Reporter)

Comment 3

17 years ago
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

17 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.
(Reporter)

Comment 5

17 years ago
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

17 years ago
(1) was the fact that it opens once.  If it didn't open at all it would be worse
severity.

Updated

17 years ago
QA Contact: madhur → tpreston
Severity: normal → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

17 years ago
Depends on: 61893
(Assignee)

Comment 7

17 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

17 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
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

17 years ago
Created attachment 93265 [details] [diff] [review]
Patch

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

17 years ago
Status: NEW → ASSIGNED
Whiteboard: [FIX]

Comment 11

17 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

17 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

17 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

17 years ago
Created attachment 93472 [details] [diff] [review]
Patch v1.0.1

Fix bits from darin and rpotts
Attachment #93265 - Attachment is obsolete: true
(Assignee)

Comment 15

17 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 on attachment 93472 [details] [diff] [review]
Patch v1.0.1

Approved for trunk
Attachment #93472 - Flags: approval+
(Assignee)

Comment 17

17 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.